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.

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

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

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 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 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?

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

(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

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

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

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