Make transplanting code turn local window proxies into remote WindowProxy objects
Categories
(Core :: DOM: Bindings (WebIDL), enhancement, P2)
Tracking
()
People
(Reporter: peterv, Assigned: mccr8)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [m4-tests])
Attachments
(5 files, 6 obsolete files)
Updated•6 years ago
|
Reporter | ||
Comment 1•6 years ago
|
||
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.
Updated•6 years ago
|
Updated•6 years ago
|
Updated•6 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
Assignee | ||
Comment 3•5 years ago
|
||
Assignee | ||
Comment 4•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 5•5 years ago
|
||
Here's a rough WIP patch, based on a patch from Peter that I hacked up.
Assignee | ||
Comment 6•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 7•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 8•5 years ago
•
|
||
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.
Assignee | ||
Comment 9•5 years ago
|
||
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.
Assignee | ||
Comment 10•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 12•5 years ago
|
||
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?
Assignee | ||
Comment 13•5 years ago
|
||
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.
Assignee | ||
Comment 14•5 years ago
|
||
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.
Updated•5 years ago
|
Assignee | ||
Comment 15•5 years ago
|
||
Also fix an existing formatting issue.
Assignee | ||
Comment 16•5 years ago
|
||
Assignee | ||
Comment 17•5 years ago
|
||
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.
Assignee | ||
Comment 18•5 years ago
|
||
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.
Updated•5 years ago
|
Comment 19•5 years ago
|
||
Comment 20•5 years ago
|
||
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
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 21•5 years ago
|
||
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•5 years ago
|
||
(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.
Assignee | ||
Comment 23•5 years ago
|
||
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•5 years ago
|
||
Comment 25•5 years ago
|
||
bugherder |
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
Updated•5 years ago
|
Description
•