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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

We could then add this flag to one of the configurations we use for jit-tests and jstests to improve our test coverage.

Comment 1

[Jan de Mooij [:jandem]]

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.

Comment 2

[Jan de Mooij [:jandem]]

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?

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Add --same-compartment

Comment 4

[Jan de Mooij [:jandem]]

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

Comment 5

[Jason Orendorff [:jorendorff]]

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!

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1518753 part 1 - Add --more-compartments JS shell flag, make same-compartment the default for newGlobal. r?jorendorff!

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.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1518753 part 2 - Fix some jit-tests to work with same-compartment realms. r?jorendorff!

Depends on D16166

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1518753 part 3 - Fix TypedArrayObject::ensureHasBuffer to create the buffer in the array's realm. r?anba!

Depends on D16167

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1518753 part 4 - Fix IsRegExpPrototype to return false for cross-realm regexp prototypes. r?anba!

Depends on D16168

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1518753 part 5 - Stop using JSProtoKey for initial shapes. r?tcampbell!

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

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Bug 1518753 part 6 - Various fixes for jstests to work with same-compartment realms. r?anba!

Depends on D16170

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1518753 part 7 - Replace newGlobal() => newGlobal({newCompartment: true}) in jit-tests that fail with same-compartment realms. r?jorendorff!

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

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1518753 part 8 - Add --more-compartments to some of the test configurations we use in automation. r?jorendorff!

Depends on D16172

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

[Natalia Csoregi [:nataliaCs]]

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