Wrapping unsafe raw JSContext pointers in a safe struct. Part 1.

[marmeladema]

Wrapping unsafe raw JSContext pointers in a safe struct. Issue #20377.

---

• create a struct JSContext(*mut jsapi::JSContext) type
• implement Deref for this new type so it's easy to obtain the inner context pointer
• add an unsafe from_ptr static method that returns a new instance of the type
• start by changing various Argument uses in CodegenRust.py for python classes that inherit from CGAbstractMethod
• convert the python code that interacts with CGClass (CGCallback, CGCallbackFunction, CGCallbackFunctionImpl, CGCallbackInterface) and any rust code that interacts with it
• update CGDictionary and any rust code that interacts with it
• make the code generator declare trait methods that accept the new type instead of *mut JSContext (CGInterfaceTrait)
• modify GlobalScope::get_cx to return a SafeJSContext

---

This change is 

[highfive]

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @emilio (or someone else) soon.

[highfive]

Heads up! This PR modifies the following files:

• @asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/node.rs, components/script/dom/testworkletglobalscope.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/bindings/reflector.rs and 6 more
• @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/node.rs, components/script/dom/testworkletglobalscope.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/bindings/reflector.rs and 6 more

[highfive]

Warning

• These commits modify unsafe code. Please review it carefully!
• These commits modify script code, but no tests are modified. Please consider adding a test!

[marmeladema]

@jdm i updated the code according to your comments. I am heading toward the right direction here?

[jdm]

Yes! This looks like what I expected to see :D

[marmeladema]

I am having trouble modifying CGAbstractStaticBindingMethod because i end up having a bunch of errors like

error[E0308]: mismatched types
   --> /home/adema/code/servo/target/debug/build/script-537c159507a8c0bf/out/Bindings/ConsoleBinding.rs:808:42
    |
808 |         call: JSNativeWrapper { op: Some(time), info: 0 as *const JSJitInfo },
    |                                          ^^^^ expected *-ptr, found struct `script_runtime::JSContext`
    |
    = note: expected type `unsafe extern "C" fn(*mut js::jsapi::JSContext, u32, *mut js::jsapi::Value) -> bool`
               found type `unsafe extern "C" fn(script_runtime::JSContext, u32, *mut js::jsapi::Value) -> bool {dom::bindings::codegen::Bindings::ConsoleBinding::ConsoleBinding::time}`

which i don't know how to solve.

[jdm]

Ah, indeed. Those will need to continue to have the raw pointer argument because the C function pointers can't be changed. We will want to declare a let cx = SafeJSContext::from_ptr(cx) as the first line of code in them instead.

[jdm]

Looking good so far!

[marmeladema]

@jdm i've followed the steps you described in the other PR #23657 (comment)
We do you think are the next steps here?

Also taskcluster is often failing but it does not look it has anything to do with my changes. I think it fails when downloading some rust components.

[jdm]

The failure does come from your changes. If you run./mach test-tidy you should reproduce it.

[jdm]

I think the next big step would be modifying GlobalScope::get_cx to return a SafeJSContext and store one internally as well. That should remove a bunch of uses of from_ptr uses right now. At that point it would probably make sense to merge the changes and file follow-ups for increasing the usage in hand-written Rust code.

[marmeladema]

The failure does comefromyourchanges. If you run/Mach test-tidy you should reproduce it.

Ok it should be fixed now.

I think the next big step would be modifying GlobalScope::get_cx to return a SafeJSContext and store one internally as well. That should remove a bunch of uses of from_ptr uses right now. At that point it would probably make sense to merge the changes and file follow-ups for increasing the usage in hand-written Rust code.

I'll look into it then 👍

[marmeladema]

@jdm I made all methods *::get_cx return a safe JSContext but I did not store one inside GlobalScope since it does not store one itself and uses Runtime::get() instead.

[jdm]

Great work! Let's fix these two nits and merge it!

[bors-servo]

🔒 Merge conflict

[marmeladema]

@jdm i rebased and fixed the conflicts but appveyor failed and i don't really understand what is the problem. I don't know what to look for in the build log.

[jdm]

It hit the time limit for the build and it's very bad at making that clear. We can ignore it.

[jdm]

@bors-servo r+

[bors-servo]

📌 Commit 88cacfb has been approved by jdm

[bors-servo]

⌛ Testing commit 88cacfb with merge 9b14bbd...

[bors-servo]

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
Approved by: jdm
Pushing 9b14bbd to master...