JS_NewGlobalObject should have the "right" defaults for shared memory even with default compartment options

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: lth, Assigned: lth)

Tracking

(Blocks 1 bug)

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(3 attachments)

This came up in the context of this shell test case:

  evalcx('').eval("new SharedArrayBuffer")

Here, evalcx (really NewSandbox called from EvalInContext in js.cpp) will create a new global object with default compartment options.  Those default options do not include the correct, current options for shared memory and atomics, which default to false but may be set to true in the current host system.

I'm not 100% sure where the bug is.  It is probably that JS_NewGlobalObject takes a default options object so that the caller is not forced to consider what the correct settings are.  But it could be that - if we can get access to the information at that point - JS_NewGlobalObject should compute a different default, based on the host settings.
(In reply to Lars T Hansen [:lth] from comment #0)
> I'm not 100% sure where the bug is.  It is probably that JS_NewGlobalObject
> takes a default options object so that the caller is not forced to consider
> what the correct settings are.

Yes, this.  If the new global object should have particular options, those should be specified.  Passing nothing means you get what you get, and if that doesn't include SAB, well, the caller should have asked for it.  Making options non-optional sounds entirely reasonable to me, or at a minimum auditing the callers that omit the argument.

(Also note, in passing, that evalcx is a bad API, and newGlobal() can almost always be used -- more clearly -- instead of it.

> But it could be that - if we can get access
> to the information at that point - JS_NewGlobalObject should compute a
> different default, based on the host settings.

http://nooooooooooooooo.com/

JS_NewGlobalObject should be stupid.  It should not depend on system state.  It should do what callers ask.  Callers should control their own destinies.  If the XBL global isn't created with the option set but we think it should be, then we should ensure that option is set in the relevant place.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> > But it could be that - if we can get access
> > to the information at that point - JS_NewGlobalObject should compute a
> > different default, based on the host settings.
> 
> http://nooooooooooooooo.com/
> 

Well okay then :)
See Also: → 1240453
Remove the options default argument and fix the JS engine (mostly test cases).  Also: Enable shared memory at one spot in the shell code that needs it.
Attachment #8708932 - Flags: review?(jwalden+bmo)
The default "options" arguments to JS_NewGlobalObject is being removed.  For now, pass whatever was being passed before, but explicitly.
Attachment #8708934 - Flags: review?(khuey)
The default "options" argument to JS_NewGlobalObject is going away.  This patch corrects test cases by making them pass an empty options argument explicitly; no functional change.
Attachment #8708940 - Flags: review?(sphink)
Comment on attachment 8708932 [details] [diff] [review]
bug1239666-no-default-options-js.patch

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

::: js/src/jsapi-tests/testBug604087.cpp
@@ +65,5 @@
> +                                                         JS::FireOnNewGlobalHook, gOptions));
> +    JS::RootedObject compartment3(cx, JS_NewGlobalObject(cx, getGlobalClass(), nullptr,
> +                                                         JS::FireOnNewGlobalHook, gOptions));
> +    JS::RootedObject compartment4(cx, JS_NewGlobalObject(cx, getGlobalClass(), nullptr,
> +                                                         JS::FireOnNewGlobalHook, gOptions));

While you're changing all three of these, add

  CHECK(compartmentN != nullptr);

after each of them.

::: js/src/jsapi-tests/testMutedErrors.cpp
@@ +49,5 @@
>      for (size_t i = 0; i < len; ++i)
>          chars[i] = asciiChars[i];
>      chars[len] = 0;
>  
> +    JS::CompartmentOptions gOptions;

Just name this |options|, as it's not a global variable.

::: js/src/jsapi-tests/testUbiNode.cpp
@@ +64,5 @@
>      RootedObject global1(cx, JS::CurrentGlobalOrNull(cx));
>      CHECK(global1);
>      CHECK(JS::ubi::Node(global1).zone() == cx->zone());
>  
> +    JS::CompartmentOptions gOptions;

Also not a global, s/gO/o/.

@@ +108,5 @@
>      RootedObject global1(cx, JS::CurrentGlobalOrNull(cx));
>      CHECK(global1);
>      CHECK(JS::ubi::Node(global1).compartment() == cx->compartment());
>  
> +    JS::CompartmentOptions gOptions;

Ditto.
Attachment #8708932 - Flags: review?(jwalden+bmo) → review+
16:47 <lth> waldo, agreed gOptions is not great, but options is already the name of a local var in all those contexts, how about globalOptions as a compromise?
16:47 <Waldo> lth: ugh -- sounds good
16:48 <lth> ok :)
16:48 <Waldo> I suppose technically "compartmentOptions" is better, but I'm not horribly fond of referring to "compartments" much tbh
16:49 <Waldo> pretty sure I've used cO before myself, but eh
Attachment #8708940 - Flags: review?(sphink) → review+
https://hg.mozilla.org/mozilla-central/rev/591c698895a2
https://hg.mozilla.org/mozilla-central/rev/85f60fdd98fa
https://hg.mozilla.org/mozilla-central/rev/cbb0ad1e2118
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.