Closed Bug 1318767 Opened 3 years ago Closed 3 years ago

Support navigating into PartialSHistories which have dead frameloaders

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Nika, Assigned: Nika)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files, 7 obsolete files)

10.18 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
3.66 KB, patch
dao
: review+
Details | Diff | Splinter Review
10.29 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
2.83 KB, patch
freesamael
: review+
Details | Diff | Splinter Review
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.
Blocks: 1310770
Blocks: 1310766
MozReview-Commit-ID: KbUd2Os5qac
Attachment #8813825 - Flags: review?(gijskruitbosch+bugs)
MozReview-Commit-ID: FDmQIjiHox7
Attachment #8813826 - Flags: review?(ehsan)
MozReview-Commit-ID: LT8NeCdf3fm
Attachment #8813827 - Flags: review?(ehsan)
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).
Attachment #8813825 - Flags: review?(gijskruitbosch+bugs)
(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 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?
Attachment #8813824 - Flags: review?(mdeboer) → review+
Flags: needinfo?(michael)
(potential conflict warning: bug 1147911)
(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.
Flags: needinfo?(michael)
(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.
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
Attachment #8814188 - Flags: review?(gijskruitbosch+bugs)
Attachment #8813825 - Attachment is obsolete: true
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
Attachment #8814203 - Flags: review?(mdeboer)
Attachment #8813824 - Attachment is obsolete: true
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?
Attachment #8813826 - Flags: review?(ehsan) → review+
Attachment #8813827 - Flags: review?(ehsan) → review+
MozReview-Commit-ID: FDmQIjiHox7
Attachment #8814223 - Flags: review?(mconley)
Attachment #8813826 - Attachment is obsolete: true
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!
Attachment #8814203 - Flags: review?(mdeboer) → review+
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);
Attachment #8814188 - Flags: review?(gijskruitbosch+bugs) → feedback+
MozReview-Commit-ID: KbUd2Os5qac
Attachment #8814465 - Flags: review?(dao+bmo)
Attachment #8814188 - Attachment is obsolete: true
Blocks: 1320391
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.
Attachment #8814223 - Flags: review?(mconley) → review-
MozReview-Commit-ID: FDmQIjiHox7
Attachment #8815046 - Flags: review?(mconley)
Attachment #8814223 - Attachment is obsolete: true
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.
Attachment #8815046 - Flags: review?(mconley) → review+
Attachment #8814465 - Flags: review?(dao+bmo) → review+
Attachment #8818203 - Attachment description: Part 3: Use SessionStore to recover from missing GroupedSHistory segments, → Part 3: Use SessionStore to recover from missing GroupedSHistory segments, r=ehsan, r=mconley
Attachment #8818203 - Flags: review+
Attachment #8815046 - Attachment is obsolete: true
Attachment #8818488 - Attachment description: Part 4: Add a test for navigating into dead PartialSHistories, → Part 4: Add a test for navigating into dead PartialSHistories, r=ehsan
Attachment #8818488 - Flags: review+
Attachment #8813827 - Attachment is obsolete: true
Pushed by echen@mozilla.com:
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
Depends on: 1331601
Depends on: 1317293
No longer depends on: 1317293
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.