993438 - Remove implicit CloneScript in JS_ExecuteScript

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

Bill said that he got confused by this, and I offered to remove it.

Luke added this a long time ago during the CPG days. I think the comment about pinch points is wrong, and that there's only one or two places this actually needs to happen in the browser.

Comment 1

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=d47c8f82e0fd

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=ba2ae8317e02

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add the necessary APIs. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Infer the eventual compartment for OMT-compiled script when the script is retrieved. v1

Currently, the script gets put in a compartment specified when the compilation
is initiated. Later on, when we retrieve that script and try to execute it, we
won't necessarily be in the same compartment. Currently, SM just handles this
and clones the script, but it's simpler and more efficient to just create the
script in the compartment of whoever calls FinishOffThreadScript.

Note that the existing code actually has a hazard in which the call to
GetBuiltinPrototypePure returns null - it just wasn't exercised in automation
(and was triggered by onNewScript-off-main-thread.js with this patch).

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Fix up the XUL prototype cache. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Fix up the Frame Message Manager. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 5 - Stop implicitly cloning, and assertSameCompartment. v1

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8403753 [details] [diff] [review]
Part 3 - Fix up the XUL prototype cache. v1

Not happy with the approach. We want to have less explicit JSAPI usage in
DOM, not more.
Couldn't we rather have JS_CloneAndExecuteScript or some such, which would
do the cloning if needed.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'll hold off reviewing until we settle on something I guess.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 1 - Add the necessary APIs. v2

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 3 - Fix up the XUL prototype cache. v2

Note that the bulk of the previous version of this patch was mostly a giant
assertion, which I've decided to jettison so that this patch can land
independently of bug 993772.

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 4 - Fix up the Frame Message Manager. v2

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8403752 [details] [diff] [review]
Part 2 - Infer the eventual compartment for OMT-compiled script when the script is retrieved. v1

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

::: js/src/jsapi.cpp
@@ +4515,5 @@
>      return cx->runtime()->canUseParallelParsing();
>  }
>  
>  JS_PUBLIC_API(bool)
> +JS::CompileOffThread(JSContext *cx,const ReadOnlyCompileOptions &options,

Need a space after the comma.

::: js/src/jsworkers.cpp
@@ +644,5 @@
> +        !GlobalObject::ensureConstructor(cx, global, JSProto_Function) ||
> +        !GlobalObject::ensureConstructor(cx, global, JSProto_RegExp) ||
> +        !GlobalObject::ensureConstructor(cx, global, JSProto_Iterator))
> +    {
> +        return nullptr;

Don't we need to delete parseTask here? It might be nice to switch parseTask to a ScopedJSDeletePtr.

@@ +679,4 @@
>      parseTask->finish();
>  
>      RootedScript script(rt, parseTask->script);
> +    assertSameCompartment(cx, script);

I'm afraid that the shell will now assert here if runOffThreadScript is called in the wrong global. That's bad for fuzzing. Can we add a check in the code for runOffThreadScript that throws an exception if we're using the wrong global?

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8404377 [details] [diff] [review]
Part 1 - Add the necessary APIs. v2

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

::: js/src/jsfriendapi.h
@@ +643,5 @@
>  JS_FRIEND_API(JSObject *)
>  GetGlobalForObjectCrossCompartment(JSObject *obj);
>  
> +JS_FRIEND_API(JSObject *)
> +GetGlobalForScriptCrossCompartment(JSScript *script);

It looks like we don't need this anymore?

Comment 15

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #13)
> >      RootedScript script(rt, parseTask->script);
> > +    assertSameCompartment(cx, script);
> 
> I'm afraid that the shell will now assert here if runOffThreadScript is
> called in the wrong global. That's bad for fuzzing. Can we add a check in
> the code for runOffThreadScript that throws an exception if we're using the
> wrong global?

I don't understand. The whole point of this patch is make sure that it's impossible to do things with the wrong global, because we just merge the script into whatever compartment the caller is in. This assertSameCompartment is just asserting that gc::MergeCompartments did what it said it would.

Comment 16

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #15)
> I don't understand. The whole point of this patch is make sure that it's
> impossible to do things with the wrong global, because we just merge the
> script into whatever compartment the caller is in. This
> assertSameCompartment is just asserting that gc::MergeCompartments did what
> it said it would.

Indeed, we could indeed assert in the shell before this patch, but not after.

https://tbpl.mozilla.org/?tree=Try&rev=ab29facc81ca

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Sorry, I wasn't thinking. That seems fine.

Comment 18

[Bobby Holley (:bholley)]

> <bholley> RyanVM: this is a try push I did yesterday. Do those bc1 failures relate to anything that was pre-existing on the tree, or are
> they mine? https://tbpl.mozilla.org/?tree=Try&rev=ab29facc81ca
> <RyanVM> bholley: known issue
> <RyanVM>	 you shoudl be able to retrigger them now
> <bholley> RyanVM: ok. I'll just push

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d062adef7edc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4745f8a481a4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c83dfbd8e41
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4905c155c80f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ea52006e7c56

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d062adef7edc
https://hg.mozilla.org/mozilla-central/rev/4745f8a481a4
https://hg.mozilla.org/mozilla-central/rev/8c83dfbd8e41
https://hg.mozilla.org/mozilla-central/rev/4905c155c80f
https://hg.mozilla.org/mozilla-central/rev/ea52006e7c56