Closed Bug 1031636 Opened 6 years ago Closed 6 years ago

Initialization of ScriptSourceObjects from ReadOnlyCompileOptions should be simpler

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jimb, Assigned: jimb)

References

Details

Attachments

(3 files)

JS::ReadOnlyCompileOptions can include an owning element, element attribute name, and introduction script, all of which need to be propagated to the ScriptSourceObject of the compiled code.

At present, it's the responsibility of whoever's setting the option values to make sure they are properly wrapped for the compartment in which we'll build the ScriptSourceObject. But this is fragile; bug 944121 and bug 1018916 are related. And the code here complains about the difficulty of getting this right:

https://hg.mozilla.org/mozilla-central/file/c90b38c47a1d/content/base/src/nsScriptLoader.cpp#l1091

Instead, the JavaScript compiler should take responsibility for ensuring option values are appropriately wrapped before storing them in the ScriptSourceObject. It has all the information it needs to do so.
Here's a patch that tries to clean all this up. It seems to simplify a bunch of things in the process.

https://tbpl.mozilla.org/?tree=Try&rev=5106fde6e463
Assignee: nobody → jimb
Status: NEW → ASSIGNED
If the above patch lands, here's the browser code that I think could come out.
Comment on attachment 8447590 [details] [diff] [review]
In off-thread compilation, rewrap the compilation options that might need it before saving them on the ScriptSourceObject.

Try push looks good; requesting review.
Attachment #8447590 - Flags: review?(sphink)
Comment on attachment 8447590 [details] [diff] [review]
In off-thread compilation, rewrap the compilation options that might need it before saving them on the ScriptSourceObject.

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

I did not achieve a full mind-meld with this code, but from what I can tell, this looks great.

::: js/src/frontend/BytecodeCompiler.cpp
@@ +214,5 @@
> +    // until after we've merged compartments.
> +    if (cx->isJSContext()) {
> +        if (!sso->initFromOptions(cx->asJSContext(), options))
> +            return nullptr;
> +    }

Is there an assertion somewhere that would fire if the initialization didn't happen? Like maybe one of those SSO slots is guaranteed to be defined, and it gets asserted or something? Or we stick in a magic value here, and it has to be set to that or already filled in? Or something. I don't know the flow well enough to suggest anything concrete; I'm just worried about uninitialized SSO's escaping somehow.

::: js/src/jsscript.cpp
@@ +1284,5 @@
>      if (!object)
>          return nullptr;
>      RootedScriptSource sourceObject(cx, &object->as<ScriptSourceObject>());
>  
>      source->incref();

Can you add a comment, something like "// decremented in ::finalize"?

@@ +1315,5 @@
> +    // we would be creating a cross-compartment script reference, which is
> +    // forbidden. In that case, simply don't bother to retain the introduction
> +    // script.
> +    if (options.introductionScript() &&
> +        options.introductionScript()->compartment() == cx->compartment()) {

Put the { on its own line.
Attachment #8447590 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] from comment #4)
> Comment on attachment 8447590 [details] [diff] [review]
> In off-thread compilation, rewrap the compilation options that might need it
> before saving them on the ScriptSourceObject.
> 
> Review of attachment 8447590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I did not achieve a full mind-meld with this code, but from what I can tell,
> this looks great.
> 
> ::: js/src/frontend/BytecodeCompiler.cpp
> @@ +214,5 @@
> > +    // until after we've merged compartments.
> > +    if (cx->isJSContext()) {
> > +        if (!sso->initFromOptions(cx->asJSContext(), options))
> > +            return nullptr;
> > +    }
> 
> Is there an assertion somewhere that would fire if the initialization didn't
> happen? Like maybe one of those SSO slots is guaranteed to be defined, and
> it gets asserted or something? Or we stick in a magic value here, and it has
> to be set to that or already filled in? Or something. I don't know the flow
> well enough to suggest anything concrete; I'm just worried about
> uninitialized SSO's escaping somehow.

ScriptSourceObject::create could store poison values in the slots it expects initFromObjects to fill.


> ::: js/src/jsscript.cpp
> @@ +1284,5 @@
> >      if (!object)
> >          return nullptr;
> >      RootedScriptSource sourceObject(cx, &object->as<ScriptSourceObject>());
> >  
> >      source->incref();
> 
> Can you add a comment, something like "// decremented in ::finalize"?

Done.


> @@ +1315,5 @@
> > +    // we would be creating a cross-compartment script reference, which is
> > +    // forbidden. In that case, simply don't bother to retain the introduction
> > +    // script.
> > +    if (options.introductionScript() &&
> > +        options.introductionScript()->compartment() == cx->compartment()) {
> 
> Put the { on its own line.

Done.
Here's the revised patch. A fresh try push, since we added some poisoning:
https://tbpl.mozilla.org/?tree=Try&rev=172e2eead913
The hazards static analysis caught a rooting hazard. Fixed and pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b796d6062a98
Flags: in-testsuite-
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/b796d6062a98
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 1308048
You need to log in before you can comment on or make changes to this bug.