Add a shell flag to make newGlobal reuse compartments by default and fix issues exposed by it
Categories
(Core :: JavaScript Engine, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox66 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(8 files, 1 obsolete file)
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review |
We could then add this flag to one of the configurations we use for jit-tests and jstests to improve our test coverage.
|
Assignee | |
Comment 1•5 years ago
|
||
Doing this for jit-tests is a bit annoying because the debugger tests need separate compartments for debugger and debuggee globals.
Some options are (1) a newGlobal option (2) a jit-test annotation (3) a new function like newGlobalForDebugger.
I'm a bit worried about that becoming a footgun though, when people forget to add it and get backed out.
|
|
Assignee | |
Comment 2•5 years ago
|
||
jstests only has 13 failures so that's doable.
For jit-tests we would have to write a script to fix the failing tests, see comment 1.
Jason, WDYT?
|
|
Assignee | |
Comment 3•5 years ago
|
||
|
|
Assignee | |
Comment 4•5 years ago
|
||
Another option is to do nothing and rely on jsreftests running in the browser getting the same-compartment treatment in the future.
|
||
Comment 5•5 years ago
•
|
||
Yes, mass change. grep finds 45 places in jit-test/tests/debug where we actually pass an argument to newGlobal(); sed or perl can take care of the rest. I can do it if you like. We just need an option that means "new compartment regardless of --same-compartment".
I see there are a handful of old ones that say
var g1 = newGlobal('same-compartment');
String arguments are currently ignored; it would be nice to fix these tests to use the existing sameCompartmentAs: this option. They would probably just start testing something useful again!
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 6•5 years ago
|
||
We want to use this shell flag in automation, but some globals really need their
own compartment so tests can use newGlobal({newCompartment: true}) to opt-out.|
|
Assignee | |
Comment 7•5 years ago
|
||
Depends on D16166
|
|
Assignee | |
Comment 8•5 years ago
|
||
Depends on D16167
|
|
Assignee | |
Comment 9•5 years ago
|
||
Depends on D16168
|
|
Assignee | |
Comment 10•5 years ago
|
||
I added this optimization in bug 1299107 to share more shapes across compartments. Unfortunately this doesn't play well with same-compartment realms (ICs can misbehave) because it relies on compartments being isolated from each other. I think we should remove this optimization: * Fixing the IC issue is impossible without deoptimizing everything. * I added it mainly for chrome globals. The shared-JSM-global work has eliminated the need for this there. * Same-compartment realms win memory back by eliminating CCWs etc. * It's quite a lot of complicated code. Depends on D16169
|
|
Assignee | |
Comment 11•5 years ago
|
||
Depends on D16170
|
|
Assignee | |
Comment 12•5 years ago
|
||
These tests mostly use either the debugger (requires separate compartemnts for debugger/debuggee) or require a new compartment for things like nukeAllCCWs(). Depends on D16171
|
|
Assignee | |
Comment 13•5 years ago
|
||
Depends on D16172
|
|
Assignee | |
Updated•5 years ago
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
|
||
Updated•5 years ago
|
|
||
Comment 14•5 years ago
|
||
Pushed by jdemooij@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/450b8f0cbb4e part 1 - Add --more-compartments JS shell flag, make same-compartment the default for newGlobal. r=jorendorff https://hg.mozilla.org/integration/autoland/rev/92f0cf276198 part 2 - Fix some jit-tests to work with same-compartment realms. r=jorendorff https://hg.mozilla.org/integration/autoland/rev/b32c2548fa6b part 3 - Fix TypedArrayObject::ensureHasBuffer to create the buffer in the array's realm. r=anba https://hg.mozilla.org/integration/autoland/rev/1d49da4facd7 part 4 - Fix IsRegExpPrototype to return false for cross-realm regexp prototypes. r=anba https://hg.mozilla.org/integration/autoland/rev/cfa1c48c7170 part 5 - Stop using JSProtoKey for initial shapes. r=tcampbell https://hg.mozilla.org/integration/autoland/rev/02251eb9e2c1 part 6 - Various fixes for jstests to work with same-compartment realms. r=anba https://hg.mozilla.org/integration/autoland/rev/83c9c1d0af97 part 7 - Replace newGlobal() => newGlobal({newCompartment: true}) in jit-tests that fail with same-compartment realms. r=jorendorff https://hg.mozilla.org/integration/autoland/rev/cdcc178f4896 part 8 - Add --more-compartments to some of the test configurations we use in automation. r=jorendorff
|
||
Comment 15•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/450b8f0cbb4e
https://hg.mozilla.org/mozilla-central/rev/92f0cf276198
https://hg.mozilla.org/mozilla-central/rev/b32c2548fa6b
https://hg.mozilla.org/mozilla-central/rev/1d49da4facd7
https://hg.mozilla.org/mozilla-central/rev/cfa1c48c7170
https://hg.mozilla.org/mozilla-central/rev/02251eb9e2c1
https://hg.mozilla.org/mozilla-central/rev/83c9c1d0af97
https://hg.mozilla.org/mozilla-central/rev/cdcc178f4896
Description
•