1995651 - Disable backup for users who have sanitizeOnShutdown turned on

Status: VERIFIED FIXED

(Firefox :: Profile Backup, task, P1)

Comment 1

[Harshit Sohaney [:hsohaney]]

Attachment: Bug 1995651 - Disable backup for users who have sanitizeOnShutdown turned on. r=#backup-reviewers

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c2c74067f048
https://hg.mozilla.org/integration/autoland/rev/28ef1ae10736
Disable backup for users who have sanitizeOnShutdown turned on. r=fchasen

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2674356a3e76
https://hg.mozilla.org/integration/autoland/rev/1fe85d2c426a
Revert "Bug 1995651 - Disable backup for users who have sanitizeOnShutdown turned on. r=fchasen" for causing mochitests failures in rowser/browser_settings.js.

Comment 4

[Serban Stanca [:SerbanS]]

Reverted this because it was causing mochitests failures in browser/browser_settings.js.

• Revert link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-FAIL | browser/components/backup/tests/browser/browser_settings.js | Backup section is now hidden - false == true -

Comment 5

[Harshit Sohaney [:hsohaney]]

Thanks! Working on a fix right now

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/8a727550756a
https://hg.mozilla.org/integration/autoland/rev/e73162392bcd
Disable backup for users who have sanitizeOnShutdown turned on. r=fchasen

Comment 7

[Sandor Molnar[:smolnar]]

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

Comment 8

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: This is a part of the MVP requirements for fxBackup. User's might see unexpected data being backed up if they want their data deleted on shutdown.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: yes
• Steps to reproduce for manual QE testing: 1. Make sure you have browser.backup.enabled=true , browser.backup.archive.enabled=true, and browser.backup.restore.enabled=true. These prefs enable all the parts of the backup service and the settings ui.

To test this, set privacy.sanitize.sanitizeOnShutdown=true or enable sanitize on shutdown from about:preferences#privacy.

Expected:
The backup section under about:preferences#sync should disappear and no more backups should be made.

• Risk associated with taking this patch: low
• Explanation of risk level: Adds a check to the backup status getters.
• String changes made/needed: No
• Is Android affected?: no

Comment 9

[Harshit Sohaney [:hsohaney]]

Attachment: Bug 1995651 - Disable backup for users who have sanitizeOnShutdown turned on. r=#backup-reviewers

Original Revision: https://phabricator.services.mozilla.com/D269500

Comment 10

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/60d16bb03c85
https://hg.mozilla.org/releases/mozilla-beta/rev/68c5fc47fd1c

Comment 11

[Bogdan Maris, Desktop Test Engineering [OUT until 12 Jan]]

During testing on Nightly 146.0a1 I noticed that if a user switches the pref privacy.sanitize.sanitizeOnShutdown=true after a backup was made before any backup and deletes a bookmark will also remove the entire backup .html file created previously.

STR:

1. Enable backup feature, add a bookmark and then create a backup
2. Switch the pref privacy.sanitize.sanitizeOnShutdown to the value true
3. Open the Browser console (just to see the error it throws)
4. Open Application menu (hamburger menu) > Bookmarks and delete a bookmark.
5. Manually go to the location where the backup was previously saved

Actual result:

• The backup created earlier is deleted once the bookmark is deleted.

Error from Browser Console:

Uncaught (in promise) TypeError: can't access property "setSelectionRange", input is null
    updated chrome://browser/content/backup/backup-settings.mjs:499
    _$didUpdate chrome://global/content/vendor/lit.all.mjs:875
    performUpdate chrome://global/content/vendor/lit.all.mjs:841
    scheduleUpdate chrome://global/content/vendor/lit.all.mjs:781
    __enqueueUpdate chrome://global/content/vendor/lit.all.mjs:754

 backup-settings.mjs:499:7

Not sure if this is intended but I would think that it is not. Ni? for clarifications.

Comment 12

[Harshit Sohaney [:hsohaney]]

This happens because once the user changes certain items (like bookmarks or passwords), we trigger a new backup to be created. However, before creating a new backup, the old backup is deleted. Since having sanitizeOnShutdown=false, the createBackup call wont work, but the delete will.

I'll let David clarify if this behavior is alright or if we should patch this up. Thanks for catching this!

Comment 13

[Bogdan Maris, Desktop Test Engineering [OUT until 12 Jan]]

Yeah, I thought that was the case why it deleted it.
Just as an another note, this exact thing also happens if we hide the backup/restore when we detect browser.profiles.created=true and trigger a backup (bug 1990634)

Comment 14

[[e-mail]]

I think there's a bigger issue here I hadn't considered... basically any time backup was on and there is a backup file being maintained, and then backup is turned off... whether by the user or by one of these other changes, we should delete the backup file immediately. It should already be gone by the time a bookmark is deleted the next time (for example)

Will move the question to Slack and it might result in a new bug too.

Comment 15

[Monica Chiorean]

Just a follow up, for comment#14 bug 1998176 was logged and fixed.
This issue is verified as fixed on Win11x64 using FF build 146.0b1 and 147.0a1 and 145.0.