1907783 - Browsing data loss when using the new clear history dialog if Firefox is not closed gracefully

Status: VERIFIED FIXED

(Toolkit :: Data Sanitization, defect, P1)

Description

[kanapa1]

Steps to reproduce:

1. Run Firefox version 128 stable or newer.
2. Make sure the privacy.sanitize.useOldClearHistoryDialog pref is set to "false", which is now the default.
3. Browse the web and open a few web pages to populate your local browsing history, or go directly to step 4 if you already have any history entries in your current profile.
4. Go to: Hamburger application menu → Settings → Privacy & Security (or directly to about:preferences#privacy), then enable the "Clear history when Firefox closes" option and configure it so that anything except browsing history is cleared for obvious reasons, let’s say temporary cached files and pages.
5. Forcefully close Firefox by issuing the following commands in a terminal: taskkill /f /im firefox.exe on Windows or kill -SIGKILL $(ps -x | grep firefox | awk '{print $1}') on Linux or in any other way that allows you to forcefully close a running application.
6. Normally launch Firefox again as if nothing had happened.
7. See what is described in the "Actual results" section.

Actual results:

During the very first launch after the forced shutdown, Firefox completely wipes out the entire browsing and download history, form data, site settings, cache, and cookies in the current profile, thereby causing permanent loss of user data with no built-in method to bring it back (or at least not until the profile backup is shipped later this year, which :mconley is actively working on).

Expected results:

After a forced shutdown of Firefox due to a freeze, BSOD/kernel panic, or an unexpected power loss, which are not pleasant experiences in themselves, Firefox should not further harm and intentionally get rid of user data without the user's consent.

Additional information:

One thing I noticed when comparing the new implementation in version 128 with the old one in 127, and without delving into the source code, is that every time Firefox is launched, it fills the privacy.sanitize.pending pref array with a few objects. One of them always contains the following array as one of its members: "itemsToClear": ["cache", "cookies", "history", "formdata", "downloads", "sessions"], which was not the case in version 127, where the content of the array was dynamic and depended on the user's settings on what they wanted to be cleared during a shutdown.

During a shutdown, however, the new implementation seems to use some other preferences to determine what data the user wants cleared. It then resets that array to the default value [] as the old implementation did.

This comes down to the key thing. That empty array prevents the browsing history and other data from being cleared during the next startup. However, forcibly closing Firefox prevents it from resetting that array to the default value [] during the shutdown, so the next time Firefox is launched, the contents of that array (i.e. cache, cookies, history, form data, downloads, and sessions) are read, and all user data is wiped out.

Regression range:

With the privacy.sanitize.useOldClearHistoryDialog pref forced to false, Mozregression points to bug 1856417 and bug 1861450 as the culprits (otherwise, bug 1896949 is shown, which is the one that let the new clear history dialog ride the trains to the release channel). Here's the relevant pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8b80895bf4a3bd55d159062a8741c771555285dd&tochange=244e6e5092d8ec91690845b38264d332b87983c4

Comment 1

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

:hsohaney, since you are the author of the regressor, bug 1896949, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Thorin [:thorin]]

IIRC on ungraceful exits, there is a sanitize on startup fallback which I think pbz and hannah implemented

Comment 3

[Marco Bonardo [:mak]]

Sanitizer.sys.mjs indeed retries the clear on startup if it failed on shutdown, and to do that it uses the pending pref to extract the last pending sanitization.
It looks like this was not handled correctly, indeed multiple code points still use PREF_SHUTDOWN_BRANCH, but the new dialog moved to a different pref branch:
https://searchfox.org/mozilla-central/rev/54a5c4f14f3eb514a29e0cebcb5a095144bcd450/browser/modules/Sanitizer.sys.mjs#178,185,346,352,363

Comment 4

[Harshit Sohaney [:hsohaney]]

Working on this now! Thanks for finding this

Comment 5

[Harshit Sohaney [:hsohaney]]

Attachment: Bug 1907783 - Use the new clear on shutdown branch to prevent losing pending shutdown items on unexpected shutdown. r?pbz

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c6bb39247ff
Use the new clear on shutdown branch to prevent losing pending shutdown items on unexpected shutdown. r=pbz,places-reviewers,Standard8

Comment 7

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/5c6bb39247ff

Comment 8

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

The patch landed in nightly and beta is affected.
:hsohaney, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox129 to wontfix.

For more information, please visit BugBot documentation.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

This is going to need some bake time before it ships to users due to the dataloss risk, so it's not a good candidate for a 128 dot release. Let's aim to have this fix ship in Fx129.

Comment 10

[kanapa1]

I can confirm that the latest Nightly with this patch included seems to work properly again, just like Firefox 127.

In my opinion as a user, uplifting this patch to the release channel will not result in any greater risk of browsing data loss than is currently the case with Firefox version 128.0. At present, for example, an unexpected power loss will wipe out your entire browsing history and more (which happened to me several times already) unless you know in advance what is going to happen and prepare for it beforehand by modifying a few prefs in about:config.

But I may be wrong, so perhaps someone familiar with this part of the Firefox codebase could comment on this.

Comment 11

[Paul Zühlcke [:pbz]]

Thanks for confirming!

A release uplift is risky per-se so it has to be weighted against the severity of the bug being addressed. While it's a dataloss bug and it's really annoying to loose all that data it's still an edge case. If we uplift code to release we risk introducing new regressions. That's why we've decided to fix it in the upcoming release instead and get some more testing time in Nightly and Beta.

Comment 12

[Paul Zühlcke [:pbz]]

Comment on attachment 9412868 [details]
Bug 1907783 - Use the new clear on shutdown branch to prevent losing pending shutdown items on unexpected shutdown. r?pbz

Beta/Release Uplift Approval Request

• User impact if declined: Potential dataloss on unexpected shutdown of Firefox. This affects users who have clearing on shutdown enabled.
• 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: See comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): It's a small code change, but since it's updating how clearing on shutdown is handled there is risk of further dataloss if we overlooked something.
  The change has automated test coverage though and has been in Nightly for a couple days and we haven't observed any issues.
• String changes made/needed:
• Is Android affected?: No

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Dataloss bug
• User impact if declined: Potential dataloss on unexpected shutdown of Firefox. This affects users who have clearing on shutdown enabled.
• Fix Landed on Version:
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): It's a small code change, but since it's updating how clearing on shutdown is handled there is risk of further dataloss if we overlooked something.
  The change has automated test coverage though and has been in Nightly for a couple days and we haven't observed any issues.

Comment 13

[Donal Meehan [:dmeehan]]

Comment on attachment 9412868 [details]
Bug 1907783 - Use the new clear on shutdown branch to prevent losing pending shutdown items on unexpected shutdown. r?pbz

Approved for 129.0b8

Comment 14

[Pulsebot]

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

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9412868 [details]
Bug 1907783 - Use the new clear on shutdown branch to prevent losing pending shutdown items on unexpected shutdown. r?pbz

Approved for 128.1esr.

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/1aacb4ce03b6

Comment 17

[Harshit Sohaney [:hsohaney]]

Uplifted to 128.1esr and 129.0b8 by pbz

Comment 18

[Daniel Bodea [:danibodea]]

I can reproduce the issue in Release v128.0.8 and Nightly v130.0a1 from 2024-07-10, while it does not reproduce in ESR v115.13.0esr; The history is cleared on an unexpected shutdown of Firefox when clearing on shutdown is enabled.
This issue no longer reproduces in Nightly v130.0a1 from 2024-07-24, Beta v129.0b8 while it still reproduces in the latest ESR v128.0esr. It will be verified in ESR v128.1esr when it is available.
Testing was performed on Windows 10, MacOS 11 and Ubuntu 22. Thank you.

Comment 19

[Daniel Bodea [:danibodea]]

I can reproduce this issue in ESR v128.0esr and I can confirm the fix in ESR v128.1.0esr. The testing was performed on Windows 10 and MacOS 11.