commit cb19f3b793ff2512404d77e994779af37d77e4e5
parent cc4d11497f2204648db0516f785617124617fa18
Author: David P <daparks@mozilla.com>
Date: Thu, 2 Oct 2025 22:38:21 +0000
Bug 1985141: Non-recoverable file system errors in snapshot folder eventually stop backups r=sthompson
Non-resolvable file-system failures can prevent us from deleting staging
items in the snapshot folder. This would previously result in a pileup of
them, which can be quite large, wasting an unbounded amount of storage space.
This patch adds browser.backup.max-num-unremovable-staging-items to set the
maximum number of allowed stale staging items. Once we exceed that number, we
stop creating additional backups.
We do this before starting the backup since it will consume storage and, if it
is also prevented from being removed, then we would have added another
unremovable backup. In other words, although stale staging folder/archive
removal already happens once we create the new archive, by then, it is too
late to fix the problem.
Differential Revision: https://phabricator.services.mozilla.com/D263492
Diffstat:
5 files changed, 300 insertions(+), 18 deletions(-)
diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
@@ -3434,6 +3434,11 @@ pref("browser.backup.template.fallback-download.esr", " https://www.firefox.com/
pref("browser.backup.errorCode", 0);
pref("browser.backup.backup-retry-limit", 100);
pref("browser.backup.disabled-on-idle-backup-retry", false);
+// Limit of number of unremovable staging directories and archives that are
+// permitted before backup will stop making additional backups. Unremovable
+// staging directories/archives are ones that the file system prevents us from
+// removing for any reason.
+pref("browser.backup.max-num-unremovable-staging-items", 5);
#ifdef NIGHTLY_BUILD
// Pref to enable the new profiles
diff --git a/browser/components/backup/BackupService.sys.mjs b/browser/components/backup/BackupService.sys.mjs
@@ -34,6 +34,8 @@ const BACKUP_RETRY_LIMIT_PREF_NAME = "browser.backup.backup-retry-limit";
const DISABLED_ON_IDLE_RETRY_PREF_NAME =
"browser.backup.disabled-on-idle-backup-retry";
const BACKUP_DEBUG_INFO_PREF_NAME = "browser.backup.backup-debug-info";
+const MAXIMUM_NUMBER_OF_UNREMOVABLE_STAGING_ITEMS_PREF_NAME =
+ "browser.backup.max-num-unremovable-staging-items";
const SCHEMAS = Object.freeze({
BACKUP_MANIFEST: 1,
@@ -171,6 +173,13 @@ XPCOMUtils.defineLazyPreferenceGetter(
false
);
+XPCOMUtils.defineLazyPreferenceGetter(
+ lazy,
+ "maximumNumberOfUnremovableStagingItems",
+ MAXIMUM_NUMBER_OF_UNREMOVABLE_STAGING_ITEMS_PREF_NAME,
+ 5
+);
+
XPCOMUtils.defineLazyServiceGetter(
lazy,
"idleService",
@@ -1578,7 +1587,9 @@ export class BackupService extends EventTarget {
/**
* Constructs the staging folder for the backup in the passed in backup
- * folder. If a pre-existing staging folder exists, it will be cleared out.
+ * folder. If the backup (snapshots) folder isn't empty, it will be cleared
+ * out. If that process fails to remove more than
+ * lazy.maximumNumberOfUnremovableStagingItems then the backup is aborted.
*
* @param {string} backupDirPath
* The path to the backup folder.
@@ -1586,19 +1597,71 @@ export class BackupService extends EventTarget {
* The path to the empty staging folder.
*/
async #prepareStagingFolder(backupDirPath) {
- let stagingPath = PathUtils.join(backupDirPath, "staging");
- lazy.logConsole.debug("Checking for pre-existing staging folder");
- if (await IOUtils.exists(stagingPath)) {
- // A pre-existing staging folder exists. A previous backup attempt must
- // have failed or been interrupted. We'll clear it out.
- lazy.logConsole.warn("A pre-existing staging folder exists. Clearing.");
- await IOUtils.remove(stagingPath, {
- recursive: true,
- retryReadonly: true,
- });
+ lazy.logConsole.debug(`Clearing snapshot folder ${backupDirPath}`);
+ let numUnremovableStagingItems = 0;
+ let folder = await IOUtils.getFile(backupDirPath);
+ let folderEntries = folder.directoryEntries;
+ if (folderEntries) {
+ let unremovableContents = [];
+ for (let folderItem of folderEntries) {
+ try {
+ lazy.logConsole.debug(`Removing ${folderItem.path}`);
+ await IOUtils.remove(folderItem, {
+ recursive: true,
+ retryReadonly: true,
+ });
+ } catch (e) {
+ lazy.logConsole.warn(
+ `Failed to remove stale snapshot item ${folderItem.path}. Exception: ${e}`
+ );
+ // Whatever the problem was with removing the snapshot dir contents
+ // (presumably a staging dir or archive), keep going until
+ // maximumNumberOfUnremovableStagingItems + 1 have failed to be
+ // removed, at which point we abandon the backup, in order to avoid
+ // filling drive space.
+ numUnremovableStagingItems++;
+ unremovableContents.push(folderItem.path);
+ if (
+ numUnremovableStagingItems >
+ lazy.maximumNumberOfUnremovableStagingItems
+ ) {
+ let error = new BackupError(
+ `Failed to remove ${numUnremovableStagingItems} items from ${backupDirPath}`,
+ ERRORS.FILE_SYSTEM_ERROR
+ );
+ error.stack = e.stack;
+ error.unremovableContents = unremovableContents;
+ throw error;
+ }
+ }
+ }
}
- await IOUtils.makeDirectory(stagingPath);
+ lazy.logConsole.debug(
+ `${numUnremovableStagingItems} unremovable staging items found. Proceeding with backup. Determining staging folder.`
+ );
+ let stagingPath;
+ for (let i = 0; i < lazy.maximumNumberOfUnremovableStagingItems + 1; i++) {
+ // Attempt to use "staging-i" as the name of the staging folder.
+ let potentialStagingPath = PathUtils.join(backupDirPath, "staging-" + i);
+ if (!(await IOUtils.exists(potentialStagingPath))) {
+ stagingPath = potentialStagingPath;
+ await IOUtils.makeDirectory(stagingPath);
+ break;
+ }
+ }
+
+ if (!stagingPath) {
+ // Should be impossible. We determined there were no more than
+ // maximumNumberOfUnremovableStagingItems items but we then found
+ // maximumNumberOfUnremovableStagingItems + 1 staging folders.
+ throw new BackupError(
+ `Internal error in attempt to create staging folder`,
+ ERRORS.FILE_SYSTEM_ERROR
+ );
+ }
+
+ lazy.logConsole.debug(`Staging folder ${stagingPath} is prepared`);
return stagingPath;
}
@@ -2453,10 +2516,19 @@ export class BackupService extends EventTarget {
existingBackupPath !== renamedBackupPath &&
existingBackupPath.match(expectedFormatRegex)
) {
- await IOUtils.remove(existingBackupPath, {
- recursive: true,
- retryReadonly: true,
- });
+ try {
+ // If any copied source files were read-only then we need to remove
+ // read-only status from them to delete the staging folder.
+ await IOUtils.remove(existingBackupPath, {
+ recursive: true,
+ retryReadonly: true,
+ });
+ } catch (e) {
+ // Ignore any failures in removing staging items.
+ lazy.logConsole.debug(
+ `Failed to remove staging item ${existingBackupPath}. Exception ${e}`
+ );
+ }
}
}
return renamedBackupPath;
diff --git a/browser/components/backup/tests/xpcshell/data/test_keep_file_open.worker.js b/browser/components/backup/tests/xpcshell/data/test_keep_file_open.worker.js
@@ -0,0 +1,43 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/. */
+
+/* import-globals-from /toolkit/components/workerloader/require.js */
+importScripts("resource://gre/modules/workers/require.js");
+
+const PromiseWorker = require("resource://gre/modules/workers/PromiseWorker.js");
+
+/**
+ * For keeping a file open in a worker while a test runs.
+ */
+class OpenFileWorker extends PromiseWorker.AbstractWorker {
+ constructor() {
+ super();
+
+ this._file = null;
+ }
+
+ postMessage(message, ...transfers) {
+ self.postMessage(message, transfers);
+ }
+
+ dispatch(method, args) {
+ return this[method](...args);
+ }
+
+ open(path) {
+ this._file = IOUtils.openFileForSyncReading(path);
+ }
+
+ close() {
+ if (this._file) {
+ this._file.close();
+ }
+ }
+}
+
+const worker = new OpenFileWorker();
+
+self.addEventListener("message", msg => worker.handleMessage(msg));
+self.addEventListener("unhandledrejection", err => {
+ throw err.reason;
+});
diff --git a/browser/components/backup/tests/xpcshell/test_BackupService.js b/browser/components/backup/tests/xpcshell/test_BackupService.js
@@ -6,6 +6,9 @@ https://creativecommons.org/publicdomain/zero/1.0/ */
const { AppConstants } = ChromeUtils.importESModule(
"resource://gre/modules/AppConstants.sys.mjs"
);
+const { BasePromiseWorker } = ChromeUtils.importESModule(
+ "resource://gre/modules/PromiseWorker.sys.mjs"
+);
const { JsonSchema } = ChromeUtils.importESModule(
"resource://gre/modules/JsonSchema.sys.mjs"
);
@@ -667,6 +670,162 @@ add_task(
);
/**
+ * Creates a unique file in the given folder and tells a worker to keep it
+ * open until we post a close message. Checks that the folder is not
+ * removable as a result.
+ *
+ * @param {string} folderpath
+ * @returns {object} {{ path: string, worker: OpenFileWorker }}
+ */
+async function openUniqueFileInFolder(folderpath) {
+ let testFile = await IOUtils.createUniqueFile(folderpath, "openfile");
+ await IOUtils.writeUTF8(testFile, "");
+ Assert.ok(
+ await IOUtils.exists(testFile),
+ testFile + " should have been created"
+ );
+ // Use a worker to keep the testFile open.
+ const worker = new BasePromiseWorker(
+ "resource://test/data/test_keep_file_open.worker.js"
+ );
+ await worker.post("open", [testFile]);
+
+ await Assert.rejects(
+ IOUtils.remove(folderpath),
+ /NS_ERROR_FILE_DIR_NOT_EMPTY/,
+ "attempt to remove folder threw an exception"
+ );
+ Assert.ok(await IOUtils.exists(folderpath), "folder is not removable");
+ return { path: testFile, worker };
+}
+
+/**
+ * Stop the worker returned from openUniqueFileInFolder and close the file.
+ *
+ * @param {object} worker The worker returned by openUniqueFileInFolder
+ */
+async function closeTestFile(worker) {
+ await worker.post("close", []);
+}
+
+/**
+ * Run a backup and check that it either succeeded with a response or failed
+ * and returned null.
+ *
+ * @param {object} backupService Instance of BackupService
+ * @param {string} profilePath Full path to profile folder
+ * @param {boolean} shouldSucceed Whether to expect success or failure
+ */
+async function checkBackup(backupService, profilePath, shouldSucceed) {
+ if (shouldSucceed) {
+ await backupService.createBackup({ profilePath }).then(result => {
+ Assert.ok(true, "createBackup did not throw an exception");
+ Assert.notEqual(
+ result,
+ null,
+ `createBackup should not have returned null`
+ );
+ });
+ await backupService.deleteLastBackup();
+ return;
+ }
+
+ await Assert.rejects(
+ backupService.createBackup({ profilePath }),
+ /Failed to remove/,
+ "createBackup threw correct exception"
+ );
+}
+
+/**
+ * Checks that browser.backup.max-num-unremovable-staging-items allows backups
+ * to succeed if the snapshots folder contains no more than that many
+ * unremovable items, and that it fails if there are more than that.
+ *
+ * @param {number} unremovableItemsLimit Max number of unremovable items that
+ * backups can succeed with.
+ */
+async function checkBackupWithUnremovableItems(unremovableItemsLimit) {
+ Services.prefs.setIntPref(
+ "browser.backup.max-num-unremovable-staging-items",
+ unremovableItemsLimit
+ );
+ registerCleanupFunction(() =>
+ Services.prefs.clearUserPref(
+ "browser.backup.max-num-unremovable-staging-items"
+ )
+ );
+
+ let sandbox = sinon.createSandbox();
+
+ const TEST_UID = "ThisIsMyTestUID";
+ const TEST_EMAIL = "foxy@mozilla.org";
+
+ sandbox.stub(UIState, "get").returns({
+ status: UIState.STATUS_SIGNED_IN,
+ uid: TEST_UID,
+ email: TEST_EMAIL,
+ });
+ const backupService = new BackupService({});
+
+ let profilePath = await IOUtils.createUniqueDirectory(
+ PathUtils.tempDir,
+ "profileDir"
+ );
+ let snapshotsFolder = PathUtils.join(
+ profilePath,
+ BackupService.PROFILE_FOLDER_NAME,
+ BackupService.SNAPSHOTS_FOLDER_NAME
+ );
+
+ let openFileWorkers = [];
+ try {
+ for (let i = 0; i < unremovableItemsLimit + 1; i++) {
+ info(`Performing backup #${i}`);
+ await checkBackup(backupService, profilePath, true /* shouldSucceed */);
+
+ // Create and open a file so that the snapshots folder cannot be
+ // emptied.
+ openFileWorkers.push(await openUniqueFileInFolder(snapshotsFolder));
+ }
+
+ // We are now over the unremovableItemsLimit.
+ info(`Performing backup that should fail`);
+ await checkBackup(backupService, profilePath, false /* shouldSucceed */);
+ } finally {
+ await Promise.all(
+ openFileWorkers.map(async ofw => await closeTestFile(ofw.worker))
+ );
+ await Promise.all(
+ openFileWorkers.map(async ofw => await IOUtils.remove(ofw.path))
+ );
+ await IOUtils.remove(profilePath, { recursive: true });
+ sandbox.restore();
+ }
+}
+
+/**
+ * Tests that any non-read-only file deletion errors do not prevent backups
+ * until the browser.backup.max-num-unremovable-staging-items limit has been
+ * reached.
+ */
+add_task(
+ async function test_createBackup_robustToNonReadonlyFileSystemErrorsAllowOneNonReadonly() {
+ await checkBackupWithUnremovableItems(1);
+ }
+);
+
+/**
+ * Tests that browser.backup.max-num-unremovable-staging-items works for value
+ * 0.
+ */
+add_task(
+ async function test_createBackup_robustToNonReadonlyFileSystemErrors() {
+ await checkBackupWithUnremovableItems(0);
+ }
+);
+
+/**
* Tests that if there's a post-recovery.json file in the profile directory
* when checkForPostRecovery() is called, that it is processed, and the
* postRecovery methods on the associated BackupResources are called with the
@@ -787,6 +946,6 @@ add_task(async function test__deleteLastBackup_file_does_not_exist() {
// Now delete the file ourselves before we call deleteLastBackup,
// so that it's missing from the disk.
await testDeleteLastBackupHelper(async lastBackupFilePath => {
- await IOUtils.remove(lastBackupFilePath);
+ await maybeRemovePath(lastBackupFilePath);
});
});
diff --git a/browser/components/backup/tests/xpcshell/xpcshell.toml b/browser/components/backup/tests/xpcshell/xpcshell.toml
@@ -20,7 +20,10 @@ prefs = [
support-files = ["data/test_xulstore.json"]
["test_BackupService.js"]
-support-files = ["data/test_archive.template.html"]
+support-files = [
+ "data/test_archive.template.html",
+ "data/test_keep_file_open.worker.js"
+]
["test_BackupService_archive.js"]
support-files = [
