1556975 - BrowsingContext::{GetTop, GetParent} shouldn't check mClosed

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, defect, P2)

Comment 1

[Andreas Farre [:farre]]

Attachment: Bug 1556975 - Don't check mClosed in BrowsingContext::{GetTop, GetParent}. r=nika

Comment 2

[Andreas Farre [:farre]]

Peter had opinions, ni'ing him.

Comment 3

[Peter Van der Beken [:peterv]]

Nika and I talked a bit about this. The check actually seems right, just that mClosed is badly named.

To make sure we're all on the same page, here's how I read the spec. From the algorithm for parent, and following various links:

• Let current be this Window object's browsing context. If current is null, then return null.
• The Window object's browsing context is the Window object's associated Document's browsing context.
• Document's browsing context is the browsing context whose session history contains the Document, if any such browsing context exists and has not been discarded, and null otherwise.

Window.close() discards the browsing context, so does removing an iframe from its parent. Discarding a browsing context also ends up discarding all its child browsing contexts.

So I think mClosed should really be mDiscarded, and used to return null in BrowsingContext::GetParent/GetTop, and BrowsingContext::Close should use a mIsClosing (to implement Window.closed).

Andreas, could you describe what exact issue we were trying to solve with this patch?

Comment 4

[Andreas Farre [:farre]]

browser_devices_get_user_media_in_frame.js starts failing if we detach all of a bc's children in the call to nsDocShell::DestroyChildren. The latter is the fix for Bug 1555287 . And the reason browser_devices_get_user_media_in_frame is failing is because window.top starts returning null (too early?). So I'm all for having a detached state that we can move to, but I don't know how to fix this, what comes down to a timing, issue I guess.

Comment 5

[Nika Layzell [:nika] (ooo, ni? for response)]

(In reply to Andreas Farre [:farre] from comment #4)

browser_devices_get_user_media_in_frame.js starts failing if we detach all of a bc's children in the call to nsDocShell::DestroyChildren. The latter is the fix for Bug 1555287 . And the reason browser_devices_get_user_media_in_frame is failing is because window.top starts returning null (too early?). So I'm all for having a detached state that we can move to, but I don't know how to fix this, what comes down to a timing, issue I guess.

In effect, there are a few different states at play here, and right now we're conflating them into a single boolean, when they're actually more complex than that. We can probably maintain it with 2 synced bools. With the current "racy" field syncing work, I don't think that we should use an enum, as in edge cases it could cause the monotonic state to move backwards.

Namely, the states are:

1. The window is active and alive. mClosed == false && mDiscarded == false.
2. The window has had .close() called on it in a "legal" context. We synchronously set mClosed = true, but leave mDiscarded until the BrowsingContext is actually discarded.
3. The BrowsingContext is actually "discarded", which roughly corresponds to being "detach"-ed under our current model (discarded is the spec term). We set both mClosed = true, and mDiscarded = true, and window.top and window.parent being returning nullptr.

I'm guessing the issue is that, in effect, we were treating the closed state as the same as the discarded state, which was causing some checks to fail.

Comment 6

[Peter Van der Beken [:peterv]]

mClosed should probably reflect the "is closing" state of the spec, and GetTop/GetParent should use the discarded state. So yes, what Nika said :-).

Comment 7

[Andreas Farre [:farre]]

The entire reason for this bug is because browser/base/content/test/webrtc/browser_devices_get_user_media_in_frame.js started failing when trying to fix Bug 1555287 by making sure nsDocShell::DestroyChildren also detaches the BrowsingContext's children. Surprise! nsDocShell::DestroyChildren is not "iterate over all nsDocShell's children and call Destroy", but it's "remove all children from nsDocShell's list of children". This means that we can't call BrowsingContext::Detach for every browsing context child in the nsDocShell's BrosingContext either Solution: just remove the children of the BrowsingContext, and rely on all children in the end calling nsDocShell::Destroy, which in turn calls BrowsingContext::Detach (which now silently accepts not finding a context in its parents list of browsing contexts).

This should probably be better handeld if/when BrowsingContext becomes the driving force between the discard algo. So filed Bug 1558176 for that.