1512260 - Figure out what to do with wrapper nuking and same-compartment realms

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

Spinning this off from bug 1512029.

Some thoughts:

* Cu.nukeSandbox(sb) is used to nuke wrappers from/to a sandbox. If we use a single compartment for all system realms (bug 1512029), we lose the ability to cut references to sandbox objects (without nuking the whole system compartment).

Maybe there could be a supportsNuking sandbox option that forces the sandbox to be in its own compartment. Then in Cu.nukeSandbox we throw an exception if that option isn't set on the sandbox.

* From kmag (comment 14): "At some point, we're going to need to support putting multiple content script sandboxes in the same compartment (mainly so they share the same X-ray expandos), and we're definitely going to want to support nuking those sandboxes."

* In general, nuking should probably operate on a target realm instead of target compartment. This works well for cutting chrome => content wrappers because there we only care about incoming references. The NukeAllReferences behavior (used for nuking sandboxes and add-on windows) becomes more complicated, though: we could say we only cut wrappers *from* the target compartment if we're nuking the last realm in it.

Comment 1

[Jan de Mooij [:jandem]]

NI to have some feedback on my ramblings in comment 0.

Comment 2

[Jan de Mooij [:jandem]]

I forgot to mention this explicitly: if we put system sandboxes in the main system compartment and we change nuking to be per-target-realm, nukeSandbox doesn't really make sense because other system realms in the same compartment could have *non-wrapper* references to things belonging to the sandbox (the whole point of same-compartment realms). So I think from this it follows that we really want to have a separate compartment per (nukable) sandbox (or multiple sandboxes).

Comment 3

[Kris Maglione [:kmag]]

(In reply to Jan de Mooij [:jandem] from comment #0)
> Maybe there could be a supportsNuking sandbox option that forces the sandbox
> to be in its own compartment. Then in Cu.nukeSandbox we throw an exception
> if that option isn't set on the sandbox.

I think we're still going to need shared-compartment sandboxes that we can nuke, becaues:

> * From kmag (comment 14): "At some point, we're going to need to support
> putting multiple content script sandboxes in the same compartment (mainly so
> they share the same X-ray expandos), and we're definitely going to want to
> support nuking those sandboxes."

> * In general, nuking should probably operate on a target realm instead of
> target compartment. This works well for cutting chrome => content wrappers
> because there we only care about incoming references. The NukeAllReferences
> behavior (used for nuking sandboxes and add-on windows) becomes more
> complicated, though: we could say we only cut wrappers *from* the target
> compartment if we're nuking the last realm in it.

I agree, I think this is probably a good plan.

(In reply to Jan de Mooij [:jandem] from comment #2)
> I forgot to mention this explicitly: if we put system sandboxes in the main
> system compartment and we change nuking to be per-target-realm, nukeSandbox
> doesn't really make sense because other system realms in the same
> compartment could have *non-wrapper* references to things belonging to the
> sandbox (the whole point of same-compartment realms). So I think from this
> it follows that we really want to have a separate compartment per (nukable)
> sandbox (or multiple sandboxes).

I think we can probably do without the ability to nuke system principal sandboxes at all, at this point. But keeping the option and requiring that nukable system sandboxes go in a separate compartment is probably OK too...

Comment 4

[Jan de Mooij [:jandem]]

Kris: thanks, I appreciate your help with this.

I'll keep the NIs just to make sure we're all on the same page with this.

Comment 5

[Jan de Mooij [:jandem]]

This adds a canBeNuked sandbox option and throws if we try to nuke a sandbox without that option: https://hg.mozilla.org/try/rev/ee2f397230c62aa1db4a4fd8d40db98fd77a4557

To make CCW nuking work on a target realm instead of compartment: https://hg.mozilla.org/try/rev/1fbc6a27c4335eb7da455156ec42efb0de2bab0f

Needs more cleanup but it seems to work well.

Comment 6

[Bobby Holley (:bholley)]

I'm a bit confused about the plan of record. Can you summarize?

Comment 7

[Bobby Holley (:bholley)]

In particular: with CPO, we can still support nuking references _to_ a global, but can only nuke references _from_ a compartment. What are the use-cases we still care about in this space, and what are the proposed semantics of the new setup?

Comment 8

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (:bholley) from comment #6)
> I'm a bit confused about the plan of record. Can you summarize?

Plan is to:

(1) Nuke references from a compartment to a *realm*. When we nuke references *from* a compartment (the NukeAllReferences option), we will cut outgoing wrappers when we nuke the last realm in the compartment.

(2) Cu.nukeSandbox will require the sandbox to be created with a canBeNuked/nukable option that forces the sandbox in its own compartment (to ensure there are no non-wrapper references to it, to make nuking more effective).

CCW nuking is being used in the following cases:

(a) Cutting chrome => content wrappers [0]. This one will work exactly like it does now because we only cut *incoming* references there, and chrome/content will never share a compartment. This is the most important one I expect.

(b) Add-on compartment nuking [1] and Cu.nukeSandbox [2]. These use the NukeAllReferences option. Per (1) above, here the new behavior will be that we only cut wrappers *from* the compartment if we're nuking the last realm in it. (2) will ensure we have a single realm per compartment for now in the sandbox case, so that should ensure no change in behavior AFAICS.

[0] https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/dom/base/WindowDestroyedEvent.cpp#124-129
[1] https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/dom/base/WindowDestroyedEvent.cpp#119
[2] https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/js/xpconnect/src/XPCComponents.cpp#2080

Comment 9

[Bobby Holley (:bholley)]

This generally seems sane.

Per IRC discussion, I'm not totally sold that we need the canBeNuked thing. The nukeSandbox consumers we looked at (IDP and Normandy) seem mostly to be just trying to clean up after themselves more aggressively, and I'm not sure they're going to want to opt-in to more overhead (CCWs and whatnot) to keep doing that.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Jan de Mooij [:jandem] from comment #8)
> Per (1) above, here the new behavior will be that
> we only cut wrappers *from* the compartment if we're nuking the last realm
> in it.

Also, how do we plan to track this? Is it a liveness question, or are we going to be tracking what was nuked? I think we probably want the latter, otherwise you could have two sandboxes holding each other alive even though both have been nuked.

Comment 11

[Bobby Holley (:bholley)]

(And note that we already have the information for this: https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/js/xpconnect/src/xpcprivate.h#2812 )

Comment 12

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (:bholley) from comment #9)
> The nukeSandbox consumers we looked at (IDP and Normandy) seem mostly to be
> just trying to clean up after themselves more aggressively, and I'm not sure
> they're going to want to opt-in to more overhead (CCWs and whatnot) to keep
> doing that.

Yeah, I think we definitely want to avoid that as much as possible for system scopes. We definitely need it for extension sandboxes, since we can't trust them to behave sanely, but I'd want a very strong argument if we were thinking about doing it for system scopes.

Might be worth keeping in debug builds as a smoke test, though...

Comment 13

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (:bholley) from comment #10)
> Also, how do we plan to track this? Is it a liveness question, or are we
> going to be tracking what was nuked?

The latter, I think. Once all realms in a compartment have been nuked, we should nuke the whole compartment.

Comment 14

[Jan de Mooij [:jandem]]

OK per IRC discussion I'll move wasNuked to JS::Realm from CompartmentPrivate/RealmPrivate. Then we can just check it in js::NukeCrossCompartmentWrappers.

Comment 15

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #12)
> (In reply to Bobby Holley (:bholley) from comment #9)
> > The nukeSandbox consumers we looked at (IDP and Normandy) seem mostly to be
> > just trying to clean up after themselves more aggressively, and I'm not sure
> > they're going to want to opt-in to more overhead (CCWs and whatnot) to keep
> > doing that.
> 
> Yeah, I think we definitely want to avoid that as much as possible for
> system scopes. We definitely need it for extension sandboxes, since we can't
> trust them to behave sanely, but I'd want a very strong argument if we were
> thinking about doing it for system scopes.

Neither of those two cases are system scopes in the general case. So I think we could just give them the same behavior we're talking about applying everywhere ("incoming references get nuked, outgoing references get nuked if you're the last non-nuked global in your compartment").

Comment 16

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (:bholley) from comment #11)
> (And note that we already have the information for this:
> https://searchfox.org/mozilla-central/rev/
> fd62b95c187a40b328d9e7fd9d848833a6942b57/js/xpconnect/src/xpcprivate.h#2812 )

I think we'll need to move this to the realm private, or possibly keep separate flags on both privates. For nuked realms, we're going to want to prohibit creating wrappers for objects in that realm. And for compartments where all realms have been nuked, we're going to want to prevent creating wrappers out of that compartment. I don't think we're going to want to iterate over all the realms every time we do that check.

Comment 17

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (:bholley) from comment #15) 
> Neither of those two cases are system scopes in the general case. So I think
> we could just give them the same behavior we're talking about applying
> everywhere ("incoming references get nuked, outgoing references get nuked if
> you're the last non-nuked global in your compartment").

Oh, yeah, I'm not even sure there'll be any extra overhead in that case unless there are multiple sandboxes with the same principal that communicate with each other.

Comment 18

[Jan de Mooij [:jandem]]

Current plan is:

(1) Move wasNuked flag from XPConnect to JS.

(2) Change nuking to be per target realm. If we're nuking *outgoing* wrappers, we do this when all realms have the wasNuked flag set. We prevent creating new wrappers when cx->realm or obj->realm has wasNuked set (this check will move from XPConnect into SpiderMonkey).

(3) If we get xpcshell test failures in the nuke-sandbox tests, fix them by using a null principal instead of the system principal for sandboxes.

Comment 19

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #18)
> We prevent
> creating new wrappers when cx->realm or obj->realm has wasNuked set (this
> check will move from XPConnect into SpiderMonkey).

Maybe it would be cleaner to have a compartment flag for this as well (as suggested by kmag in comment 16), and check that instead of cx->realm->wasNuked. That way, if you manage to create a new realm in a *nuked compartment*, we still block it from creating new wrappers which seems nice.

Also clearing the NI from Boris: I think I have enough information, but of course feel free to comment.

Comment 20

[Jan de Mooij [:jandem]]

Attachment: Bug 1512260 - Make wrapper nuking work with a target realm instead of target compartment. r?kmag

For *incoming* wrappers this preserves behavior. We nuke *outgoing* wrappers
when all realms in the compartment have been nuked. To implement this I moved
the wasNuked flag from XPConnect to JS::Compartment as nukedOutgoingWrappers and
to JS::Realm as nukedIncomingWrappers.

The code to create a dead wrapper in the nuked compartment/realm case was also
moved into the JS engine. I added a shell test for it.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8dd01db9f92
Make wrapper nuking work with a target realm instead of target compartment. r=kmag

Comment 22

[Andrei Ciure[:aciure]]

Backed out changeset a8dd01db9f92 (bug 1512260) requsted by owner (missing test)

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=216547803&revision=a8dd01db9f92757385f96a322296c4b027b7e72c

backout: https://hg.mozilla.org/integration/autoland/rev/c322f02577d386a90b72d29bf98d99256de13563

Comment 23

[Jan de Mooij [:jandem]]

I'm not sure why but Mercurial lost the jit-test file I added when I rebased this patch on top of a parent revision. I'll try this again later.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddb7c9f71ce2
Make wrapper nuking work with a target realm instead of target compartment. r=kmag

Comment 25

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #23)
> I'm not sure why but Mercurial lost the jit-test file I added when I rebased
> this patch on top of a parent revision.

I think it's a bug in the evolve extension (I used the `hg grab` command). I can repro with a toy repo and I'll report it to the Mercurial folks today.

Comment 26

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/ddb7c9f71ce2