Closed Bug 1709136 Opened 4 years ago Closed 3 years ago

Remove the explicit remoteness switch that happens during the restore process

Categories

(Firefox :: Session Restore, task)

task

Tracking

()

RESOLVED FIXED
91 Branch
Fission Milestone M7a
Tracking Status
firefox91 --- fixed

People

(Reporter: u608768, Assigned: annyG)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files, 3 obsolete files)

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.

Summary: Remove the remoteness switch that happens during the restore process → Remove the explicit remoteness switch that happens during the restore process
Fission Milestone: M8 → M7a
Assignee: nobody → agakhokidze
Status: NEW → ASSIGNED
Blocks: 1709132

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.

Attachment #9223951 - Attachment description: WIP: Bug 1709136 - Remove the explicit remoteness switch that happens during the restore process → Bug 1709136 - Part 1: Remove the explicit remoteness switch that happens during the restore process, r=nika!,kashav!
Blocks: 1703351
Attachment #9226015 - Attachment is obsolete: true
Attachment #9226016 - Attachment is obsolete: true
Attachment #9226017 - Attachment is obsolete: true
Pushed by agakhokidze@mozilla.com: 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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 91 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: