Shell module APIs should use wrapper objects to represent scripts

RESOLVED INVALID

Status

()

enhancement
RESOLVED INVALID
10 months ago
8 months ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

61 Branch
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

10 months ago
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.
Assignee

Comment 1

10 months ago
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+
Assignee

Updated

10 months ago
Depends on: 1485615
Assignee

Comment 3

10 months ago
(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.

Comment 4

10 months ago
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

Comment 5

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71791e10b6e9
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Assignee

Comment 6

9 months ago
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+
Assignee

Updated

8 months ago
Resolution: FIXED → INVALID
You need to log in before you can comment on or make changes to this bug.