Closed Bug 1632338 Opened 3 months ago Closed 3 months ago

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

Categories

(Core :: XPConnect, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla77
Fission Milestone M5b
Tracking Status
firefox77 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(1 file)

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.

Fission Milestone: --- → ?

(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.

See Also: → 1632332

Probably doesn't affect non-Fission users.

Do we see this signature in crash reports yet?

Severity: -- → critical
Fission Milestone: ? → M5b
Priority: -- → P1

(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.

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.

(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.

Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1cdf7616482
Allow remote window proxies before SetNewDocument. r=peterv
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
You need to log in before you can comment on or make changes to this bug.