Closed Bug 944121 Opened 6 years ago Closed 6 years ago

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


(Core :: JavaScript Engine, defect)

Not set





(Reporter: jimb, Assigned: jimb)




(5 files)

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.
Assignee: nobody → jimb
Attachment #8351673 - Flags: review?(wmccloskey) → review?(bhackett1024)
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.
Attachment #8351672 - Flags: review?(bhackett1024) → review+
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;

Attachment #8351673 - Flags: review?(bhackett1024) → review+
Attachment #8351674 - Flags: review?(bhackett1024) → review+
Attachment #8351675 - Flags: review?(bhackett1024) → review+
Depends on: 952885
Okay, bug 952885 and its blocker have all the necessary patches to let this work move forward.
Err, I meant to put that comment in bug 941876...
The static rooting hazard analysis found a bug in this patch; fixed and re-pushed.
(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.)
You need to log in before you can comment on or make changes to this bug.