Closed Bug 1559489 Opened 1 year ago Closed 1 year ago

Transplant remote window proxies back into local window proxies

Categories

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

task

Tracking

()

RESOLVED FIXED
mozilla70
Fission Milestone M4
Tracking Status
firefox70 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files, 1 obsolete file)

I'm splitting this out from bug 1510760. When a page navigates from one origin to another, it is possible that it goes from being in a different process than the things holding a window proxy to being in the same process. In this case, we have to extend the transplanting from bug 1510760 to look in every compartment for a remote window proxy for that page, and transplant it into a normal local window proxy with CCWs, etc.

Blocks: 1561356
Status: NEW → ASSIGNED
Fission Milestone: --- → M4
Priority: -- → P2

Here's a rough WIP patch if anybody wants to try it. It at least passes the transplant test. The names are all really bad.

The idea is that in nsGlobalWindowOuter::SetNewDocument() if there's no existing wrapper then we call a new function RemapRemoteWindowProxies that is sort of like JS_TransplantObject(). It takes a callback object that acts as a partial filter-map on compartments. For this case, it returns the remote window proxy for the bc in the compartment, if any.

js::RemapRemoteWindowProxies() iterates over all compartments and calls the partial filter-map on all of them, gathering up any objects that are returned into a list. This is similar to the beginning of js::RemapAllWrappersForObject(), except that if we get an object for the compartment of the target object, that gets set aside.

After the loop is done, if it found an object in target's compartment, it swaps it, and uses that as the new target. This is okay to do, because like with transplanting we have just created this target, and there should be no references to it. We want to do this before dealing with the other remote window proxies, because those are going to get turned into wrapped versions of the target.

For every one of the other remote window proxies we found, we wrap the target into their compartment, swap them, then do some futzing with the CCW map which is hopefully ok. Finally, I nuke the remote window proxy in its new location, because it seems bad to just have it sitting around.

Other than that, all of the patch really does is change the in-process case of MaybeWrapWindowProxy(). First, I drop the assert that the BC has a window proxy, because we end up in this wrapping code during SetNewDocument before we set the window proxy on the BC. The assert was only there because my initial plan was to fix up remote window proxies during wrapping, and the idea was that you'd get the local window proxy off of the BC. Now remote window proxies are fixed up separately, so this isn't needed. I also changed this code back to a release assert that we don't have a remote window proxy: wrapping code should no longer see a remote window proxy for an in-process BC, because we explicitly fix that up, as I said at least once in this paragraph.

This is just a convenience method.

Also, clean up a formatting issue.

This patch cleans up remote outer window proxies when we navigate back
into the process.

It adds a flag to mDanglingRemoteOuterProxies that is set in between
BrowsingContext::SetDocShell(), where we can tell that the browsing
context is going from being remote to being local, to
nsGlobalWindowOuter::SetNewDocument(), where the local outer window
proxy is actually created. Once the outer window is created, the
remote window proxies can be cleaned up in
CleanUpDanglingRemoteOuterWindowProxies().

The clean up is done by a process that is similar to object
transplanting, except that instead of looking in the cross-compartment
wrapper table for each compartment to find objects to be turned into
CCWs to the new object, it looks in the remote proxy map for each
compartment. SpiderMonkey doesn't know about the proxy maps, so this
has to be done by a new callback object CompartmentObjectCallback.

Now that this cleanup is being done, it shouldn't be possible to wrap
a remote outer window proxy when the browsing context is local, so
MaybeWrapWindowProxy() can be simplified. I had to drop the assert
here that the browsing context has a window proxy because during clean
up we call wrap on a local outer window proxy before the BC gets the
window proxy set on it. I had the assert because my original plan was
to implicitly fix remote proxies during wrapping, but that is no
longer necessary.

Blocks: 1567251

As seen in bug 1568238, I think the handling of waiver wrappers here is not correct. At some point there, we're navigating from local-to-remote, and we hit an assert that the waiver wrapper isn't in the right realm. I think navigating from local to remote doesn't change the realm, so I would surmise that we previously went from remote to local, which does change the realm, and didn't update the waiver table, because my new TransplantObject variant I define in these patches doesn't have the equivalent of xpc::TransplantObject(). However, I think it doesn't really make sense for remote window proxies to have waiver wrappers, so probably we need to explicitly drop them when we navigate from local to remote. I think we don't hit the code in xpc::TransplantObject() that does this, because the remote window proxy is still in the same realm.

In my simple test case, the waiver turned into a dead object proxy, which seems like reasonable behavior, so I'm not sure why it isn't happening in test_mozfiledataurl.html. That test does a lot more navigation, so maybe you need a longer chain to trigger it.

I'll try to confirm if my theory here is accurate.

Depends on: 1570484
Blocks: 1568238
Attachment #9077854 - Attachment is obsolete: true
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/08e2e7007fe1
part 1 - Add IsDOMRemoteProxyObject. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/1cf1849035ab
part 2 - Split out the back half of RemapWrapper into a new method. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/cde59b36166e
part 3 - Add helper method for transplant assert. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/e41c0ed4d437
part 4 - Remote-to-local window transplanting. r=tcampbell,bzbarsky
You need to log in before you can comment on or make changes to this bug.