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

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Alice0775 White]

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

Comment 1

[Alice0775 White]

Disable Fission will fix the issue.

Comment 2

[Alice0775 White]

Regression window(using cached build):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c25899d7b631470b983650ee254177381a62eaad&tochange=7d7faf0b6d7a7ee76d6685a61f26b4c20a6e5d29
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c26c356d8d197c2c2c90cb74549538cc03983898&tochange=1ac1579b1471067fdeb105b8f1fed563326f5451

So, suspect: Bug 1668215

Comment 3

[Chris Peterson [:cpeterson]]

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.

Comment 4

[Chris Peterson [:cpeterson]]

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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 6

[Alice0775 White]

(In reply to Alice0775 White from comment #2)

Regression window(using cached build):
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c25899d7b631470b983650ee254177381a62eaad&tochange=7d7faf0b6d7a7ee76d6685a61f26b4c20a6e5d29
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c26c356d8d197c2c2c90cb74549538cc03983898&tochange=1ac1579b1471067fdeb105b8f1fed563326f5451

So, suspect: Bug 1668215

Further bisection:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ee7eeea3fb66321fabf86ffd3a1caf4fa3511e43&tochange=1aa67f749387d4d6d26f7d69511434ac71112110

Regressed by: Bug 1668357

Comment 7

[u608768]

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.

Comment 8

[Alice0775 White]

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

Comment 9

[u608768]

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.

Comment 10

[u608768]

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

Comment 11

[u608768]

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

Comment 12

[u608768]

Attachment: WIP: Bug 1724370

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Lyubomir [:mystiquewolf]]

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

Comment 16

[Chris Peterson [:cpeterson]]

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

Comment 17

[u608768]

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

Comment 18

[u608768]

Attachment: Bug 1724370 - Ensure shutdown is blocked on all window flushes, r?farre

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.

Comment 19

[u608768]

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

Comment 20

[Alice0775 White]

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

Comment 21

[u608768]

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

Comment 22

[Alice0775 White]

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

Comment 23

[u608768]

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

Comment 24

[Alice0775 White]

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d06db2fa1c24
Ensure shutdown is blocked on all window flushes, r=farre

Comment 26

[u608768]

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

Comment 27

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/d06db2fa1c24

Comment 28

[u608768]

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:

Comment 29

[u608768]

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.

Comment 30

[Pascal Chevrel:pascalc]

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.

Comment 31

[Pascal Chevrel:pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/bd7074b75f0a

Comment 32

[Alexandru Trif, Desktop Test Engineering [:atrif]]

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

Comment 33

[u608768]

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.

Comment 34

[Alexandru Trif, Desktop Test Engineering [:atrif]]

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