1284808 - Move RuntimeOptions to JSContext

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

This renames the RuntimeOptions class to ContextOptions and moves it to the context.

It revealed a few places where background threads used these options (that's racy). I fixed that by moving the options to ExclusiveContext, so each background thread now gets a (read-only) copy of the main thread's ContextOptions.

Luke, asking you to review this so you're aware of these changes (it conflicts with the no-cx wasm patches).

r? baku for the dom/ changes.

Comment 1

[Andrea Marchesini [:baku]]

Comment on attachment 8768348 [details] [diff] [review]
Patch

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

::: dom/workers/WorkerPrivate.cpp
@@ +1451,2 @@
>                                      WorkerPrivate* aWorkerPrivate,
> +                                    const JS::ContextOptions& aContextOptions)

can you fix the indentation here?

Something like:

UpdateContextOptionsRunnable(WorkerPrivate* aWorkerPrivate,
                             const JS::ContextOptions& aContextOptions)

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 8768348 [details] [diff] [review]
Patch

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

::: js/src/asmjs/WasmGenerator.cpp
@@ +820,5 @@
>                                            Move(fg->callSiteLineNums_));
>      if (!func)
>          return false;
>  
> +    bool baselineCompile = cx_->options().wasmAlwaysBaseline() && BaselineCanCompile(fg);

Ah, I see I am one of the guilty ones :|  With bug 1284056 landed, this line lives in WasmCompile.cpp : CompileArgs::init(JSContext*).

::: js/src/jscntxt.h
@@ +119,5 @@
>  
> +  protected:
> +    // Background threads get a read-only copy of the main thread's
> +    // ContextOptions.
> +    JS::ContextOptions options_;

Hah, yes, much less racy.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc866385dd01
Rename RuntimeOptions to ContextOptions and move it to the context. r=luke,baku

Comment 4

[Carsten Book [:Tomcat]]

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

Comment 5

[B.J. Herbison]

This change appears to be causing serious problems for some Linux users. See bug 1285243.

Comment 6

[Jan de Mooij [:jandem]]

(In reply to B.J. Herbison from comment #5)
> This change appears to be causing serious problems for some Linux users. See
> bug 1285243.

According to bug 1285243 comment 6, it's not this bug, but bug 1284440.