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.

MozReview-Commit-ID: 2zNXDtkx3bs

MozReview-Commit-ID: KbUd2Os5qac

MozReview-Commit-ID: FDmQIjiHox7

MozReview-Commit-ID: LT8NeCdf3fm

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

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

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

(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

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 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?

MozReview-Commit-ID: FDmQIjiHox7

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 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);

MozReview-Commit-ID: KbUd2Os5qac

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.

MozReview-Commit-ID: FDmQIjiHox7

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.

MozReview-Commit-ID: FDmQIjiHox7

Do not run in non-e10s mode.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c5fc640fb9004c90cdf58d5ac63a1f897efcef2&group_state=expanded Another try: -b do is running.

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

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