Closed
Bug 1239666
Opened 8 years ago
Closed 8 years ago
JS_NewGlobalObject should have the "right" defaults for shared memory even with default compartment options
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: lth, Assigned: lth)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
14.21 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
923 bytes,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
2.77 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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•8 years ago
|
||
(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.
Assignee | ||
Comment 3•8 years ago
|
||
(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 :)
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=686b49955c14
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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)
Assignee | ||
Comment 7•8 years ago
|
||
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 8•8 years ago
|
||
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+
Assignee | ||
Comment 9•8 years ago
|
||
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
Updated•8 years ago
|
Attachment #8708940 -
Flags: review?(sphink) → review+
Attachment #8708934 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 10•8 years ago
|
||
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•8 years ago
|
||
bugherder |
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: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•