Closed Bug 1673617 Opened 4 years ago Closed 4 years ago

Reduce complexity of session restore with SHIP

Categories

(Core :: DOM: Navigation, task, P1)

task

Tracking

()

RESOLVED FIXED
87 Branch
Fission Milestone M6c
Tracking Status
firefox87 --- fixed

People

(Reporter: neha, Assigned: jesup)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 5 obsolete files)

Mainly cleanup required for session restore to work correctly with SHIP, which will fix some session restore test failures.

Assignee: nobody → kmadan
Severity: -- → N/A
Status: NEW → ASSIGNED
Fission Milestone: --- → M6c
Priority: -- → P1

This reverts some of the work from bug 1507287. It tries to clean-up how
restores work when SHiP is enabled by splitting up the process into explicit
phases that both content and parent attempt to follow.

WIP: There's a few TODOs through the code, and probably a number of corner cases
that haven't been tested yet.

Attachment #9184653 - Attachment description: Bug 1673617 - Avoid registering multiple listeners on the same browser element [WIP] → Bug 1673617 - Avoid registering multiple listeners on the same browser element, r?nika
Attachment #9184102 - Attachment description: Bug 1673617 - WIP → Bug 1673617 - Refactor how session restore works with SHIP, r?nika

Depends on D95227

Based on :kashav's most recent try run (https://treeherder.mozilla.org/jobs?repo=try&revision=365c54434cf41e6a8e31ae3a89ad9de79388a774, based on comments from https://phabricator.services.mozilla.com/D94900#3103893), I took a quick look at the failures which were occuring. The first failure in the log was TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_615394-SSWindowState_events_duplicateTab.js | undefined assertion name - Got +0, expected 1.

This assertion is being fired here: https://searchfox.org/mozilla-central/rev/277ab3925eae21b419167a34624ec0ab518d0c94/browser/components/sessionstore/test/browser_615394-SSWindowState_events_duplicateTab.js#46, which is caused by the SSWindowStateReady event not having fired before the SSTabRestoring event is fired. This makes sense with the changes in the revision, as SSWindowStateReady is fired from https://searchfox.org/mozilla-central/rev/277ab3925eae21b419167a34624ec0ab518d0c94/browser/components/sessionstore/SessionStore.jsm#4758.

Previously, this would have fired before the SSTabRestoring event, as that event would be fired from the receiveMessage callback from the content process after restoring session history is complete here: https://searchfox.org/mozilla-central/rev/277ab3925eae21b419167a34624ec0ab518d0c94/browser/components/sessionstore/SessionStore.jsm#1421, which wouldn't be called until at least one spin of the event loop after the history restore had been requested here: https://searchfox.org/mozilla-central/rev/277ab3925eae21b419167a34624ec0ab518d0c94/browser/components/sessionstore/SessionStore.jsm#4701-4706.

With kashav's changes, this now flipped around, as we can restore history synchronously, due to the history object being in the parent process. This means in turn that SSTabRestoring will fire before SSWindowStateReady, due to _sendRestoreHistory being called before _setWindowStateReady.

It may be enough to delay the call to SSTabRestoring by dispatching the call to _restoreHistoryComplete to the event loop.

I am uncertain whether this will fix any of the other test failures after the first one in that try push, and haven't looked into them at all.

ni? :jesup as requested by :neha

Flags: needinfo?(rjesup)
Blocks: 1674871
Flags: needinfo?(rjesup)
Depends on: 1680911
Depends on: 1572084

Randell is fixing the remaining test failures in Kashav's patches to get them ready for review and landing.

Assignee: kmadan → rjesup

With mods from kashav's last Try run

Attachment #9184102 - Attachment is obsolete: true
Attachment #9184102 - Attachment is obsolete: false
Attachment #9184102 - Attachment is obsolete: true
Attachment #9189480 - Attachment is obsolete: true
Attachment #9192634 - Attachment is obsolete: true
Attachment #9192649 - Attachment is obsolete: true
Blocks: 1679858
Attachment #9185556 - Attachment is obsolete: true
Depends on: 1687495
Blocks: 1687517
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/3ce0d9f55436 Avoid registering multiple listeners on the same browser element, r=nika https://hg.mozilla.org/integration/autoland/rev/533092a71225 Create BrowsingContext::mChildSessionHistory more aggressively, r=nika
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/f0ea749647d3 Refactor how session restore works with SHIP, r=nika
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/d230e44fd9be Avoid registering multiple listeners on the same browser element, r=nika https://hg.mozilla.org/integration/autoland/rev/6fa47045b11b Create BrowsingContext::mChildSessionHistory more aggressively, r=nika https://hg.mozilla.org/integration/autoland/rev/2a0dd70342ce Refactor how session restore works with SHIP, r=nika
Whiteboard: [ETA ? patches landed but backed out]
Regressions: 1690134
Pushed by rjesup@wgate.com: https://hg.mozilla.org/integration/autoland/rev/56962291638a Avoid registering multiple listeners on the same browser element, r=nika https://hg.mozilla.org/integration/autoland/rev/ca97eba8c7c3 Create BrowsingContext::mChildSessionHistory more aggressively, r=nika https://hg.mozilla.org/integration/autoland/rev/7c8497aa7523 Refactor how session restore works with SHIP, r=nika
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch
Whiteboard: [ETA ? patches landed but backed out]
Flags: needinfo?(rjesup)
No longer depends on: 1687495
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: