Closed Bug 1518753 Opened 1 year ago Closed 1 year ago

Add a shell flag to make newGlobal reuse compartments by default and fix issues exposed by it

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla66
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.

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.

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?

Flags: needinfo?(jorendorff)
Attached patch Add --same-compartment (obsolete) — Splinter Review

Another option is to do nothing and rely on jsreftests running in the browser getting the same-compartment treatment in the future.

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!

Flags: needinfo?(jorendorff)
Summary: Add a shell flag to change newGlobal to reuse compartments by default → Add a shell flag to make newGlobal reuse compartments by default and fix issues exposed by it
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.
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
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
Attachment #9035271 - Attachment is obsolete: true
Blocks: 1519414
Attachment #9035572 - Attachment description: Bug 1518753 part 1 - Add --same-compartment JS shell flag and a newGlobal option to opt-out of this. r?jorendorff! → Bug 1518753 part 1 - Add --more-compartments JS shell flag, make same-compartment the default for newGlobal. r?jorendorff!
Attachment #9035573 - Attachment description: Bug 1518753 part 2 - Fix some jit-tests to work with --same-compartment. r?jorendorff! → Bug 1518753 part 2 - Fix some jit-tests to work with same-compartment realms. r?jorendorff!
Attachment #9035577 - Attachment description: Bug 1518753 part 6 - Various fixes for jstests to pass with --same-compartment. r?anba! → Bug 1518753 part 6 - Various fixes for jstests to work with same-compartment realms. r?anba!
Attachment #9035578 - Attachment description: Bug 1518753 part 7 - Replace newGlobal() => newGlobal({newCompartment: true}) in jit-tests that fail with --same-compartment. r?jorendorff! → Bug 1518753 part 7 - Replace newGlobal() => newGlobal({newCompartment: true}) in jit-tests that fail with same-compartment realms. r?jorendorff!
Attachment #9035579 - Attachment description: Bug 1518753 part 8 - Add --same-compartment to some of the test configurations we use in automation. r?jorendorff! → Bug 1518753 part 8 - Add --more-compartments to some of the test configurations we use in automation. r?jorendorff!
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
Depends on: 1520093
Depends on: 1520536
You need to log in before you can comment on or make changes to this bug.