1510760 - Make transplanting code turn local window proxies into remote WindowProxy objects

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), enhancement, P2)

Comment 1

[Peter Van der Beken [:peterv]]

When navigating causes a window to change origins and so to switch processes, we'll need to replace a nsOuterWindowProxy with a RemoteOuterWindowProxy or vice versa (keeping the references to the original object working). This involves making xpc::TransplantObject/JS_TransplantObject code deal with the RemoteOuterWindowProxy proxies. As RemoteOuterWindowProxy is not a JS::Wrapper proxy (it doesn't have a JSObject target), we store it in a separate hash on the compartment's private. Either the JS engine will need to learn how to deal with that hash and the RemoteOuterWindowProxy proxies, or we'll need to add a callback that Gecko implements so it can deal with them. The JS engine also has various checks that will need to be modified so that they accept RemoteOuterWindowProxy where they expect a JS::Wrapper now.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Test that window transplants work when navigation moves out of process.

This is the test case Nika gave me for this issue. I've modified it a little bit. First, I store references to the outer window proxies in content and not chrome. I also added a few more debugging messages. I also made the test bail out early, with an explanation in a comment, rather than time out.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: WIP for remote window proxy transplanting.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: WIP for remote window proxy transplanting.

Comment 5

[Andrew McCreight [:mccr8]]

Here's a rough WIP patch, based on a patch from Peter that I hacked up.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: WIP for remote window proxy transplanting.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: WIP for remote window proxy transplanting.

Comment 8

[Andrew McCreight [:mccr8]]

I've cleaned up the patch a little bit and rebased it. Nika figured out another issue I was having, and I've put up a patch for it in bug 1557808. With that, browser_windowProxy_transplant.js passes. However, there's a GC engine assertion in shutdown. Specifically, during shutdown ~nsGlobalWindowOuter gets called, and it calls SetProxyReservedSlot() on GetWrapperMaybeDead(), which hits an assert that seems to indicate that the proxy is dead. I'll dig into what exactly is going wrong.

Comment 9

[Andrew McCreight [:mccr8]]

I'm pretty sure the issue with the UAF is that we're changing the class of the JS object during transplanting, so the original outer window proxy finalize hook never gets called. This can be fixed by calling the finalize hook if we're changing the class, hopefully.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: WIP for remote window proxy transplanting.

This passes the test for going from in process to out-of-process now. I need to clean up the code, and figure out if I can split out any code only needed for out-of-process to in-process, because that's not being tested now. If I can, I'll file a new bug depending on this one.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: Bug 1510760, part 5 - Support local-to-remote window proxy transplanting.

When a BrowsingContext changes from being local to remote, we have to
change all window proxies from being local to remote, using
transplanting. The actual window proxy becomes a remote window
proxy. Cross compartment wrappers (CCWs) to the window proxy also
become remote window proxies in their respective compartments, rather
than CCWs to a remote proxy in the old compartment of the window
proxy, because the window is no longer actually in that
compartment. This also avoids having to figure out what Xray behavior
for remote window proxies should be.

This patch adds a remapSwap callback that is called whenever
transplanting is going to swap two objects. Gecko uses this to update
the remote proxy map, if the object being swapped is a remote object
proxy.

It drops the requirement that both arguments to JS_TransplantObject
have the same class, because we need to transplant a window proxy with
a remote window proxy. It also deals with this by not adding origobj
to the wrapper map unless it is a CCW, to handle transplanting to a
remote proxy.

The core design here, with the remote window proxies in every
compartment and the use of the remapSwap callback, is taken from a
patch by peterv.

Comment 12

[Andrew McCreight [:mccr8]]

I put together a patch that does not require the remap callback. Instead, it threads the "existing" value from transplanting all the way through into the prewrap callback (null gets passed in for the non-transplant case). Then I can use that as the value stored in the compartment remote window proxy map.

However, when I do that, I hit my assert for detecting a wrap of a remote proxy for a now-local BC. That is actually what I would have expected the behavior to be, so it raises some issues:

• How did the new version of my patch change behavior at all? It really shouldn't.
• Why did my original patch (still attached to this bug) not hit the assert?
• I really should not fatally assert in this case, because it seems like a decent number of people are dogfooding Fission now, and I don't want them to crash. But what should I do instead? Maybe I turn them into a dead wrapper proxy? Or maybe I shouldn't land the local-to-remote case without also fixing the remote-to-local case?

Comment 13

[Andrew McCreight [:mccr8]]

I worked out why my original patch wasn't asserting. Ted had pointed out to me that my patch was failing to call remapSwap() before every call to JSObject::swap(). It turns out that if I move remapSwap() inside JSObject::swap() then I also hit the assert in the patch here. In my new patch, the equivalent of the remapSwap() stuff is done inside the prewrap hook, so it doesn't miss any cases. Therefore, I think that was just a bug in my original patch. I still need to decide what to do instead of the release assert.

Comment 14

[Andrew McCreight [:mccr8]]

There's a lot more mindless piping around of values in the new approach, so I have split the existing patch into a number of smaller patches. I'll indicate on each patch whether it represents any substantial changes from the prior patch.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: Bug 1510760, part 1 - Make GetBrowsingContext and GetOuterWindow callable elsewhere.

Also fix an existing formatting issue.

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: Bug 1510760, part 2 - Add the remote proxy handler to SetDOMProxyInformation.

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: Bug 1510760, part 3 - Thread the transplant object into the prewrap hook.

In a later patch, the prewrap hook will need to know the address of
the object we are eventually going to transplant into. In the
non-transplant case, the value will be null.

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: Bug 1510760, part 4 - Add transplant support to GetRemoteOuterWindowProxy().

When we call GetRemoteOuterWindowProxy in the middle of a transplant,
the remote proxy that the function returns will be almost immediately
swapped with some other object. Rather than trying to fix up the
remote proxy map when that happens, this patch adds a new argument
that is a pointer to the object, if any, that the remote proxy is
going to be swapped to. This will be used in the remote proxy map.

Having a value in the remote proxy map that is not a remote proxy
could cause issues if somebody ends up calling
GetRemoteOuterWindowProxy() a second time before the transplant has
finished. To avoid that, my patch asserts that we are returning an
object with the appropriate class.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75b769608cce
part 1 - Make GetBrowsingContext and GetOuterWindow callable elsewhere. r=peterv
https://hg.mozilla.org/integration/autoland/rev/cb12d4068aca
part 2 - Add the remote proxy handler to SetDOMProxyInformation. r=peterv,tcampbell
https://hg.mozilla.org/integration/autoland/rev/3bc5442338bc
part 3 - Thread the transplant object into the prewrap hook. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/524ac8b3040d
part 4 - Add transplant support to GetRemoteOuterWindowProxy(). r=peterv
https://hg.mozilla.org/integration/autoland/rev/19a972ea7855
part 5 - Support local-to-remote window proxy transplanting. r=tcampbell,peterv

Comment 20

[Noemi Erli[:noemi_erli]]

Backed out 5 changesets (Bug 1510760) for bustages in nsGlobalWindowOuter.cpp

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=257178296&resultStatus=testfailed%2Cbusted%2Cexception&revision=19a972ea785504b027654fb743097ed52b677fad

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=257178296&repo=autoland&lineNumber=48806

Backout: https://hg.mozilla.org/integration/autoland/rev/c024f802807e6e8e6de4fff6fbc3a566e92e8310

Comment 21

[Andrew McCreight [:mccr8]]

There are two failures. First, there's a rooting hazard, which I should be able to easily fix by rooting earlier. The second one is a TV failure. It looks like the transplant test crashes in chaos mode. I suppose I should figure out what is going on there.

Comment 22

[Kris Maglione [:kmag]]

(In reply to Andrew McCreight [:mccr8] from comment #21)

The second one is a TV failure. It looks like the transplant test crashes in chaos mode. I suppose I should figure out what is going on there.

Follow-up please :) Tests are allowed to crash in chaos mode.

Comment 23

[Andrew McCreight [:mccr8]]

Alright. It looks like it is hitting a release assert:
MOZ_DIAGNOSTIC_ASSERT(false, "Trying to cache out of process context");
in mozilla::dom::ContentParent::RecvCacheBrowsingContextChildren()

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a0fbc4cb9a2
part 1 - Make GetBrowsingContext and GetOuterWindow callable elsewhere. r=peterv
https://hg.mozilla.org/integration/autoland/rev/b3461fd37c34
part 2 - Add the remote proxy handler to SetDOMProxyInformation. r=peterv,tcampbell
https://hg.mozilla.org/integration/autoland/rev/be69ee8881f9
part 3 - Thread the transplant object into the prewrap hook. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/73b5cb06eb32
part 4 - Add transplant support to GetRemoteOuterWindowProxy(). r=peterv
https://hg.mozilla.org/integration/autoland/rev/44e094d2d705
part 5 - Support local-to-remote window proxy transplanting. r=tcampbell,peterv

Comment 25

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/0a0fbc4cb9a2
https://hg.mozilla.org/mozilla-central/rev/b3461fd37c34
https://hg.mozilla.org/mozilla-central/rev/be69ee8881f9
https://hg.mozilla.org/mozilla-central/rev/73b5cb06eb32
https://hg.mozilla.org/mozilla-central/rev/44e094d2d705