1709136 - Remove the explicit remoteness switch that happens during the restore process

Status: RESOLVED FIXED

(Firefox :: Session Restore, task)

Description

[u608768]

For restores that require remoteness changes, we'll currently call SessionStore._sendRestoreHistory(), then switch the browser's remoteness (with updateBrowserRemotenessByURL()), and then call _sendRestoreHistory() again on the post-remoteness-flip browser.

This causes complications for SSTabRestoring timing (bug 1702055 required a few hacks to fix these). We should remove the first restoreHistory() call and the updateBrowserRemotenessByURL() call.

Fission:M8 but this is mostly just clean-up, so can be pushed further if need be.

Comment 1

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1709136 - Part 1: Remove the explicit remoteness switch that happens during the restore process, r=nika!,kashav!

Comment 2

[Anny Gakhokidze [:annyG]]

There has been several issues I have uncovered so far that stem from not executing the explicit remoteness switch in addition to changing the SetCurrentURI call within SessionStoreUtils::RestoreDocShellState to pass false for aFireOnLocationChange argument.

1. In the case when we open a new tab, scroll on it, duplicate the tab, and then force reload, the scroll position wasn't being reset. I tracked it down to two flush calls, where the first flush call was supposed to reset the session store, but because it's epoch value was not considered recent (the epoch has been deleted from sessionstore's set), we ignored the update. Instead, the flush call from checkScroll function went through with the scroll values before the force reload. This was happening because when we reset the epoch upon XULFrameLoaderCreated event, we didn't update it in other places, namely we didn't update the session store listener's value. A simple call to requestEpochUpdate fixed this problem.

2. When the aFireOnLocationChange argument is false, then we get the wrong value here. When I check the session history entry's uri, it is about:rights, as expected. So this means that the currentURI of the browser doesn't get updated because updateForLocationChange doesn't get called, which eventually can be traced down to the nsDocShell not sending the onLocationChange event. However, if I set aFireOnLocationChange to true, then I have a test like browser/components/sessionstore/test/browser_restore_container_tabs_oa.js failing (as well as some others) with a bunch of assertions. We end up firing XULFrameLoaderCreated event more often than we want, sometimes we get the remote type wrong ("web" instead of the "webIsolated=https://example.com"). This leads me to believe that setting aFireOnLocationChange to true just fires off the event too early.
   This situation makes bug 1709132 more pertinent - the solution here might be just to remove SetCurrentURI call as kashav pointed out in the comments there.
   Kashav and I talked about this, he came up with preliminary patches for removing SetCurrentURI, and figured that the patches probably need to land at the same time as patches for this current bug. Sounds good.

3. And there is the ever persistent bug with the http redirect. [Here] we keep getting the pre-redirect url. The current SH entry also shows the wrong url. I am currently debugging this.

Comment 3

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1709136 - Part 2: Update SessionHistoryEntry in ReplaceLoadingSessionHistoryEntryForLoad, r=smaug!

Depends on D116209

Comment 4

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1709136 - Part 3: Remove nsIDocShell::SetCurrentURI call in RestoreDocShellState, r=nika!

Depends on D117238

Comment 5

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1709136 - Part 4: Fix consumers of SSTabRestoring to retrieve current browser URI from the event detail, r=nika!

Depends on D117239

Comment 6

[Anny Gakhokidze [:annyG]]

Attachment: Bug 1709136 - Part 5: Fix test consumers of SSTabRestoring that relied on browser.currentURI to be set, r=kashav!

Depends on D117240

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7289a07292f6
Part 1: Remove the explicit remoteness switch that happens during the restore process, r=nika,kashav
https://hg.mozilla.org/integration/autoland/rev/90b17c352a43
Part 2: Update SessionHistoryEntry in ReplaceLoadingSessionHistoryEntryForLoad, r=smaug

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/7289a07292f6
https://hg.mozilla.org/mozilla-central/rev/90b17c352a43