Closed Bug 1485031 Opened 2 years ago Closed 2 years ago

Shell module APIs should use wrapper objects to represent scripts

Categories

(Core :: JavaScript Engine, enhancement)

61 Branch
enhancement
Not set
normal

Tracking

()

RESOLVED INVALID
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(2 files)

Currently the shell module APIs/hooks pass the internal ModuleObject to represent a module.  This allowed us to easily write the shell module loader in JS, but it's a problem for dynamic module import because we will need to be able to pass classic scripts to some of the hooks as well (e.g. HostResolveImportedModule, HostImportModuleDynamically).

We should change these APIs to operate in terms of a wrapper object that represents a module or classic script.
Patch to add these wrapper objects.

This adds ShellScriptObject to the shell and a weak cache table mapping scripts to these objects in the shell compartment private structure.  This allows us to pass one of these objects when we want to pass a JSScript pointer to JS.

I had to make grayRoots a GCPtr now that these actually get deleted, because otherwise the destructor complains, and therefore also had to add a manual expose barrier when reading the value out of it.  This is a bit annoying.

parseModule now returns one of these wrapper objects too so that meant a ton of test code changes.  Sorry about that.  

I added helper functions to instantiate and evaluate module scripts given one of these wrapper objects and also a function to extract the module object for tests that need to access this.  This all ends up shell only, and I moved a couple functions from TestingFunctions.cpp to js.cpp too.
Attachment #9002782 - Flags: review?(sphink)
Comment on attachment 9002782 [details] [diff] [review]
bug1485031-shell-script-wrapper

Review of attachment 9002782 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/shell/js.cpp
@@ +4362,5 @@
> +
> +JSScript*
> +ShellScriptObject::script() const
> +{
> +    return static_cast<JSScript*>(getReservedSlot(ScriptSlot).toGCThing());

I think this can be

    return getReservedSlot(ScriptSlot).toGCThing()->as<JSScript*>();

@@ +4442,5 @@
> +                                  "instantiateModule", "0", "s");
> +        return false;
> +    }
> +
> +    if (!args[0].isObject() || !args[0].toObject().is<ShellScriptObject>())

I don't know if it matters, but this will disallow a cross-compartment wrapper to a ShellScriptObject.

@@ +4466,5 @@
> +                                  "evaluateModule", "0", "s");
> +        return false;
> +    }
> +
> +    if (!args[0].isObject() || !args[0].toObject().is<ShellScriptObject>())

same (and more later, but I'll stop mentioning them)

@@ +4586,5 @@
> +        JS_ReportErrorASCII(cx, "Module environment unavailable");
> +        return nullptr;
> +    }
> +
> +    // Use the initial environment so that tests can check bindings exists

*exist

@@ +4589,5 @@
> +
> +    // Use the initial environment so that tests can check bindings exists
> +    // before they have been instantiated.
> +    RootedModuleEnvironmentObject env(cx, &module->initialEnvironment());
> +    MOZ_ASSERT(env);

I'm not sure if this assertion is needed given that you're taking a reference to module->initialEnvironment(), but it's fine.

@@ +4642,5 @@
> +        JS_ReportErrorASCII(cx, "First argument should be a ShellScriptObject");
> +        return false;
> +    }
> +
> +    if (!args[1].isString()) {

Would these ever be numeric? I'm wondering if it would be better to use ToString here and for name, below.
Attachment #9002782 - Flags: review?(sphink) → review+
Depends on: 1485615
(In reply to Steve Fink [:sfink] [:s:] from comment #2)

Thanks for the comments.

> > +    if (!args[0].isObject() || !args[0].toObject().is<ShellScriptObject>())
> 
> I don't know if it matters, but this will disallow a cross-compartment
> wrapper to a ShellScriptObject.

I don't think we ever want to be passing these between compartments so I'll leave it as is.

> > +    if (!args[1].isString()) {
> 
> Would these ever be numeric? I'm wondering if it would be better to use
> ToString here and for name, below.

Good idea, fixed.
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71791e10b6e9
Use wrapper object to represent scripts in shell module APIs r=sfink
https://hg.mozilla.org/mozilla-central/rev/71791e10b6e9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
As explained in bug 1481196 comment 6 I'm going to back this out.

Just running this past you as it's not a straight backout due to intervening changes.
Attachment #9014063 - Flags: review?(sphink)
Comment on attachment 9014063 [details] [diff] [review]
backout-bug1485031-shell-script-wrapper

Review of attachment 9014063 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/TestingFunctions.cpp
@@ +4553,5 @@
> +
> +    RootedModuleEnvironmentObject env(cx, GetModuleEnvironment(cx, module));
> +    Rooted<IdVector> ids(cx, IdVector(cx));
> +    if (!JS_Enumerate(cx, env, &ids))
> +        return false;

I don't know if some style-checker is going to yell at you yet, but these now need { brackets }. I don't know if it's easier to keep this backout relatively clean, and then add the brackets in another patch?

@@ +4558,5 @@
> +
> +    uint32_t length = ids.length();
> +    RootedArrayObject array(cx, NewDenseFullyAllocatedArray(cx, length));
> +    if (!array)
> +        return false;

another

@@ +4562,5 @@
> +        return false;
> +
> +    array->setDenseInitializedLength(length);
> +    for (uint32_t i = 0; i < length; i++)
> +        array->initDenseElement(i, StringValue(JSID_TO_STRING(ids[i])));

another

@@ +4597,5 @@
> +    RootedModuleEnvironmentObject env(cx, GetModuleEnvironment(cx, module));
> +    RootedString name(cx, args[1].toString());
> +    RootedId id(cx);
> +    if (!JS_StringToId(cx, name, &id))
> +        return false;

(more below)
Attachment #9014063 - Flags: review?(sphink) → review+
Resolution: FIXED → INVALID
You need to log in before you can comment on or make changes to this bug.