1318767 - Support navigating into PartialSHistories which have dead frameloaders

Status: RESOLVED FIXED

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

Description

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

We need to support the case when a GroupedSHistory needs to navigate to a history index which is handled by a PartialSHistory which is holding a dead frameloader. This frameloader could be dead for many reasons (perhaps the tab crashed, perhaps we removed it to clear out its bfcache entries, etc.)

My current plan is to handle this by using SessionStore to restore the full history of the GroupedSHistory into the currently active DocShell, and then reverting from a GroupedSHistory state into a normal complete SHistory state. This will also involve removing all of the hidden frameloaders which are used for bfcache purposes.

Comment 1

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

Attachment: Part 1: When performing SessionRestore on a GroupedSHistory tab, ensure that it reverts to a complete SHistory state before restoring

MozReview-Commit-ID: 2zNXDtkx3bs

Comment 2

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

Attachment: Part 2: Correctly swap web progress listeners when swapping frameloaders

MozReview-Commit-ID: KbUd2Os5qac

Comment 3

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

Attachment: Part 3: Use SessionStore to recover from missing GroupedSHistory segments

MozReview-Commit-ID: FDmQIjiHox7

Comment 4

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

Attachment: Part 4: Add a test for navigating into dead PartialSHistories

MozReview-Commit-ID: LT8NeCdf3fm

Comment 5

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

Comment on attachment 8813825 [details] [diff] [review]
Part 2: Correctly swap web progress listeners when swapping frameloaders

Clearing review because I have discovered from testing that devtools calls _swapBrowserDocShells from within devtools passing something which doesn't appear to be a tab as aOtherBrowser for responsive design mode. I may need to rearchitect this patch a bit to keep that use case "working" (although I expect it doesn't hook up listeners correctly).

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Michael Layzell [:mystor] [:mrl] from comment #5)
> Comment on attachment 8813825 [details] [diff] [review]
> Part 2: Correctly swap web progress listeners when swapping frameloaders
> 
> Clearing review because I have discovered from testing that devtools calls
> _swapBrowserDocShells from within devtools passing something which doesn't
> appear to be a tab as aOtherBrowser for responsive design mode. I may need
> to rearchitect this patch a bit to keep that use case "working" (although I
> expect it doesn't hook up listeners correctly).

Please reach out to me if you need any help with this, I worked on this DevTools code.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8813824 [details] [diff] [review]
Part 1: When performing SessionRestore on a GroupedSHistory tab, ensure that it reverts to a complete SHistory state before restoring

Review of attachment 8813824 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is good - I don't see an issue with the proposed changes. I do have a question below, so postponing r+ for a short while.

::: browser/base/content/tabbrowser.xml
@@ +1690,5 @@
>                  throw new Exception("Cannot change opener on an already non-remote browser!");
>                }
>              }
>  
> +            // Abort if we can avoid recreating the frameloader unnecessarially. 

nit: -a and trailing whitespace.

@@ +1691,5 @@
>                }
>              }
>  
> +            // Abort if we can avoid recreating the frameloader unnecessarially. 
> +            if (!aOptions.newFrameLoader && isRemote == aShouldBeRemote) {

Shouldn't you keep `&& !aOptions.freshProcess` in here, just to be safe?

@@ +1823,5 @@
>                return this.updateBrowserRemoteness(aBrowser, false);
>              }
>  
>              return this.updateBrowserRemoteness(aBrowser,
>                                                  /* aShouldBeRemote */ true,

Can you remove this comment here? The arg's purpose is obviously deducible from the method name, now that the others are not interfering anymore (good choice!)

::: browser/components/sessionstore/SessionStore.jsm
@@ +3548,5 @@
>        isRemotenessUpdate = tabbrowser.switchBrowserIntoFreshProcess(browser);
>      } else {
> +      let newFrameloader = !!browser.frameLoader.groupedSessionHistory;
> +      isRemotenessUpdate = tabbrowser.updateBrowserRemotenessByURL(browser, uri,
> +                                                                   { newFrameloader });

What will the net-effect be when you request a new frameloader? With a groupedSessionHistory, you want a new frameloader - but will that do?

Comment 8

[:Gijs (out for now; he/him)]

(potential conflict warning: bug 1147911)

Comment 9

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

(In reply to :Gijs Kruitbosch from comment #8)
> (potential conflict warning: bug 1147911)

Thanks for the pointer. That patch is definitely going to conflict with this one. I will probably move the new added parameter into aOptions as well.

Comment 10

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

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> What will the net-effect be when you request a new frameloader? With a
> groupedSessionHistory, you want a new frameloader - but will that do?

A frameloader keeps the docshell or tabparent/child alive, which keeps the sessionhistory alive. Replacing the frameloader is sorta like getting a fresh start - the docshell will be brand new, and empty, with no session history (or partialSHistory) in the way of the sessionrestore.

Comment 11

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

Attachment: Part 2: Correctly swap web progress listeners when swapping frameloaders

I changed the code to only change the codepath which I needed for this patch, which should keep it working with the docshell case (the tests seem to pass localy).

MozReview-Commit-ID: KbUd2Os5qac

Comment 12

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

Attachment: Part 1: When performing SessionRestore on a GroupedSHistory tab, ensure that it reverts to a complete SHistory state before restoring

Updated to apply on the new mozilla-inbound (which has the patch mentioned earlier which also changes updateBrowserRemoteness). This should also fix your review comments.

MozReview-Commit-ID: 2zNXDtkx3bs

Comment 13

[(no longer active)]

Comment on attachment 8813826 [details] [diff] [review]
Part 3: Use SessionStore to recover from missing GroupedSHistory segments

Review of attachment 8813826 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great, thanks!

::: dom/base/GroupedSHistory.cpp
@@ +125,5 @@
> +      // Check if we are trying to swap to a dead frameloader, and return
> +      // NS_ERROR_DOM_INVALID_STATE_ERR if we are.
> +      nsCOMPtr<nsIFrameLoader> frameLoader;
> +      partialHistory->GetOwnerFrameLoader(getter_AddRefs(frameLoader));
> +      if (frameLoader->GetIsDead()) {

Please null-check frameLoader here.

@@ +126,5 @@
> +      // NS_ERROR_DOM_INVALID_STATE_ERR if we are.
> +      nsCOMPtr<nsIFrameLoader> frameLoader;
> +      partialHistory->GetOwnerFrameLoader(getter_AddRefs(frameLoader));
> +      if (frameLoader->GetIsDead()) {
> +        // XXX: Is this an appropriate error to use here?

Not really... this error code is used for the Web-facing InvalidStateErr.  You should probably use something internal.  Maybe NS_ERROR_NOT_AVAILABLE?

::: toolkit/content/widgets/browser.xml
@@ +392,5 @@
>            ]]>
>          </body>
>        </method>
>  
> +      <field name="_sessionStore">null</field>

Can you please ask someone else closer to Firefox to look over these changes too?  Maybe Mike?

Comment 14

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

Attachment: Part 3: Use SessionStore to recover from missing GroupedSHistory segments, r=ehsan

MozReview-Commit-ID: FDmQIjiHox7

Comment 15

[Mike de Boer [:mikedeboer]]

Comment on attachment 8814203 [details] [diff] [review]
Part 1: When performing SessionRestore on a GroupedSHistory tab, ensure that it reverts to a complete SHistory state before restoring

Review of attachment 8814203 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the explanation, Michael!

Comment 16

[:Gijs (out for now; he/him)]

Comment on attachment 8814188 [details] [diff] [review]
Part 2: Correctly swap web progress listeners when swapping frameloaders

Review of attachment 8814188 [details] [diff] [review]:
-----------------------------------------------------------------

This looks sane enough - some simplification suggestions below. Can you get the next review from dao?

::: browser/base/content/tabbrowser.xml
@@ +2926,4 @@
>          <parameter name="aFlags"/>
>          <body>
>            <![CDATA[
> +            let remoteBrowser = aOtherTab.linkedBrowser.ownerDocument.defaultView.gBrowser;

Nit: I would call this `otherTabBrowser`, like in the browser.xml patch, to distinguish gBrowser/tabbrowser from the <browser> linked to the remote/other tab. "remote" has become pretty confusing terminology-wise because of e10s.

@@ +2926,5 @@
>          <parameter name="aFlags"/>
>          <body>
>            <![CDATA[
> +            let remoteBrowser = aOtherTab.linkedBrowser.ownerDocument.defaultView.gBrowser;
> +            let otherBrowser = remoteBrowser.getBrowserForTab(aOtherTab);

This should just be aOtherTab.linkedBrowser, I think? So in fact, you could do:

let otherBrowser = aOtherTab.linkedBrowser;
let otherTabBrowser = otherBrowser.getTabBrowser();

I think?

::: toolkit/content/widgets/browser.xml
@@ +1270,5 @@
>            }
> +          let ourTab = ourTabBrowser.getTabForBrowser(this);
> +          let otherTab = otherTabBrowser.getTabForBrowser(aOtherBrowser);
> +          if (!ourTab || !otherTab) {
> +            this.swapDocShells(aOtherBrowser);

When does this happen? If both browsers have a tabbrowser, I expect that both of them have tabs as well. Am I missing something?

More generally, seems like this would be clearer if written as:


if (ourTabBrowser && otherTabBrowser) {
  let ourTab = ...;
  let otherTab = ...;
  // could potentially have an if() statement here to check for
  // ourTab && otherTab, but we should explain why that's necessary in a comment.
  return ourTabBrowser.swapBrowsers(ourTab, otherTab, aFlags);
}
// One of us is not connected to a tabbrowser, so just swap
return this.swapDocShells(aOtherBrowser);

Comment 17

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

Attachment: Part 2: Correctly swap web progress listeners when swapping frameloaders

MozReview-Commit-ID: KbUd2Os5qac

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8814223 [details] [diff] [review]
Part 3: Use SessionStore to recover from missing GroupedSHistory segments, r=ehsan

Review of attachment 8814223 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly okay, but I'm worried about bringing SessionStore into a toolkit/ binding. See below.

::: toolkit/content/widgets/browser.xml
@@ +417,5 @@
> +        <getter>
> +        <![CDATA[
> +          if (!this._sessionStore) {
> +            let {SessionStore} =
> +              Components.utils.import("resource:///modules/sessionstore/SessionStore.jsm", {});

Bringing this browser/ module into a toolkit-defined binding is a little problematic. We should avoid this if at all possible.

Did you consider putting the navigateAndRestoreByIndex on nsIXULBrowserWindow instead, and putting the implementation in browser/base/content/browser.js's XULBrowserWindow instead? That way, it can safely refer to SessionStore.

Comment 19

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

Attachment: Part 3: Use SessionStore to recover from missing GroupedSHistory segments, r=ehsan

MozReview-Commit-ID: FDmQIjiHox7

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8815046 [details] [diff] [review]
Part 3: Use SessionStore to recover from missing GroupedSHistory segments, r=ehsan

Review of attachment 8815046 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: dom/interfaces/base/nsIBrowser.idl
@@ +58,5 @@
>    /**
>     * Close the browser (usually means to remove a tab).
>     */
>    void closeBrowser();
> +

I guess this extra newline was unintentional.

Comment 21

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 3: Use SessionStore to recover from missing GroupedSHistory segments, r=ehsan, r=mconley

MozReview-Commit-ID: FDmQIjiHox7

Comment 22

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 4: Add a test for navigating into dead PartialSHistories, r=ehsan

Do not run in non-e10s mode.

Comment 23

[Samael Wang [:freesamael] (away for now)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c5fc640fb9004c90cdf58d5ac63a1f897efcef2&group_state=expanded

Another try: -b do is running.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9d879cfa0f7
Part 1: When performing SessionRestore on a GroupedSHistory tab, ensure that it reverts to a complete SHistory state before restoring, r=mikedeboer
https://hg.mozilla.org/integration/mozilla-inbound/rev/447db63f9b10
Part 2: Correctly swap web progress listeners when swapping frameloaders, r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/44a95445e11d
Part 3: Use SessionStore to recover from missing GroupedSHistory segments, r=ehsan, r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/3360b8d2f04b
Part 4: Add a test for navigating into dead PartialSHistories, r=ehsan

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a9d879cfa0f7
https://hg.mozilla.org/mozilla-central/rev/447db63f9b10
https://hg.mozilla.org/mozilla-central/rev/44a95445e11d
https://hg.mozilla.org/mozilla-central/rev/3360b8d2f04b