commit 711e72297a67bb7962e3159f5d7d98866c7c11c9
parent 73282cdccc5342ee5392c2b79859fd0c683dd09c
Author: Chris DuPuis <cdupuis@mozilla.com>
Date: Fri, 31 Oct 2025 21:32:22 +0000
Bug 1997371 - createBackup should not create fallback folders if the user's selected folder does not exist r=hsohaney,dmcintosh
When creating a backup, the method resolveArchiveDestFolderPath is used to find (and create if necessary) the "Restore Firefox" folder inside the user's selected backup folder.
However, based on an older set of requirements, this method has a fallback to create a "Restore Firefox" in a default location, different than what the user specified, if the creation inside the user's selected folder fails.
Our current requirements are to only create backups in a user-specified location, so this fallback folder creation should be removed.
Differential Revision: https://phabricator.services.mozilla.com/D270730
Diffstat:
3 files changed, 6 insertions(+), 305 deletions(-)
diff --git a/browser/components/backup/BackupService.sys.mjs b/browser/components/backup/BackupService.sys.mjs
@@ -1173,28 +1173,14 @@ export class BackupService extends EventTarget {
}
/**
- * Attempts to find the right folder to write the single-file archive to, and
- * if it does not exist, to create it.
- *
- * If the configured destination's parent folder does not exist and cannot
- * be recreated, we will fall back to the `defaultParentDirPath`. If
- * `defaultParentDirPath` happens to not exist or cannot be created, we will
- * fall back to the home directory. If _that_ folder does not exist and cannot
- * be recreated, this method will reject.
+ * Attempts to find the right folder to write the single-file archive to, creating
+ * it if it does not exist yet.
*
* @param {string} configuredDestFolderPath
* The currently configured destination folder for the archive.
* @returns {Promise<string, Error>}
*/
async resolveArchiveDestFolderPath(configuredDestFolderPath) {
- lazy.logConsole.log(
- "Resolving configured archive destination folder: ",
- configuredDestFolderPath
- );
-
- // Try to create the configured folder ancestry. If that fails, we clear
- // configuredDestFolderPath so that we can try the fallback paths, as
- // if the folder was never set.
try {
await IOUtils.makeDirectory(configuredDestFolderPath, {
createAncestors: true,
@@ -1203,43 +1189,6 @@ export class BackupService extends EventTarget {
return configuredDestFolderPath;
} catch (e) {
lazy.logConsole.warn("Could not create configured destination path: ", e);
- }
- lazy.logConsole.warn(
- "The destination directory was invalid. Attempting to fall back to " +
- "default parent folder: ",
- BackupService.DEFAULT_PARENT_DIR_PATH
- );
- let fallbackFolderPath = PathUtils.join(
- BackupService.DEFAULT_PARENT_DIR_PATH,
- BackupService.BACKUP_DIR_NAME
- );
- try {
- await IOUtils.makeDirectory(fallbackFolderPath, {
- createAncestors: true,
- ignoreExisting: true,
- });
- return fallbackFolderPath;
- } catch (e) {
- lazy.logConsole.warn("Could not create fallback destination path: ", e);
- }
-
- let homeDirPath = PathUtils.join(
- Services.dirsvc.get("Home", Ci.nsIFile).path,
- BackupService.BACKUP_DIR_NAME
- );
- lazy.logConsole.warn(
- "The destination directory was invalid. Attempting to fall back to " +
- "Home folder: ",
- homeDirPath
- );
- try {
- await IOUtils.makeDirectory(homeDirPath, {
- createAncestors: true,
- ignoreExisting: true,
- });
- return homeDirPath;
- } catch (e) {
- lazy.logConsole.warn("Could not create Home destination path: ", e);
throw new BackupError(
"Could not resolve to a writable destination folder path.",
ERRORS.FILE_SYSTEM_ERROR
@@ -1248,47 +1197,6 @@ export class BackupService extends EventTarget {
}
/**
- * Attempts to resolve an existing folder path to use for archives,
- * following the same fallback order as `resolveArchiveDestFolderPath`.
- *
- * Order of resolution:
- * 1. Configured destination folder
- * 2. Default parent folder
- * 3. Home directory
- *
- * @param {string} configuredDestFolderPath
- * @returns {Promise<string, Error>}
- */
- async resolveExistingArchiveDestFolderPath(configuredDestFolderPath) {
- if (
- configuredDestFolderPath &&
- (await IOUtils.exists(configuredDestFolderPath))
- ) {
- return configuredDestFolderPath;
- }
-
- let fallbackFolderPath = PathUtils.join(
- BackupService.DEFAULT_PARENT_DIR_PATH,
- BackupService.BACKUP_DIR_NAME
- );
- if (await IOUtils.exists(fallbackFolderPath)) {
- return fallbackFolderPath;
- }
-
- let homeDirPath = PathUtils.join(
- Services.dirsvc.get("Home", Ci.nsIFile).path,
- BackupService.BACKUP_DIR_NAME
- );
- if (await IOUtils.exists(homeDirPath)) {
- return homeDirPath;
- }
-
- throw new Error("Could not resolve an existing destination folder path.", {
- cause: ERRORS.FILE_SYSTEM_ERROR,
- });
- }
-
- /**
* Computes the appropriate link to place in the single-file archive for
* downloading a version of this application for the same update channel.
*
@@ -4259,19 +4167,9 @@ export class BackupService extends EventTarget {
}
try {
- // Check if the default folder exists
- let archiveDestPath = await this.resolveExistingArchiveDestFolderPath(
- this.#_state.backupDirPath
- );
-
- let dirExists = await this.#infalliblePathExists(archiveDestPath);
- if (!dirExists) {
- return {
- multipleBackupsFound: false,
- };
- }
-
- let files = await IOUtils.getChildren(archiveDestPath);
+ let files = await IOUtils.getChildren(this.#_state.backupDirPath, {
+ ignoreAbsent: true,
+ });
// filtering is an O(N) operation, we can return early if there's too many files
// in this folder to filter to avoid a performance bottleneck
if (speedUpHeuristic && files && files.length > 1000) {
diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_resolveArchiveDestFolderPath.js b/browser/components/backup/tests/xpcshell/test_BackupService_resolveArchiveDestFolderPath.js
@@ -116,198 +116,3 @@ add_task(async function test_find_folder() {
);
await IOUtils.remove(PARENT_FOLDER, { recursive: true });
});
-
-/**
- * Tests that we fall back to the DEFAULT_PARENT_DIR_PATH folder if the
- * configured path cannot be written to. This might happen if, for example, the
- * configured destination is a removable drive that has been removed.
- */
-add_task(async function test_fallback_to_default() {
- if (AppConstants.platform == "win") {
- todo_check_true(
- false,
- "Programmatically setting folder permissions does not work on " +
- "Windows, so this test is skipped."
- );
- return;
- }
-
- const UNWRITABLE_PARENT = PathUtils.join(gTestRoot, "UnwritableParent");
- await IOUtils.makeDirectory(UNWRITABLE_PARENT);
- // Make the folder read-only across the board. 0o444 is the chmod numeric code
- // for that.
- await IOUtils.setPermissions(UNWRITABLE_PARENT, 0o444);
-
- const CONFIGURED_FOLDER = PathUtils.join(
- UNWRITABLE_PARENT,
- "ImpossibleChild"
- );
- Assert.ok(
- !(await IOUtils.exists(CONFIGURED_FOLDER)),
- "Configured folder should not exist."
- );
-
- const DEFAULT_FOLDER = PathUtils.join(gTestRoot, "FakeDocuments");
- await IOUtils.makeDirectory(DEFAULT_FOLDER);
-
- let bs = new BackupService();
- // Stub out the DEFAULT_PARENT_DIR_PATH into a folder path we control in this
- // test, so that we don't pollute this machine's actual Documents folder.
- let sandbox = sinon.createSandbox();
- sandbox
- .stub(BackupService, "DEFAULT_PARENT_DIR_PATH")
- .get(() => DEFAULT_FOLDER);
-
- const CONFIGURED_DESTINATION_PATH = PathUtils.join(
- CONFIGURED_FOLDER,
- BackupService.BACKUP_DIR_NAME
- );
- const EXPECTED_DESTINATION_PATH = PathUtils.join(
- DEFAULT_FOLDER,
- BackupService.BACKUP_DIR_NAME
- );
- let path = await bs.resolveArchiveDestFolderPath(CONFIGURED_DESTINATION_PATH);
- Assert.equal(
- path,
- EXPECTED_DESTINATION_PATH,
- "Got back the expected folder path."
- );
- Assert.ok(await IOUtils.exists(path), "The destination folder was created.");
-
- await IOUtils.remove(DEFAULT_FOLDER, { recursive: true });
- await IOUtils.remove(UNWRITABLE_PARENT, { recursive: true });
- sandbox.restore();
-});
-
-/**
- * Tests that we fall back to the Home folder if the configured path AND the
- * DEFAULT_PARENT_DIR_PATH cannot be written to.
- */
-add_task(async function test_fallback_to_home() {
- if (AppConstants.platform == "win") {
- todo_check_true(
- false,
- "Programmatically setting folder permissions does not work on " +
- "Windows, so this test is skipped."
- );
- return;
- }
-
- const UNWRITABLE_PARENT = PathUtils.join(gTestRoot, "UnwritableParent");
- await IOUtils.makeDirectory(UNWRITABLE_PARENT);
- // Make the folder read-only across the board. 0o444 is the chmod numeric code
- // for that.
- await IOUtils.setPermissions(UNWRITABLE_PARENT, 0o444);
-
- const CONFIGURED_FOLDER = PathUtils.join(
- UNWRITABLE_PARENT,
- "ImpossibleChild"
- );
- Assert.ok(
- !(await IOUtils.exists(CONFIGURED_FOLDER)),
- "Configured folder should not exist."
- );
-
- const DEFAULT_FOLDER = PathUtils.join(gTestRoot, "FakeDocuments");
- await IOUtils.makeDirectory(DEFAULT_FOLDER);
- await IOUtils.setPermissions(DEFAULT_FOLDER, 0o444);
-
- let bs = new BackupService();
- // Stub out the DEFAULT_PARENT_DIR_PATH into a folder path we control in this
- // test, so that we don't pollute this machine's actual Documents folder.
- let sandbox = sinon.createSandbox();
- sandbox
- .stub(BackupService, "DEFAULT_PARENT_DIR_PATH")
- .get(() => DEFAULT_FOLDER);
-
- const CONFIGURED_DESTINATION_PATH = PathUtils.join(
- CONFIGURED_FOLDER,
- BackupService.BACKUP_DIR_NAME
- );
- const EXPECTED_DESTINATION_PATH = PathUtils.join(
- gFakeHomePath,
- BackupService.BACKUP_DIR_NAME
- );
- let path = await bs.resolveArchiveDestFolderPath(CONFIGURED_DESTINATION_PATH);
- Assert.equal(
- path,
- EXPECTED_DESTINATION_PATH,
- "Got back the expected folder path."
- );
- Assert.ok(await IOUtils.exists(path), "The destination folder was created.");
-
- await IOUtils.remove(EXPECTED_DESTINATION_PATH, { recursive: true });
- await IOUtils.remove(DEFAULT_FOLDER, { recursive: true });
- await IOUtils.remove(UNWRITABLE_PARENT, { recursive: true });
- sandbox.restore();
-});
-
-/**
- * Tests that if we fall back to the $HOME folder and some how that doesn't
- * exist, then we reject.
- */
-add_task(async function test_fallback_to_home_fail() {
- if (AppConstants.platform == "win") {
- todo_check_true(
- false,
- "Programmatically setting folder permissions does not work on " +
- "Windows, so this test is skipped."
- );
- return;
- }
-
- const UNWRITABLE_PARENT = PathUtils.join(gTestRoot, "UnwritableParent");
- await IOUtils.makeDirectory(UNWRITABLE_PARENT);
- // Make the folder read-only across the board. 0o444 is the chmod numeric code
- // for that.
- await IOUtils.setPermissions(UNWRITABLE_PARENT, 0o444);
-
- const CONFIGURED_FOLDER = PathUtils.join(
- UNWRITABLE_PARENT,
- "ImpossibleChild"
- );
- Assert.ok(
- !(await IOUtils.exists(CONFIGURED_FOLDER)),
- "Configured folder should not exist."
- );
-
- const DEFAULT_FOLDER = PathUtils.join(gTestRoot, "FakeDocuments");
- await IOUtils.makeDirectory(DEFAULT_FOLDER);
- await IOUtils.setPermissions(DEFAULT_FOLDER, 0o444);
-
- const UNWRITABLE_HOME_FOLDER = PathUtils.join(gTestRoot, "UnwritableHome");
- await IOUtils.makeDirectory(UNWRITABLE_HOME_FOLDER);
- await IOUtils.setPermissions(UNWRITABLE_HOME_FOLDER, 0o444);
-
- let unwritableHomeFolderFile = await IOUtils.getFile(UNWRITABLE_HOME_FOLDER);
- let dirsvc = Services.dirsvc.QueryInterface(Ci.nsIProperties);
- dirsvc.undefine(HOME_KEY);
- dirsvc.set(HOME_KEY, unwritableHomeFolderFile);
-
- // Stub out the DEFAULT_PARENT_DIR_PATH into a folder path we control in this
- // test, so that we don't pollute this machine's actual Documents folder.
- let sandbox = sinon.createSandbox();
- sandbox
- .stub(BackupService, "DEFAULT_PARENT_DIR_PATH")
- .get(() => DEFAULT_FOLDER);
-
- let bs = new BackupService();
-
- const CONFIGURED_DESTINATION_PATH = PathUtils.join(
- CONFIGURED_FOLDER,
- BackupService.BACKUP_DIR_NAME
- );
-
- await Assert.rejects(
- bs.resolveArchiveDestFolderPath(CONFIGURED_DESTINATION_PATH),
- /Could not resolve/
- );
-
- sandbox.restore();
- await IOUtils.remove(UNWRITABLE_HOME_FOLDER, { recursive: true });
- await IOUtils.remove(DEFAULT_FOLDER, { recursive: true });
- await IOUtils.remove(UNWRITABLE_PARENT, { recursive: true });
-
- dirsvc.undefine(HOME_KEY);
- dirsvc.set(HOME_KEY, gFakeHomeFile);
-});
diff --git a/browser/components/backup/tests/xpcshell/test_backupService_findBackupsInWellKnownLocations.js b/browser/components/backup/tests/xpcshell/test_backupService_findBackupsInWellKnownLocations.js
@@ -21,13 +21,11 @@ add_task(
});
return p;
}
+ Services.prefs.setStringPref("browser.backup.location", BACKUP_DIR);
// Create the service and force it to search our temp folder instead of the real default
let bs = new BackupService();
let sandbox = sinon.createSandbox();
- sandbox
- .stub(bs, "resolveExistingArchiveDestFolderPath")
- .callsFake(async _configured => BACKUP_DIR);
// getBackupFileInfo should return without throwing to simulate
// what happens when a valid backup file's validity is checked
