944121 - CompileOptions GC things don't play well with off-thread compilation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jim Blandy :jimb]

Off-thread compilation fails with a cross-compartment assertion if the CompileOptions has an owning element set (with CompileOptions::setElement).

When we compile JS off the main thread, we create a separate, temporary compartment in which to do the compilation, with its own global, whose class is named "internal-worker-global". Once the compilation has produced a JSScript, we *merge the temporary compartment* with the compartment that actually requested the compilation, and *repoint all the references to the temporary global that we anticipate* to the proper global.

So, the ScriptSourceObject gets allocated in the temporary compartment, and then complains about being asked to point to the owning element, which is in the true target compartment.

The SpiderMonkey tests really don't exercise off-main-thread compilation very well. In order to test it, we'll need to create a version of js/src/shell/js.cpp's OffThreadCompileScript that has all the options of js/src/shell/js.cpp's Evaluate.

One fix would be for ParseTask::init, or possibly OwningCompileOptions::copy, to not actually copy over those elements of the CompileOptions that have compartments --- and then for WorkerThreadState::finishParseTask to store them in the new script's ScriptSourceObject, after merging the compartments.

Other possibilities might be:
1) creating the ScriptSourceObject early, in the right compartment, and then attaching it when compilation is finished.

2) creating the ScriptSourceObject late, etc.

But it seems to me those involve a lot more plumbing, because they deviate from the current approach: creating the ScriptSourceObject in the same compartment as its JSScript, and letting the compartment merge put everything in the right place. Better to treat element and elementProperty (should be elementAttribute) as yet more things to be fixed up in finishParseTask, which is the place where we do all sorts of similar fixups.

Comment 1

[Jim Blandy :jimb]

Attachment: Make js::ParseTask use PersistentRooted instead of AddObjectRoot / JS_RemoveObjectRoot.

Comment 2

[Jim Blandy :jimb]

Attachment: Abstract JS shell's compilation options parsing out into its own function.

Comment 3

[Jim Blandy :jimb]

Attachment: Add options argument to the JS shell's offThreadCompileScript function.

Comment 4

[Jim Blandy :jimb]

Attachment: For off-thread compilation, put off initializing some slots of ScriptSourceObject until after the compartment merge.

Comment 5

[Jim Blandy :jimb]

Note to self: the third patch adds a test which fails, and is fixed by the fourth; so they should be merged before landing, to avoid breaking bisection too much.

Comment 6

[Brian Hackett [Laid off!]]

Comment on attachment 8351673 [details] [diff] [review]
Abstract JS shell's compilation options parsing out into its own function.

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

::: js/src/shell/js.cpp
@@ +847,5 @@
> +            return false;
> +        }
> +    }
> +
> +return true;

whitespace

Comment 7

[Jim Blandy :jimb]

Okay, bug 952885 and its blocker have all the necessary patches to let this work move forward.

Comment 8

[Jim Blandy :jimb]

Err, I meant to put that comment in bug 941876...

Comment 9

[Jim Blandy :jimb]

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

Comment 10

[Jim Blandy :jimb]

Attachment: Add options argument to the JS shell's offThreadCompileScript function. r=bhackett

The static rooting hazard analysis found a bug in this patch; fixed and re-pushed.

Comment 11

[Jim Blandy :jimb]

... to try, that is:
https://tbpl.mozilla.org/?tree=Try&rev=a5d474b60dd8

Comment 12

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a9e712e2f63
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b21c9d16999
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5da9f1e91fe
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bcd545cf484

Comment 13

[Jim Blandy :jimb]

(Note that those four changesets are all five patches. The second-to-last introduces a test that fails; the last patch fixes the test. I merged the two to avoid introducing bisection problems.)

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a9e712e2f63
https://hg.mozilla.org/mozilla-central/rev/8b21c9d16999
https://hg.mozilla.org/mozilla-central/rev/d5da9f1e91fe