commit 59c2bad9f0fb05ef777ff974e7bbac6fd622bafb
parent 5d163df8ca6fa596e244440f2721becc238efa80
Author: Chris DuPuis <cdupuis@mozilla.com>
Date: Fri, 7 Nov 2025 17:40:24 +0000
Bug 1997090 - reverse order of delete and create backup r=hsohaney
When there is reason to automatically regenerate the backup, the current behavior is to first delete old backup and then create a new one. This leaves a period where there is no backup file in existence, while the new backup is being generated.
To avoid leaving the user without a backup file, switch the order, so we first create the new backup file and then delete the old one.
Differential Revision: https://phabricator.services.mozilla.com/D271509
Diffstat:
3 files changed, 58 insertions(+), 43 deletions(-)
diff --git a/browser/components/backup/BackupService.sys.mjs b/browser/components/backup/BackupService.sys.mjs
@@ -1195,16 +1195,10 @@ export class BackupService extends EventTarget {
this.#postRecoveryResolver = resolve;
this.#backupWriteAbortController = new AbortController();
this.#regenerationDebouncer = new lazy.DeferredTask(async () => {
- if (
- !this.#backupWriteAbortController.signal.aborted &&
- this.archiveEnabledStatus.enabled
- ) {
- await this.deleteLastBackup();
- if (lazy.scheduledBackupsPref) {
- await this.createBackupOnIdleDispatch({
- reason: "user deleted some data",
- });
- }
+ if (!this.#backupWriteAbortController.signal.aborted) {
+ await this.createBackupOnIdleDispatch({
+ reason: "user deleted some data",
+ });
}
}, BackupService.REGENERATION_DEBOUNCE_RATE_MS);
}
@@ -4046,7 +4040,7 @@ export class BackupService extends EventTarget {
* not been sent to the application for at least
* IDLE_THRESHOLD_SECONDS_PREF_NAME seconds.
*/
- onIdle() {
+ async onIdle() {
lazy.logConsole.debug("Saw idle callback");
if (!this.#takenMeasurements) {
this.takeMeasurements();
@@ -4092,12 +4086,19 @@ export class BackupService extends EventTarget {
// loop in the parent process isn't so busy with higher priority things.
let expectedBackupTime =
lastBackupDate + lazy.minimumTimeBetweenBackupsSeconds;
- this.createBackupOnIdleDispatch({
- reason:
- expectedBackupTime < this._startupTimeUnixSeconds
- ? "missed"
- : "idle",
- });
+ try {
+ await this.createBackupOnIdleDispatch({
+ reason:
+ expectedBackupTime < this._startupTimeUnixSeconds
+ ? "missed"
+ : "idle",
+ });
+ } catch (e) {
+ lazy.logConsole.error(
+ "createBackupOnIdleDispatch promise rejected",
+ e
+ );
+ }
} else {
lazy.logConsole.debug(
"Last backup was too recent. Not creating one for now."
@@ -4121,10 +4122,11 @@ export class BackupService extends EventTarget {
* is not busy with higher priority events. This is intentionally broken out
* into its own method to make it easier to stub out in tests.
*
- * @param {...*} args
- * Arguments to pass through to createBackup.
+ * @param {object} [options]
+ * @param {boolean} [options.deletePreviousBackup]
+ * @param {string} [options.reason]
*/
- createBackupOnIdleDispatch(...args) {
+ createBackupOnIdleDispatch({ deletePreviousBackup = true, reason }) {
let now = Math.floor(Date.now() / 1000);
let errorStateDebugInfo = Services.prefs.getStringPref(
BACKUP_DEBUG_INFO_PREF_NAME,
@@ -4143,15 +4145,27 @@ export class BackupService extends EventTarget {
lazy.logConsole.debug(
`We've already retried in the last ${lazy.minimumTimeBetweenBackupsSeconds}s. Waiting for next valid idleDispatch to try again.`
);
- return;
+ return Promise.resolve();
}
+ // Determine path to old backup file
+ const oldBackupFile = this.#_state.lastBackupFileName;
+ const isScheduledBackupsEnabled = lazy.scheduledBackupsPref;
- ChromeUtils.idleDispatch(() => {
+ let { backupPromise, resolve } = Promise.withResolvers();
+ ChromeUtils.idleDispatch(async () => {
lazy.logConsole.debug(
"idleDispatch fired. Attempting to create a backup."
);
+ let oldBackupFilePath;
+ if (await this.#infalliblePathExists(lazy.backupDirPref)) {
+ oldBackupFilePath = PathUtils.join(lazy.backupDirPref, oldBackupFile);
+ }
- this.createBackup(...args).catch(e => {
+ try {
+ if (isScheduledBackupsEnabled) {
+ await this.createBackup({ reason });
+ }
+ } catch (e) {
lazy.logConsole.debug(
`There was an error creating backup on idle dispatch: ${e}`
);
@@ -4162,8 +4176,22 @@ export class BackupService extends EventTarget {
Services.prefs.setBoolPref(DISABLED_ON_IDLE_RETRY_PREF_NAME, true);
Glean.browserBackup.backupThrottled.record();
}
- });
+ } finally {
+ // Now delete the old backup file, if it exists
+ if (deletePreviousBackup && oldBackupFilePath) {
+ lazy.logConsole.log(
+ "Attempting to delete last backup file at ",
+ oldBackupFilePath
+ );
+ await IOUtils.remove(oldBackupFilePath, {
+ ignoreAbsent: true,
+ retryReadonly: true,
+ });
+ resolve();
+ }
+ }
});
+ return backupPromise;
}
/**
diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_regeneration.js b/browser/components/backup/tests/xpcshell/test_BackupService_regeneration.js
@@ -129,16 +129,8 @@ async function expectRegeneration(taskFn, msg) {
let bs = new BackupService();
- // Now we set up some stubs on the BackupService to detect calls to
- // deleteLastBackup and createbackupOnIdleDispatch, which are both called
- // on regeneration.
- let deleteDeferred = Promise.withResolvers();
- sandbox.stub(bs, "deleteLastBackup").callsFake(() => {
- Assert.ok(true, "Saw deleteLastBackup call");
- deleteDeferred.resolve();
- return Promise.resolve();
- });
-
+ // Now we set up a stub on the BackupService to detect calls to
+ // createbackupOnIdleDispatch
let createBackupDeferred = Promise.withResolvers();
sandbox.stub(bs, "createBackupOnIdleDispatch").callsFake(options => {
Assert.ok(true, "Saw createBackupOnIdleDispatch call");
@@ -162,11 +154,6 @@ async function expectRegeneration(taskFn, msg) {
await taskFn();
- let regenerationPromises = [
- deleteDeferred.promise,
- createBackupDeferred.promise,
- ];
-
// We'll wait for 1 second before considering the regeneration a bust.
let timeoutPromise = new Promise((resolve, reject) =>
// eslint-disable-next-line mozilla/no-arbitrary-setTimeout
@@ -176,7 +163,7 @@ async function expectRegeneration(taskFn, msg) {
);
try {
- await Promise.race([Promise.all(regenerationPromises), timeoutPromise]);
+ await Promise.race([createBackupDeferred.promise, timeoutPromise]);
Assert.ok(true, msg);
} catch (e) {
Assert.ok(false, "Timed out waiting for regeneration.");
diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_retryHeuristic.js b/browser/components/backup/tests/xpcshell/test_BackupService_retryHeuristic.js
@@ -81,7 +81,7 @@ add_task(async function test_retry_limit() {
// ensure that there is no error code set
Services.prefs.setIntPref(BACKUP_ERROR_CODE_PREF_NAME, ERRORS.NONE);
- bs.createBackupOnIdleDispatch();
+ bs.createBackupOnIdleDispatch({});
// #backupInProgress is set to true
await bsInProgressStateUpdate(bs, true);
@@ -121,7 +121,7 @@ add_task(async function test_retry_limit() {
// check if it switched to no longer creating backups on idle
const previousCalls = bs.createBackup.callCount;
- bs.createBackupOnIdleDispatch();
+ bs.createBackupOnIdleDispatch({});
// wait a tick for the pref to update
await new Promise(executeSoon);
@@ -148,7 +148,7 @@ add_task(async function test_retry_limit() {
// eslint-disable-next-line mozilla/no-arbitrary-setTimeout
await new Promise(resolve => setTimeout(resolve, 10));
- bs.createBackupOnIdleDispatch();
+ bs.createBackupOnIdleDispatch({});
// #backupInProgress is set to true
await bsInProgressStateUpdate(bs, true);
