1996845 - Closing a ChromeWindow sends a "browsing-context-discarded" notification with "replace" as reasoning

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I would like to detect when a top-level browsing context in the parent process gets discarded. Therefore I'm using the browsing-context-discarded observer notification in our code for the Remote Agent (including Marionette). But as I noticed when closing various kinds of Chrome windows the reasoning (as passed via why) mentions replace instead of discard. This makes it not possible for me to really assume that the browsing context is gone.

The related code that is setting replace can be found at:
https://searchfox.org/firefox-main/rev/da0861d6f7f7280518d2a4511d07401aecf50afa/docshell/base/BrowsingContext.cpp#1118-1120

Why do we actually set replace for top-level browsing contexts in the parent process, but not for those living in the content process and which could be replaced by a cross-group navigation - where this information would be helpful.

Nika, could you please help out? Thanks!

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, when triggering a cross-group navigation in a content process we incorrectly leave the why argument as replace for the browsing-context-discarded notification, but have it as replace for browsing-context-attached`.

Maybe we don't know at the time when the browsing context is discarded if it will be replaced? But why do we set it as such for parent process browsing contexts only?

Comment 2

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

Ah that's a good catch. The check for sending "replace" is just a bit incorrect here. It's checking for !Canonical()->GetWebProgress() as for content BrowsingContexts that is an indicator that the BC had ReplacedBy called on it (https://searchfox.org/firefox-main/rev/251eeab1c468eb1557269952c712580e4cc16e29/docshell/base/CanonicalBrowsingContext.cpp#257-258).

It appears that a few months later, a better member to be checking was added (mIsReplaced: https://searchfox.org/firefox-main/rev/251eeab1c468eb1557269952c712580e4cc16e29/docshell/base/CanonicalBrowsingContext.cpp#249-250). If we change the check to that it should probably work better.

    if (XRE_IsParentProcess() && Canonical()->IsReplaced()) {
      why = u"replace";
    }

The XRE_IsParentProcess() check is required as we need to check Canonical() which is only available in the parent process. A content process cannot know whether the BC was replaced (as there's some isolation there), so if you need to know context like that you need to watch these observer notifications in the parent process.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #2)

It appears that a few months later, a better member to be checking was added (mIsReplaced: https://searchfox.org/firefox-main/rev/251eeab1c468eb1557269952c712580e4cc16e29/docshell/base/CanonicalBrowsingContext.cpp#249-250). If we change the check to that it should probably work better.

    if (XRE_IsParentProcess() && Canonical()->IsReplaced()) {
      why = u"replace";
    }

That looks actually pretty good! And it also seems to work fine.

The XRE_IsParentProcess() check is required as we need to check Canonical() which is only available in the parent process. A content process cannot know whether the BC was replaced (as there's some isolation there), so if you need to know context like that you need to watch these observer notifications in the parent process.

Oh, that is great to know. So if the BC is replaced in the content process would it be possible to also include the BC that we are switching to / or from as reference to the attached and discarded observer notifications? That would help us to more easily find out what the new BC will be in case of replacements, when updating our internal mappings to WebDriver BiDi specific UUIDs.

Comment 4

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

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #3)

Oh, that is great to know. So if the BC is replaced in the content process would it be possible to also include the BC that we are switching to / or from as reference to the attached and discarded observer notifications? That would help us to more easily find out what the new BC will be in case of replacements, when updating our internal mappings to WebDriver BiDi specific UUIDs.

We can't include any such information within a content process, no. The BrowsingContext which we're replacing the previous BC with may not be present in the content process at all.

Within the parent process, I don't think we actually actively keep track of the information in the way you are suggesting so that we could easily include it on the specific notifications. You can correlate the two BCs with one being swapped out for the other using the browserID member of the BrowsingContext though.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Nika Layzell [:nika] (ni? for response) from comment #4)

Within the parent process, I don't think we actually actively keep track of the information in the way you are suggesting so that we could easily include it on the specific notifications. You can correlate the two BCs with one being swapped out for the other using the browserID member of the BrowsingContext though.

Ok, good to know but I thought I would have asked if that's possible anyway. We indeed already do exactly the check for the browser but not via the browserId but permanentKey because for unloaded tabs the id becomes 0 for each of those tabs and as such we cannot uniquely identify the related browser element.

Let me put a patch together so that we can fix the reported bug.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1996845 - [dom] Fix the replacement detection for the browsing-context-discarded notification.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/22e501aa0ae6
https://hg.mozilla.org/integration/autoland/rev/c250635da7b9
[dom] Fix the replacement detection for the browsing-context-discarded notification. r=nika

Comment 8

[Cosmin Sabou [:CosminS]]

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