Closed Bug 1724370 Opened 4 months ago Closed 3 months ago

[Fission] Recently opened tabs (~15s) are not restored correctly when restarting the browser.

Categories

(Firefox :: Session Restore, defect)

Firefox 92
Desktop
Windows 10
defect

Tracking

()

RESOLVED FIXED
94 Branch
Fission Milestone MVP
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- unaffected
firefox90 --- unaffected
firefox91 --- disabled
firefox92 + wontfix
firefox93 + fixed
firefox94 --- fixed

People

(Reporter: alice0775, Assigned: kashav)

References

(Blocks 1 open bug)

Details

(Keywords: nightly-community, Whiteboard: fission-hard-blocker)

Attachments

(2 files)

[Tracking Requested - why for this release]:

Steps To reproduce:

  1. Enable 'Restore previous session'
  2. Open https://www.wikipedia.org/ in 1st tab
  3. Open New tab(2nd tab) and Open https://www.mozilla.org/en-US/contribute/ in this tab(2nd tab)
    4 Restart Browser

Actual Results:
Tabs are not restored correctly
Two 'New Tab' are restored sometimes. Or 2nd tab is 'New Tab'. Or only one tab is restored. Or Only one tab with 'New Tab' is opened.

Expected Results:
Tabs are should be restored properly.

Disable Fission will fix the issue.

Summary: Tabs are not restored correctly when restarting the browser. → [Fission] Tabs are not restored correctly when restarting the browser.

Olli, your fix for LoadingSessionHistoryInfo bug 1668215 caused a regression in session restore.

Kashav, since this is a session restore bug, can you please take a look? Olli is busy with some other Fission M8 and MVP bugs.

Fission Milestone: --- → MVP
Flags: needinfo?(kmadan)
Flags: needinfo?(bugs)
Whiteboard: fission-soft-blocker

Tentatively tracking this bug as a hard blocker for Fission MVP. A bug in session restore is serious. OTOH, the regressing bug landed ten months ago and no one reported this bug until now.

Whiteboard: fission-soft-blocker → fission-hard-blocker

Hmm, bug 1668215 is really ancient and we didn't have working session store for Fission at that point.

Flags: needinfo?(bugs)
Flags: needinfo?(bugs)

I haven't been able to reproduce this, on Linux at least. How are you restarting the browser? I wonder if we're dropping shutdown flushes on Windows for some reason.

Assignee: nobody → kmadan
Flags: needinfo?(kmadan)

(In reply to :kashav (he/him) from comment #7)

I haven't been able to reproduce this, on Linux at least. How are you restarting the browser? I wonder if we're dropping shutdown flushes on Windows for some reason.

Steps 1-4 are the preparation phase for a new profile:

  1. Enable 'Restore previous session' in about:preferences#general

  2. Enable 'Fission (Site Isolation)' in about:preferences#experimenta
    Restart as instructed by the browser (and close default browser notification if any).

  3. Press Ctrl+W several times to close tabs and the browser.

  4. Start Nightly (and close default browser notification if any)
    Now, only one tab(Firefox Home) is opened.

  5. Open https://www.wikipedia.org/ in 1st tab

  6. Open New tab(2nd tab) and Open https://www.mozilla.org/en-US/contribute/ in the tab(2nd tab)

  7. Wait for stop tab spinner animation

  8. Quit Nightly (Ctrl+Shift+Q) (and close multiple tab close notification if any)

  9. Start Nightly

Tabs are not restored, only one tab(New Tab) is opened. 100% reproducible for me.

I can reproduce, thanks for the detailed STR.

We're collecting history changes correctly, but sometimes don't write the flushed data to disk soon enough on shutdown. In theory the flushed data is sent to SessionStore at the same time with and without SHIP, but there may be some timing differences that are causing a delay. We may have to block shutdown on a final SessionSaver write (or figure something else out if that's not possible). It's actually a little odd that this works perfectly with SHIP disabled.

We'll still be saving the last ~15s of changes (or whatever browser.sessionstore.interval is set to), so I'm not sure if this has to be a hard blocker.

Flags: needinfo?(bugs)
Summary: [Fission] Tabs are not restored correctly when restarting the browser. → [Fission] Recently opened tabs (~15s) are not restored correctly when restarting the browser.

This isn't actually a regression. It's just a bug in how we collect session history on shutdown with SHIP enabled.

Keywords: regression
No longer regressed by: 1668357

(In reply to :kashav (he/him) from comment #9)

We're collecting history changes correctly, but sometimes don't write the flushed data to disk soon enough on shutdown.
It's actually a little odd that this works perfectly with SHIP disabled.

So this problem actually exists without Fission too.

The reason the bug doesn't reproduce without SHIP is because ContentSessionStore uses idle dispatch to push history updates to the parent, which means history updates are generally (always?) pushed to Session Store well before the 15s interval timer expires. Then when Session Store receives the "quit-application" event (which is when it closes the session file and saves final state to disk), it has an up-to-date copy of session history data.

We're going to have to live with this for 92 for the Fission release experiments.

Duplicate of this bug: 1725618

(In reply to Ryan VanderMeulen [:RyanVM] from comment #13)

We're going to have to live with this for 92 for the Fission release experiments.

Does this mean that users in the stable channel have to explicitly disable Fission to prevent their session from being lost?

Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm) → needinfo?(kmadan)

(In reply to Lyubomir from comment #15)

Does this mean that users in the stable channel have to explicitly disable Fission to prevent their session from being lost?

If I understand correctly, only tabs opened less than 15 seconds before quitting Firefox will not be saved.

(In reply to Chris Peterson [:cpeterson] from comment #16)

(In reply to Lyubomir from comment #15)

Does this mean that users in the stable channel have to explicitly disable Fission to prevent their session from being lost?

If I understand correctly, only tabs opened less than 15 seconds before quitting Firefox will not be saved.

Yeah, slightly worse: we'll lose any navigation that happened in the last ~15 seconds before shutdown.

Flags: needinfo?(kmadan)

We currently only block shutdown on flushes for windows that appear in the
BrowserWindowTracker.orderedWindows list, which may or may not include the most
recent window (this appears to depend on how the window was closed). Including
windows for which we saw "domwindowclosed" means that we'll also wait on flushes
for the most recent window.

This also ensures that we're queuing a SessionStoreUpdate from TabListener on
STATE_START/STATE_STOP (as ContentSessionStore.jsm does), and fixes a small bug
where we were trying to remove an observer with the deferred.reject callback,
instead of the actual observer.

Flags: needinfo?(alice0775)

(In reply to :kashav (he/him) from comment #19)

Alice, can you test the STR from comment #8 with a build from https://treeherder.mozilla.org/jobs?repo=try&revision=ef103de53a3957dff0da28cac2caa7f595a0ca0d?

I can still reproduce the issue in the try build(Windows 2012 x64 opt B).

Flags: needinfo?(alice0775)

(In reply to :kashav (he/him) from comment #21)

What about this one: https://treeherder.mozilla.org/jobs?repo=try&revision=350be18272a5af40e04c37dd3d0a153216f7aa0d?

same, I can reproduce the issue.

(In reply to :kashav (he/him) from comment #23)

Thanks for testing these. What about https://treeherder.mozilla.org/jobs?repo=try&revision=af2782392579fbb8c488f411920b5d6208336af5?

Yes, The try build fixes the issue. No longer reproduce the issue.

Pushed by kmadan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d06db2fa1c24
Ensure shutdown is blocked on all window flushes, r=farre

We'll still want to land the changes from D123156 eventually, but that's a larger project.

Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch

Comment on attachment 9239272 [details]
Bug 1724370 - Ensure shutdown is blocked on all window flushes, r?farre

Beta/Release Uplift Approval Request

  • User impact if declined: Any users with Fission enabled will lose new tabs or navigations that happened in the last ~15s before shutdown.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: See comment #8.
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): Fixes a bug in the session store shutdown blocker (for Fission and non-Fission), but this may delay shutdown if something goes wrong. The blocker is still bounded by toolkit.asyncshutdown.crash_timeout - 10ms, so in the worst case it'll just cause shutdown to take a little longer.
  • String changes made/needed:
Attachment #9239272 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Has the fix been verified in Nightly?: Yes

We haven't actually had a Nightly with this patch yet, so this was based on comment #24 and other manual testing. Should probably wait a bit before actually uplifting.

QA Whiteboard: [qa-triaged]

Comment on attachment 9239272 [details]
Bug 1724370 - Ensure shutdown is blocked on all window flushes, r?farre

This has baked a few days on nightly with no regression reported, let's uplift to 93 beta 5, thanks.

Attachment #9239272 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Hello! Reproduced the issue with Firefox 93.0a1 (20210809213353) on Windows 10x64.

Tried to verify the issue with 93.0b5 (20210914185637) and 94.0a1 (20210915092453) but it seems that the issue is still reproducible when following STR from comment 8. This seems to be fixed if Firefox is closed only with one New tab opened and repeating steps 5-9 from comment 8. This applies to the affected build as well. Screen recording here: link. Should we reopen this bug? Thank you!

Note: On 93.0b5, I used the pref fission.autostart:true instead of enabling Site Isolation (about:preferences#experimental not on beta).

Flags: needinfo?(kmadan)

Thanks for checking. When this happens, can you check if History > Restore Previous Session is available? I'm noticing that sometimes after a restart the "Open previous windows and tabs" option becomes unchecked (which is unrelated to this bug, but perhaps related to the ongoing bug 1724959 work).

Edit: I'm also able to reproduce what happens in the recording for new profiles. Fortunately I haven't been able to reproduce the problem of tabs/navigations being lost for existing profiles, so that makes this less worse (since hopefully most users won't be creating new profiles and then immediately restarting?), but we should figure this out still.

Flags: needinfo?(kmadan) → needinfo?(alexandru.trif)

(In reply to :kashav (he/him) from comment #33)

Thanks for checking. When this happens, can you check if History > Restore Previous Session is available? I'm noticing that sometimes after a restart the "Open previous windows and tabs" option becomes unchecked (which is unrelated to this bug, but perhaps related to the ongoing bug 1724959 work).

Restore Previous Session is not available inside History and Open previous windows and tabs is still checked inside Preferences. Screen recording here. If more information is needed please let me know!

Flags: needinfo?(alexandru.trif)
You need to log in before you can comment on or make changes to this bug.