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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Lars T Hansen [:lth]]

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.

Comment 1

[Lars T Hansen [:lth]]

That might or might not also be responsible for failures like these on Nightly:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45c957c765c0&selectedJob=15446330&filter-searchStr=Linux%20x64%20debug%20Mochitest%20e10s%20Mochitest%20e10s%20M-e10s%286%29

Comment 2

[Jeff Walden [:Waldo]]

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

Comment 3

[Lars T Hansen [:lth]]

(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 :)

Comment 4

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=686b49955c14

Comment 5

[Lars T Hansen [:lth]]

Attachment: bug1239666-no-default-options-js.patch

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.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1239666-no-default-options-dom.patch

The default "options" arguments to JS_NewGlobalObject is being removed.  For now, pass whatever was being passed before, but explicitly.

Comment 7

[Lars T Hansen [:lth]]

Attachment: bug1239666-no-default-options-devtools.patch

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.

Comment 8

[Jeff Walden [:Waldo]]

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.

Comment 9

[Lars T Hansen [:lth]]

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

Comment 10

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/591c698895a24dee2387b5b8785e4a968941addf
Bug 1239666 - part 1, get rid of the default parameter.  r=waldo

https://hg.mozilla.org/integration/mozilla-inbound/rev/85f60fdd98facbcc80fa7edb22eca55ff534d68b
Bug 1239666 - part 2, dom/indexedDB change.  r=khuey

https://hg.mozilla.org/integration/mozilla-inbound/rev/cbb0ad1e211825863b355d0b9ace98b373268bb3
Bug 1239666 - part 3, devtools test case changes.  r=sphink

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/591c698895a2
https://hg.mozilla.org/mozilla-central/rev/85f60fdd98fa
https://hg.mozilla.org/mozilla-central/rev/cbb0ad1e2118