[WIP] Added JSContext struct with implementation #23657

oneturkmen · 2019-06-28T20:20:29Z


        [WIP] Added JSContext struct with implementation

highfive · 2019-06-28T20:20:35Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/script_runtime.rs
@KiChjang: components/script/script_runtime.rs

highfive · 2019-06-28T20:20:37Z

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!

oneturkmen · 2019-07-09T20:43:10Z

components/script/dom/bindings/utils.rs

@@ -388,7 +389,7 @@ pub unsafe extern "C" fn resolve_global(
    let bytes = slice::from_raw_parts(ptr, length as usize);

    if let Some(init_fun) = InterfaceObjectMap::MAP.get(bytes) {
-        init_fun(cx, Handle::from_raw(obj));
+        init_fun(*cx, Handle::from_raw(obj));


@jdm is this what you meant (in terms of function call changes)?

In this case, you modified MAP to store functions that accept script_runtime::JSContext. enumerate_global and resolve_global either need to accept cx: script_runtime::JSContext (and pass it unmodified to init_fun) or cx: *mut JSContext (and pass it to init_fun by creating a new script_runtime::JSContext based on cx).

oneturkmen · 2019-07-09T20:44:12Z

components/script/build.rs

@@ -47,7 +47,7 @@ fn main() {
    let mut phf = File::create(&phf).unwrap();
    write!(
        &mut phf,
-        "pub static MAP: phf::Map<&'static [u8], unsafe fn(*mut JSContext, HandleObject)> = "
+        "pub static MAP: phf::Map<&'static [u8], /* unsafe */ fn(/* *mut */ JSContext, HandleObject)> = "


@jdm same here. I commented unsafe and *mut for now since we accept safe wrapped JSContext now.

(sorry for the mess, I should've removed those)

oneturkmen · 2019-07-09T20:45:14Z

components/script/dom/bindings/callback.rs

@@ -15,7 +15,8 @@ use crate::dom::globalscope::GlobalScope;
 use crate::dom::window::Window;
 use js::jsapi::Heap;
 use js::jsapi::JSAutoRealm;
-use js::jsapi::{AddRawValueRoot, IsCallable, JSContext, JSObject};
+use js::jsapi::{AddRawValueRoot, IsCallable, /*JSContext, */JSObject};
+use crate::script_runtime::JSContext;


@jdm and I also tried to re-import a new, safe JSContext. I am not sure if this helps since the compiler just spits out a lot of errors.

Yeah, I would expect this to cause a lot of errors because now there are many functions accepting *mut script_runtime::JSContext. I would recommend undoing the changes to most of these files. The only time we should be changing code from cx to *cx is when cx is of type script_runtime::JSContext and the receiving code expects *mut jsapi::JSContext.

jdm

Rather than changing all of the other files in components/script/dom to import script_runtime::JSContext, I think the best path forward is to only modify CodegenRust.py and get the result compiling by passing *mut JSContext to all of the code that expects that type by dereferencing the new script_runtime::JSContext values. Once everything builds, it will be much easier to change individual functions, methods, or files to use the new type and slowly update codegen to pass cx directly to the functions/methods that now expect it. Does that make sense?

oneturkmen · 2019-07-09T23:51:28Z

@jdm yes, thank you!

jdm · 2019-07-10T19:22:28Z

components/script/dom/bindings/codegen/CodegenRust.py

@@ -3314,7 +3314,7 @@ def __init__(self, errorResult, arguments, argsPre, returnType,
        needsCx = needCx(returnType, (a for (a, _) in arguments), True)

        if "cx" not in argsPre and needsCx:
-            args.prepend(CGGeneric("cx"))
+            args.prepend(CGGeneric("JSContext::from_ptr(cx)"))


I suspect that this will cause problems in the short term.

Hmm did not we need to adjust this line? i.e.:

adjust generated code to pass JSContext::from_ptr(cx) when necessary (here)

jdm · 2019-07-10T19:23:31Z

components/script/dom/bindings/codegen/CodegenRust.py

@@ -6692,7 +6692,7 @@ def argument_type(descriptorProvider, ty, optional=False, defaultValue=None, var

 def method_arguments(descriptorProvider, returnType, arguments, passJSBits=True, trailing=None, inCompartment=False):
    if needCx(returnType, arguments, passJSBits):
-        yield "cx", "*mut JSContext"
+        yield "cx", "JSContext"


I suspect this might cause problems in the short term.

Same as above. I thought we needed to change the type:

make the code generator declare trait methods that accept the new type instead of *mut JSContext (here and here; both the type )

components/script/dom/bindings/codegen/CodegenRust.py

jdm · 2019-07-10T19:26:11Z

components/script/dom/bindings/codegen/CodegenRust.py

@@ -7134,7 +7134,7 @@ def getCall(self):
            "${getCallable}"
            "rooted!(in(cx) let rootedThis = ${thisObj});\n"
            "let ok = ${callGuard}JS_CallFunctionValue(\n"
-            "    cx, rootedThis.handle(), callable.handle(),\n"
+            "    *cx, rootedThis.handle(), callable.handle(),\n"


I suspect this is going to cause problems in the short term.

Don't JS_* functions expect *mut JSContext of js::jsapi? By dereferencing, I think we provide them with the type they want. Or did I miss something?

jdm · 2019-07-10T20:23:11Z

All of my comments were made under the assumption that we are not blindly replacing the jsapi::JSContext use statements in CodegenRust.py with script_runtime::JSContext. The reason I think this is not the best path forward is that if we are strategic about replacing small numbers of them at a time, it will be easier to deal with the resulting compilation errors rather than changing all of them and having to wade through thousands of errors before a successful build.

jdm · 2019-07-10T20:23:57Z

I would really like to have checkpoints where Servo compiles with a small subset of the code that has been modified to use the new time.

oneturkmen · 2019-07-10T20:27:12Z

Ok, let me rollback and I will start integrating safe JSContext use statements in CodegenRust.py one at a time.

oneturkmen · 2019-07-10T20:36:36Z

Let's say I am starting completely from scratch. I implement the new, safe JSContext in script_runtime.rs, wrap old unsafe JSContext (now named as RawJSContext) into it, implement the Deref trait for it (so that we can get the unsafe RawJSContext when necessary) and add #[repr(transparent)] (for the FFI). We also implement the static from_ptr method that returns an instance of the new type.

Further, do we proceed with the following subtasks (points 4 and 5 in #20377)? i.e.

make the code generator declare trait methods that accept the new type instead of *mut JSContext (here and here; both the type )

adjust generated code to pass JSContext::from_ptr(cx) when necessary (here)

as well as what you mentioned with Argument(...):

That will require changing a bunch of calls in CodegenRust.py from Argument('*mut JSContext', 'cx') to Argument('JSContext', 'cx'), and then changing all of the calls to JSAPI methods from rust-mozjs (ie. ObjectIsDate, to_jsval, JS_[whatever]) that still expect *mut JSContext to pass *cx instead of cx.

Or, do I directly go and start integrating new use crate::script_runtime::JSContext statements into CodegenRust.py and see what compiler says?

Sorry for being bothersome, I am a bit confused about the steps to get this issue done.

jdm · 2019-07-10T20:56:58Z

Sorry that it seems like I've been giving contradictory suggestions! Here's how I think we should go about it, where each step is a separate commit that should build:

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)

At that point it should be much more clear where the remaining work lies.


        Playing with Argument types in CodegenRust.py

jdm · 2019-07-15T20:16:05Z

Closing due to lack of available time by the author.

[WIP] Added JSContext struct with implementation

Loading status checks…

921133d

highfive assigned nox Jun 28, 2019

highfive added the S-awaiting-review label Jun 28, 2019

jdm assigned jdm and unassigned nox Jul 4, 2019

oneturkmen reviewed Jul 9, 2019

View changes

jdm requested changes Jul 9, 2019

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Jul 9, 2019

oneturkmen force-pushed the oneturkmen:dont-take-raw-jscontext branch from fee35dd to 972699c Jul 10, 2019

highfive added S-awaiting-review and removed S-needs-code-changes labels Jul 10, 2019

jdm reviewed Jul 10, 2019

View changes

components/script/dom/bindings/codegen/CodegenRust.py Outdated Show resolved Hide resolved

jdm reviewed Jul 10, 2019

View changes

jdm removed the S-awaiting-review label Jul 10, 2019

oneturkmen force-pushed the oneturkmen:dont-take-raw-jscontext branch from 39c8f71 to 921133d Jul 10, 2019

highfive added the S-awaiting-review label Jul 10, 2019

Playing with Argument types in CodegenRust.py

Loading status checks…

f7eff63

jdm closed this Jul 15, 2019

jdm mentioned this pull request Jul 15, 2019

Make DOM bindings methods not take raw JSContext pointers #20377

Closed

marmeladema mentioned this pull request Jul 22, 2019

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

Merged

8 of 8 tasks complete

servo / servo

[WIP] Added JSContext struct with implementation #23657

[WIP] Added JSContext struct with implementation #23657

oneturkmen commented Jun 28, 2019 •

edited

highfive commented Jun 28, 2019

highfive commented Jun 28, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm left a comment

oneturkmen commented Jul 9, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Jul 10, 2019

jdm commented Jul 10, 2019

oneturkmen commented Jul 10, 2019

oneturkmen commented Jul 10, 2019

jdm commented Jul 10, 2019

jdm commented Jul 15, 2019

servo / servo

Join GitHub today

[WIP] Added JSContext struct with implementation #23657

[WIP] Added JSContext struct with implementation #23657

Conversation

oneturkmen commented Jun 28, 2019 • edited

highfive commented Jun 28, 2019

highfive commented Jun 28, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm left a comment

oneturkmen commented Jul 9, 2019

This comment has been minimized.

This comment has been minimized.

oneturkmen Jul 10, 2019 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Jul 10, 2019

jdm commented Jul 10, 2019

oneturkmen commented Jul 10, 2019

oneturkmen commented Jul 10, 2019

jdm commented Jul 10, 2019

jdm commented Jul 15, 2019

oneturkmen commented Jun 28, 2019 •

edited

oneturkmen Jul 10, 2019 •

edited