1632338 - Figure out what to do about the gap between making a BrowsingContext local and fixing up any remote window proxies

Status: RESOLVED FIXED

(Core :: XPConnect, task, P1)

Description

[Andrew McCreight [:mccr8]]

The intermittent failures in bug 1608600 and bug 1625031 are (hopefully) about a poorly written test, but they also revealed a problem with the way I implemented cleaning up dangling remote window proxies.

When we change a BC from remote to local in BrowsingContext::SetDocShell(), we set mDanglingRemoteOuterProxies to true. Then later in nsGlobalWindowOuter::SetNewDocument(), if that flag is true, we use transplanting to turn all of the old remote window proxies into a local window proxy (or CCWs to that window proxy).

The problem is that if we try to wrap one of the old remote window proxies in between those two steps, then we hit a release assert in MaybeWrapWindowProxy(), because it seems bad that we're returning a remote window proxy for a local BC.

Now, maybe we shouldn't actually assert in that case, because that remote window proxy will eventually get turned into a proper local window proxy. Arguably, the real problem would happen if somebody tries to use the remote window proxy where the bc is local, but maybe we have other checks for that?

I'm not sure how much of a problem in practice this is. As this is a release assert, it should show up as crash reports where the crash reason is MOZ_RELEASE_ASSERT(isWindowProxy), but I don't see any. But maybe my searching is just bad.

This should also get a test.

Comment 1

[Andrew McCreight [:mccr8]]

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

Arguably, the real problem would happen if somebody tries to use the remote window proxy where the bc is local, but maybe we have other checks for that?

Also, if this is a problem, it is a problem even if we aren't trying to wrap existing remote window proxies.

Comment 2

[Chris Peterson [:cpeterson]]

Probably doesn't affect non-Fission users.

Do we see this signature in crash reports yet?

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Chris Peterson [:cpeterson] from comment #2)

Probably doesn't affect non-Fission users.
We only create remote window proxies for Fission windows, so it shouldn't affect non-Fission users.

Do we see this signature in crash reports yet?

I looked for the release assert in crash stats, and didn't find it. This is probably just an overly-strong assert, so it should be easy to fix.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1632338 - Allow remote window proxies before SetNewDocument.

In BrowsingContext::SetDocShell(), we indicate that any remote outer window
proxies need to be cleaned up, if we've transitioned from a remote window
proxy to a local one. However, we don't actually do the cleanup until
nsGlobalWindowOuter::SetNewDocument(), so don't assert if we find remote
window proxies when we're in between these two periods.

Also includes a formatting fix by clang-format.

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Chris Peterson [:cpeterson] from comment #2)

Probably doesn't affect non-Fission users.

We only create remote window proxies with Fission, so it shouldn't affect non-Fission users.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1cdf7616482
Allow remote window proxies before SetNewDocument. r=peterv

Comment 7

[Stefan Hindli [:stefan_hindli]]

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