1624511 - Tab crashes cause total data loss (session/URL/etc.) in all pinned tabs loaded in that content process

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

[Matthew N. [:MattN]]

Attachment: Screen Shot of a pinned tab that happened to be in the same process as a crashed one

Twice in the last few days tab crashes on Nightly have caused total data loss (including the URL and session back/forward history) of all the tabs in that content process. This includes tabs that had been restored for a long time so it wasn't a race condition losing the URL while it's loading (there are known bugs about that). I clicked "Restore All Tabs" on the tab crashed page btw. I don't see anything relevant in the Browser Console.

Notice in my screenshot that the tab title is "New Tab", the back and forward buttons are disabled and the address bar is blank. The only thing that remains is the favicon but I have no idea what bugs I had pinned since the favicons are the same…

Expected result:
I should never lose the session history from crashed tabs and I definitely shouldn't lose the tab's URL.

I didn't try to reproduce the problem in Safe Mode. I filed this is Tabbed Browser, not Session Restore since IMO the URL in the address bar and back/forward history shouldn't have been lost in the first place.

[Tracking Requested - why for this release]: Tab data is lost upon content process crash… the whole point of session restore was to

Comment 1

[Oana Botisan, Desktop QA]

I am not able to reproduce this issue. Do you have some steps or do you know another way to crash a tab beside using about:crashcontent?

The only thing I could reproduce was that if I crashed the whole tab, after Firefox is reopened the previous session can not be restored.

Comment 2

[Matthew N. [:MattN]]

(In reply to Oana Botisan, Desktop Release QA from comment #1)

I am not able to reproduce this issue. Do you have some steps or do you know another way to crash a tab beside using about:crashcontent?

One thing I noticed is that when I had the bug the crashed-restore-all-button = Restore All Crashed Tabs string was shown rather than "Restore This Tab" but I don't know how to trigger that… when I reduce dom.ipc.processCount to two and then open a bunch of tabs and crash the one tab using https://github.com/luser/crashme-simple/blob/master/contentscript.js in the Browser Content Toolbox or using about:crashcontent, it only shows the crash page on the tab and the other tabs just restore from the session.

I just figured out STR with pinned tabs that existed in a previous session (which matches my original experience)… I don't know if the problem applies to non-pinned:

1. In a new profile set browser.startup.page to 3 to enable session restore and dom.ipc.processCount to 2 in about:config (just to make it easier to repro without opening so many tabs)
2. Load 8 tabs of https://example.com/
3. Pin the first 4 of those new tabs
4. Quit Firefox
5. Start Firefox with the same profile (the pinned tabs should be automatically restored, wait for that)
6. Click on a non-pinned example.com tab to load it
7. Load about:crashcontent in that tab

Expected result:

• I can restore the selected non-pinned tab
• Pinned tabs loaded in the same process can also be restored

Actual result:

• I can restore the selected non-pinned tab
• The pinned tabs have no URL or back/forward history and I have no option to restore the page so other session data (e.g. form field values) is also lost.

The only thing I could reproduce was that if I crashed the whole tab, after Firefox is reopened the previous session can not be restored.

I guess that should be filed as a separate bug as that sounds bad too unless you are also talking about pinned tabs.

Comment 3

[Matthew N. [:MattN]]

[Tracking Requested - why for this release]: data loss on (pinned?) tabs from a crash in the same process

Last good revision: f56289d7d8045d53b0c0a48dc35b563ec7f71b01
First bad revision: ea61d93b261638d675374e209c5937f6542f2bbb
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f56289d7d8045d53b0c0a48dc35b563ec7f71b01&tochange=ea61d93b261638d675374e209c5937f6542f2bbb

=> ea61d93b261638d675374e209c5937f6542f2bbb Emma Malysz — Bug 1242912, batch insert tabs during a session restore instead of adding tabs individually. r=Gijs

Comment 4

[Emma Malysz]

It looks like we have most of the correct data for the pinned tab within the revivedCrashedTab method, but I believe the problem is that we return early here https://searchfox.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#3944-3946 and don't end up entering restoreTab.
Looking more into why this._crashedBrowsers.has(browser.permanentKey) would now be false after that patch.

Comment 5

[Emma Malysz]

Loading a pinned tab's data that was in the same process as the crashed tab without out session restore seems to be fixed by Bug 1618936.
We're still ultimately missing the oop-browser-crashed event for the pinned tab and not entering the crashed state.

Comment 6

[Emma Malysz]

Attachment: Bug 1624511, ensure that tab crashes within the same content process can be restored.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa6696b7ade8
ensure that tab crashes within the same content process can be restored. r=Gijs

Comment 8

[Dorel Luca [:dluca]]

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

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:emalysz, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Emma Malysz]

Comment on attachment 9135891 [details]
Bug 1624511, ensure that tab crashes within the same content process can be restored.

Beta/Release Uplift Approval Request

• User impact if declined: We will be collecting incorrect telemetry numbers for our 32 bit users on 64 bit OS.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This implements a check that was missed in bug 1553546 when we moved this information off the main thread.
• String changes made/needed: None

Beta/Release Uplift Approval Request

• User impact if declined: Potential for data loss on tab crashes.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Detailed in Comment #2
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Helps ensure we do not lose data for a pinned tab that is created from the same content process.
• String changes made/needed: None

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 9135891 [details]
Bug 1624511, ensure that tab crashes within the same content process can be restored.

dataloss fix, approved for 75 rc1

Comment 12

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/ea8c56f0814d

Comment 13

[Oana Botisan, Desktop QA]

I managed to reproduce the issue using the steps from comment 2 using a build from 2020-03-23 on Mac OS 10.11.
I verified the fix using latest Nightly 76.0a1 and Firefox 75.0 (https://treeherder.mozilla.org/#/jobs?repo=mozilla-release&revision=ea8c56f0814df739bfa1939e55d53ddab7e40e4c&selectedJob=295423948) on the same OS. The issue is not reproducing anymore.

Comment 14

[antistress]

I also had several times this bug on my 74 version
Thank you for the fix !
I hope that Debian will soon update Firefox to v75