commit c35177695e41167b01ce65e6c2198e0fb31c5781
parent 5c18c056ee6ed0f8f589d20f11e0097e2fe75241
Author: David P <daparks@mozilla.com>
Date: Thu, 2 Oct 2025 22:38:21 +0000
Bug 1985141: Remove read-only status from staging files when deleting r=xpcom-reviewers,sthompson,beth
Staging areas can resist deletion if a file in them is read-only (say, because
the original file was read-only and the staged file is a direct copy). This
attempts to remove read-only status from any directory contents (like
`chmod -R`) and then reissues the delete when the first delete attempt fails.
Differential Revision: https://phabricator.services.mozilla.com/D263490
Diffstat:
5 files changed, 156 insertions(+), 26 deletions(-)
diff --git a/browser/components/backup/BackupService.sys.mjs b/browser/components/backup/BackupService.sys.mjs
@@ -1393,7 +1393,12 @@ export class BackupService extends EventTarget {
renamedStagingPath,
backupDirPath
).finally(async () => {
- await IOUtils.remove(renamedStagingPath, { recursive: true });
+ // retryReadonly is needed in case there were read only files in
+ // the profile.
+ await IOUtils.remove(renamedStagingPath, {
+ recursive: true,
+ retryReadonly: true,
+ });
});
currentStep = STEPS.CREATE_BACKUP_CREATE_ARCHIVE;
@@ -2442,6 +2447,7 @@ export class BackupService extends EventTarget {
) {
await IOUtils.remove(existingBackupPath, {
recursive: true,
+ retryReadonly: true,
});
}
}
diff --git a/browser/components/backup/tests/xpcshell/test_BackupService.js b/browser/components/backup/tests/xpcshell/test_BackupService.js
@@ -515,12 +515,48 @@ add_task(async function test_createBackup_signed_in() {
});
/**
- * Tests that any internal file system errors in BackupService.createBackup
- * do not bubble up any errors.
+ * Makes a folder readonly. Windows does not support read-only folders, so
+ * this creates a file inside the folder and makes that read-only.
+ *
+ * @param {string} folderpath Full path to folder to make read-only.
+ * @param {boolean} isReadonly Whether to set or clear read-only status.
+ */
+async function makeFolderReadonly(folderpath, isReadonly) {
+ if (AppConstants.platform !== "win") {
+ await IOUtils.setPermissions(folderpath, isReadonly ? 0o444 : 0o666);
+ let folder = await IOUtils.getFile(folderpath);
+ Assert.equal(
+ folder.isWritable(),
+ !isReadonly,
+ `folder is ${isReadonly ? "" : "not "}read-only`
+ );
+ } else if (isReadonly) {
+ // Permissions flags like 0o444 are not usually respected on Windows but in
+ // the case of creating a unique file, the read-only status is. See
+ // OpenFile in nsLocalFileWin.cpp.
+ let tempFilename = await IOUtils.createUniqueFile(
+ folderpath,
+ "readonlyfile",
+ 0o444
+ );
+ let file = await IOUtils.getFile(tempFilename);
+ Assert.equal(file.isWritable(), false, "file in folder is read-only");
+ } else {
+ // Recursively set any folder contents to be writeable.
+ let attrs = await IOUtils.getWindowsAttributes(folderpath);
+ attrs.readonly = false;
+ await IOUtils.setWindowsAttributes(folderpath, attrs, true /* recursive */);
+ }
+}
+
+/**
+ * Tests that read-only files in BackupService.createBackup cause backup
+ * failure (createBackup returns null) and does not bubble up any errors.
*/
add_task(
{
- // Bug 1905724 - Need to find a way to deny write access to backup directory on Windows
+ // We override read-only on Windows -- see
+ // test_createBackup_override_readonly below.
skip_if: () => AppConstants.platform == "win",
},
async function test_createBackup_robustToFileSystemErrors() {
@@ -544,9 +580,9 @@ add_task(
// won't be able to make writes
let inaccessibleProfilePath = await IOUtils.createUniqueDirectory(
PathUtils.tempDir,
- "createBackupErrorInaccessible"
+ "createBackupErrorReadonly"
);
- IOUtils.setPermissions(inaccessibleProfilePath, 0o444);
+ await makeFolderReadonly(inaccessibleProfilePath, true);
const bs = new BackupService({});
@@ -569,7 +605,62 @@ add_task(
// Failure bubbles up an error for handling by the caller
})
.finally(async () => {
+ await makeFolderReadonly(inaccessibleProfilePath, false);
+ await IOUtils.remove(inaccessibleProfilePath, { recursive: true });
+ sandbox.restore();
+ });
+ }
+);
+
+/**
+ * Tests that BackupService.createBackup can override simple read-only status
+ * when handling staging files. That currently only works on Windows.
+ */
+add_task(
+ {
+ skip_if: () => AppConstants.platform !== "win",
+ },
+ async function test_createBackup_override_readonly() {
+ 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,
+ });
+
+ // Create a fake profile folder that contains a read-only file. We do this
+ // because Windows does not respect read-only status on folders. The
+ // file's read-only status will make the folder un(re)movable.
+ let inaccessibleProfilePath = await IOUtils.createUniqueDirectory(
+ PathUtils.tempDir,
+ "createBackupErrorReadonly"
+ );
+ await makeFolderReadonly(inaccessibleProfilePath, true);
+ await Assert.rejects(
+ IOUtils.remove(inaccessibleProfilePath),
+ /Could not remove/,
+ "folder is not removable"
+ );
+
+ const bs = new BackupService({});
+
+ await bs
+ .createBackup({ profilePath: inaccessibleProfilePath })
+ .then(result => {
+ Assert.notEqual(result, null, "Should not return null on success");
+ })
+ .catch(e => {
+ console.error(e);
+ Assert.ok(false, "Should not have bubbled up an error");
+ })
+ .finally(async () => {
+ await makeFolderReadonly(inaccessibleProfilePath, false);
await IOUtils.remove(inaccessibleProfilePath, { recursive: true });
+ await bs.deleteLastBackup();
sandbox.restore();
});
}
diff --git a/dom/chrome-webidl/IOUtils.webidl b/dom/chrome-webidl/IOUtils.webidl
@@ -312,11 +312,15 @@ namespace IOUtils {
* @param attrs The attributes to set. Attributes will only be set if they are
* |true| or |false| (i.e., |undefined| attributes are not
* changed).
+ * @param recursive Whether or not to apply the changes to folder contents
+ * recursively.
*
* @return A promise that resolves is the attributes were set successfully.
*/
[NewObject]
- Promise<undefined> setWindowsAttributes(DOMString path, optional WindowsFileAttributes attrs = {});
+ Promise<undefined> setWindowsAttributes(DOMString path,
+ optional WindowsFileAttributes attrs = {},
+ optional boolean recursive = false);
#elif defined(XP_MACOSX)
/**
* Return whether or not the file has a specific extended attribute.
diff --git a/xpcom/ioutils/IOUtils.cpp b/xpcom/ioutils/IOUtils.cpp
@@ -1025,7 +1025,7 @@ already_AddRefed<Promise> IOUtils::GetWindowsAttributes(GlobalObject& aGlobal,
/* static */
already_AddRefed<Promise> IOUtils::SetWindowsAttributes(
GlobalObject& aGlobal, const nsAString& aPath,
- const WindowsFileAttributes& aAttrs, ErrorResult& aError) {
+ const WindowsFileAttributes& aAttrs, bool aRecursive, ErrorResult& aError) {
return WithPromiseAndState(
aGlobal, aError, [&](Promise* promise, auto& state) {
nsCOMPtr<nsIFile> file = new nsLocalFile();
@@ -1063,8 +1063,9 @@ already_AddRefed<Promise> IOUtils::SetWindowsAttributes(
DispatchAndResolve<Ok>(
state->mEventQueue, promise,
- [file = std::move(file), setAttrs, clearAttrs]() {
- return SetWindowsAttributesSync(file, setAttrs, clearAttrs);
+ [file = std::move(file), setAttrs, clearAttrs, aRecursive]() {
+ return SetWindowsAttributesSync(file, setAttrs, clearAttrs,
+ aRecursive);
});
});
}
@@ -1763,17 +1764,14 @@ Result<Ok, IOUtils::IOError> IOUtils::RemoveSync(nsIFile* aFile,
return Err(IOError(rv, "Could not remove `%s': file does not exist",
aFile->HumanReadablePath().get()));
}
- if (rv == NS_ERROR_FILE_DIR_NOT_EMPTY) {
- return Err(IOError(rv,
- "Could not remove `%s': the directory is not empty",
- aFile->HumanReadablePath().get()));
- }
#ifdef XP_WIN
-
- if (rv == NS_ERROR_FILE_ACCESS_DENIED && aRetryReadonly) {
- if (auto result =
- SetWindowsAttributesSync(aFile, 0, FILE_ATTRIBUTE_READONLY);
+ // If aRetryReadonly && aRecursive then we will try recursively removing
+ // read-only status and then delete again.
+ if (aRetryReadonly && (rv == NS_ERROR_FILE_ACCESS_DENIED ||
+ (rv == NS_ERROR_FILE_DIR_NOT_EMPTY && aRecursive))) {
+ if (auto result = SetWindowsAttributesSync(
+ aFile, 0, FILE_ATTRIBUTE_READONLY, aRecursive);
result.isErr()) {
return Err(IOError::WithCause(
result.unwrapErr(),
@@ -1783,9 +1781,14 @@ Result<Ok, IOUtils::IOError> IOUtils::RemoveSync(nsIFile* aFile,
return RemoveSync(aFile, aIgnoreAbsent, aRecursive,
/* aRetryReadonly = */ false);
}
-
#endif
+ if (rv == NS_ERROR_FILE_DIR_NOT_EMPTY) {
+ return Err(IOError(rv,
+ "Could not remove `%s': the directory is not empty",
+ aFile->HumanReadablePath().get()));
+ }
+
return Err(
IOError(rv, "Could not remove `%s'", aFile->HumanReadablePath().get()));
}
@@ -2197,18 +2200,39 @@ Result<uint32_t, IOUtils::IOError> IOUtils::GetWindowsAttributesSync(
}
Result<Ok, IOUtils::IOError> IOUtils::SetWindowsAttributesSync(
- nsIFile* aFile, const uint32_t aSetAttrs, const uint32_t aClearAttrs) {
+ nsIFile* aFile, const uint32_t aSetAttrs, const uint32_t aClearAttrs,
+ bool aRecursive) {
MOZ_ASSERT(!NS_IsMainThread());
nsCOMPtr<nsILocalFileWin> file = do_QueryInterface(aFile);
MOZ_ASSERT(file);
- if (nsresult rv = file->SetWindowsFileAttributes(aSetAttrs, aClearAttrs);
+ nsresult rv;
+ if (rv = file->SetWindowsFileAttributes(aSetAttrs, aClearAttrs);
NS_FAILED(rv)) {
return Err(IOError(rv, "Could not set Windows file attributes for `%s'",
aFile->HumanReadablePath().get()));
}
+ if (!aRecursive) {
+ return Ok{};
+ }
+
+ auto fileInfo = MOZ_TRY(StatSync(aFile));
+ if (fileInfo.mType != FileType::Directory) {
+ return Ok{};
+ }
+
+ auto entries = MOZ_TRY(GetChildrenSync(aFile, /* ignoreAbsent = */ false));
+ for (const auto& entry : entries) {
+ nsCOMPtr<nsIFile> file = new nsLocalFile();
+ rv = PathUtils::InitFileWithPath(file, entry);
+ if (NS_WARN_IF(NS_FAILED(rv))) {
+ continue;
+ }
+ MOZ_TRY(SetWindowsAttributesSync(file, aSetAttrs, aClearAttrs, aRecursive));
+ }
+
return Ok{};
}
diff --git a/xpcom/ioutils/IOUtils.h b/xpcom/ioutils/IOUtils.h
@@ -188,7 +188,8 @@ class IOUtils final {
static already_AddRefed<dom::Promise> SetWindowsAttributes(
dom::GlobalObject& aGlobal, const nsAString& aPath,
- const mozilla::dom::WindowsFileAttributes& aAttrs, ErrorResult& aError);
+ const mozilla::dom::WindowsFileAttributes& aAttrs, bool aRecursive,
+ ErrorResult& aError);
#elif defined(XP_MACOSX)
static already_AddRefed<dom::Promise> HasMacXAttr(dom::GlobalObject& aGlobal,
const nsAString& aPath,
@@ -538,13 +539,17 @@ class IOUtils final {
/**
* Set the Windows-specific attributes of the file.
*
- * @param aFile The location of the file.
- * @param aAttrs The attributes to set on the file.
+ * @param aFile The location of the file.
+ * @param aSetAttrs The attributes to set on the file.
+ * @param aClearAttrs The attributes to clear on the file.
+ * @param aRecursive Whether or not to apply the change to folder contents
+ * recursively.
*
* @return |Ok| if the attributes were successfully set, or an error.
*/
static Result<Ok, IOError> SetWindowsAttributesSync(
- nsIFile* aFile, const uint32_t aSetAttrs, const uint32_t aClearAttrs);
+ nsIFile* aFile, const uint32_t aSetAttrs, const uint32_t aClearAttrs,
+ bool aRecursive);
#elif defined(XP_MACOSX)
static Result<bool, IOError> HasMacXAttrSync(nsIFile* aFile,
const nsCString& aAttr);
