1031636 - Initialization of ScriptSourceObjects from ReadOnlyCompileOptions should be simpler

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

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.

Comment 1

[Jim Blandy :jimb]

Attachment: In off-thread compilation, rewrap the compilation options that might need it before saving them on the ScriptSourceObject.

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

Comment 2

[Jim Blandy :jimb]

Attachment: Don't worry about the compartment of the owning element for a script's JS code.

If the above patch lands, here's the browser code that I think could come out.

Comment 3

[Jim Blandy :jimb]

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.

Comment 4

[Steve Fink [:sfink] [:s:]]

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.

Comment 5

[Jim Blandy :jimb]

(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.

Comment 6

[Jim Blandy :jimb]

Attachment: revised per comments

Here's the revised patch. A fresh try push, since we added some poisoning:
https://tbpl.mozilla.org/?tree=Try&rev=172e2eead913

Comment 7

[Jim Blandy :jimb]

The hazards static analysis caught a rooting hazard. Fixed and pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b796d6062a98

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b796d6062a98