commit ee231a59a0abee2a974a5280b4e75031951a1e48 parent 385b95367724fb9618970585ba0b16f59f8edc88 Author: David P. <daparks@mozilla.com> Date: Thu, 23 Oct 2025 22:07:11 +0000 Bug 1990634: Fix some FxBackup async and test issues r=mconley * Asyncs without awaits for non-async functions BackupService.initBackupScheduler and BackupService.uninitBackupScheduler. * Missing retryReadonly for backup file delete. * More robust handling of issue with Windows kernel briefly preventing deletes in tests. * test_BackupService_archive was no longer using the correct default directory when run on machines with a personal OneDrive folder, since bug 1989660. * Moves some exceptions from nsIRequestObserver.onStopRequest (that should have been no-ops -- see docs for that method) to nsIStreamListener.onDataAvailable (again, see the method's docs), which aborts the stream, as was the original intent. * Closes a file left open in the archive worker. Differential Revision: https://phabricator.services.mozilla.com/D267708 Diffstat:
12 files changed, 98 insertions(+), 62 deletions(-)
diff --git a/browser/components/backup/Archive.worker.mjs b/browser/components/backup/Archive.worker.mjs @@ -410,6 +410,8 @@ ${ArchiveUtils.INLINE_MIME_END_MARKER} oldBuffer = buffer; } + syncReadFile.close(); + if (!contentType) { throw new BackupError( "Failed to find embedded data in archive", diff --git a/browser/components/backup/BackupService.sys.mjs b/browser/components/backup/BackupService.sys.mjs @@ -298,6 +298,14 @@ class BinaryReadableStream { * The number of bytes available in the stream */ onDataAvailable(request, stream, offset, count) { + if (this._done) { + // No need to load anything else - abort reading in more + // attachments. + throw Components.Exception( + "Got binary block - cancelling loading the multipart stream.", + Cr.NS_BINDING_ABORTED + ); + } if (!this._enabled) { // We don't care about this data, just move on. return; @@ -319,13 +327,6 @@ class BinaryReadableStream { this._done = true; controller.close(); - - // No need to load anything else - abort reading in more - // attachments. - throw Components.Exception( - "Got binary block - cancelling loading the multipart stream.", - Cr.NS_BINDING_ABORTED - ); } }, @@ -2287,6 +2288,14 @@ export class BackupService extends EventTarget { * The number of bytes available in the stream */ onDataAvailable(request, stream, offset, count) { + if (this._done) { + // No need to load anything else - abort reading in more + // attachments. + throw Components.Exception( + "Got JSON block. Aborting further reads.", + Cr.NS_BINDING_ABORTED + ); + } if (!this._enabled) { // We don't care about this data, just move on. return; @@ -2318,12 +2327,6 @@ export class BackupService extends EventTarget { ) ); } - // No need to load anything else - abort reading in more - // attachments. - throw Components.Exception( - "Got JSON block. Aborting further reads.", - Cr.NS_BINDING_ABORTED - ); } }, @@ -3684,10 +3687,8 @@ export class BackupService extends EventTarget { * * The scheduler will automatically uninitialize itself on the * quit-application-granted observer notification. - * - * @returns {Promise<undefined>} */ - async initBackupScheduler() { + initBackupScheduler() { if (this.#backupSchedulerInitted) { lazy.logConsole.warn( "BackupService scheduler already initting or initted." @@ -3759,10 +3760,8 @@ export class BackupService extends EventTarget { /** * Uninitializes the backup scheduling system. - * - * @returns {Promise<undefined>} */ - async uninitBackupScheduler() { + uninitBackupScheduler() { if (!this.#backupSchedulerInitted) { lazy.logConsole.warn( "Tried to uninitBackupScheduler when it wasn't yet enabled." @@ -4379,7 +4378,10 @@ export class BackupService extends EventTarget { "Attempting to delete last backup file at ", backupFilePath ); - await IOUtils.remove(backupFilePath, { ignoreAbsent: true }); + await IOUtils.remove(backupFilePath, { + ignoreAbsent: true, + retryReadonly: true, + }); } this.#_state.lastBackupDate = null; diff --git a/browser/components/backup/tests/xpcshell/head.js b/browser/components/backup/tests/xpcshell/head.js @@ -15,6 +15,7 @@ ChromeUtils.defineESModuleGetters(this, { MockRegistrar: "resource://testing-common/MockRegistrar.sys.mjs", PrivateBrowsingUtils: "resource://gre/modules/PrivateBrowsingUtils.sys.mjs", AppConstants: "resource://gre/modules/AppConstants.sys.mjs", + setTimeout: "resource://gre/modules/Timer.sys.mjs", }); const HISTORY_ENABLED_PREF = "places.history.enabled"; @@ -47,6 +48,10 @@ add_setup(async () => { await OSKeyStoreTestUtils.cleanup(); MockRegistrar.unregister(osKeyStoreCID); }); + + // This will set gDirServiceProvider, which doesn't happen automatically + // in xpcshell tests, but is needed by BackupService. + Cc["@mozilla.org/xre/directory-provider;1"].getService(Ci.nsIXREDirProvider); }); const BYTES_IN_KB = 1000; @@ -198,21 +203,62 @@ async function assertFilesExist(parentPath, testFilesArray) { } /** + * Perform removalFn, potentially a few times, with special consideration for + * Windows, which has issues with file removal. Sometimes remove() throws + * ERROR_LOCK_VIOLATION when the file is not unlocked soon enough. This can + * happen because kernel operations still hold a handle to it, or because e.g. + * Windows Defender started a scan, or whatever. Some applications (for + * example SQLite) retry deletes with a delay on Windows. We emulate that + * here, since this happens very frequently in tests. + * + * This registers a test failure if it does not succeed. + * + * @param {Function} removalFn Async function to (potentially repeatedly) + * call. + * @throws The resultant file system exception if removal fails, despite the + * special considerations. + */ +async function doFileRemovalOperation(removalFn) { + const kMaxRetries = 5; + let nRetries = 0; + let lastException; + while (nRetries < kMaxRetries) { + nRetries++; + try { + await removalFn(); + // Success! + return; + } catch (error) { + if ( + AppConstants.platform !== "win" || + !/NS_ERROR_FILE_IS_LOCKED/.test(error.message) + ) { + throw error; + } + lastException = error; + } + await new Promise(res => setTimeout(res, 100)); + } + + // All retries failed. Re-throw last exception. + Assert.ok(false, `doRemovalOperation failed after ${nRetries} attempts`); + throw lastException; +} + +/** * Remove a file or directory at a path if it exists and files are unlocked. + * Prefer this to IOUtils.remove in tests to avoid intermittent failures + * because of the Windows-ism mentioned in doFileRemovalOperation. * * @param {string} path path to remove. */ async function maybeRemovePath(path) { - try { + let nAttempts = 0; + await doFileRemovalOperation(async () => { + nAttempts++; await IOUtils.remove(path, { ignoreAbsent: true, recursive: true }); - } catch (error) { - // Sometimes remove() throws when the file is not unlocked soon - // enough. - if (error.name != "NS_ERROR_FILE_IS_LOCKED") { - // Ignoring any errors, as the temp folder will be cleaned up. - console.error(error); - } - } + Assert.ok(true, `Removed ${path} on attempt #${nAttempts}`); + }); } /** diff --git a/browser/components/backup/tests/xpcshell/test_BackupService.js b/browser/components/backup/tests/xpcshell/test_BackupService.js @@ -3,9 +3,6 @@ https://creativecommons.org/publicdomain/zero/1.0/ */ "use strict"; -const { AppConstants } = ChromeUtils.importESModule( - "resource://gre/modules/AppConstants.sys.mjs" -); const { BasePromiseWorker } = ChromeUtils.importESModule( "resource://gre/modules/PromiseWorker.sys.mjs" ); @@ -340,7 +337,7 @@ async function testCreateBackupHelper(sandbox, taskFn) { "initiated recovery" ); - taskFn(bs, manifest); + await taskFn(bs, manifest); await maybeRemovePath(backupFilePath); await maybeRemovePath(fakeProfilePath); @@ -409,7 +406,11 @@ async function testDeleteLastBackupHelper(taskFn) { await taskFn(LAST_BACKUP_FILE_PATH); } - await bs.deleteLastBackup(); + // NB: On Windows, deletes of backups in tests run into an issue where + // the file is locked briefly by the system and deletes fail with + // NS_ERROR_FILE_IS_LOCKED. See doFileRemovalOperation for details. + // We therefore retry this delete a few times before accepting failure. + await doFileRemovalOperation(async () => await bs.deleteLastBackup()); Assert.equal( bs.state.lastBackupDate, diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_archive.js b/browser/components/backup/tests/xpcshell/test_BackupService_archive.js @@ -69,7 +69,7 @@ add_setup(async () => { registerCleanupFunction(async () => { await OSKeyStoreTestUtils.cleanup(); - await IOUtils.remove(testProfilePath, { recursive: true }); + await maybeRemovePath(testProfilePath); }); }); @@ -112,8 +112,8 @@ add_task(async function test_createArchive_unencrypted() { assertExtractionsMatch(EXTRACTION_PATH); - await IOUtils.remove(FAKE_ARCHIVE_PATH); - await IOUtils.remove(EXTRACTION_PATH); + await maybeRemovePath(FAKE_ARCHIVE_PATH); + await maybeRemovePath(EXTRACTION_PATH); }); /** @@ -167,8 +167,8 @@ add_task(async function test_createArchive_encrypted() { assertExtractionsMatch(EXTRACTION_PATH); - await IOUtils.remove(FAKE_ARCHIVE_PATH); - await IOUtils.remove(EXTRACTION_PATH); + await maybeRemovePath(FAKE_ARCHIVE_PATH); + await maybeRemovePath(EXTRACTION_PATH); }); /** @@ -313,8 +313,8 @@ add_task(async function test_createArchive_encrypted_truncated() { "Extraction should have been automatically destroyed." ); - await IOUtils.remove(FAKE_ARCHIVE_PATH); - await IOUtils.remove(FAKE_COMPRESSED_FILE); + await maybeRemovePath(FAKE_ARCHIVE_PATH); + await maybeRemovePath(FAKE_COMPRESSED_FILE); }); /** @@ -388,7 +388,7 @@ add_task(async function test_createArchive_early_binary_stream_close() { "Extraction should have been automatically destroyed." ); - await IOUtils.remove(FAKE_ARCHIVE_PATH); + await maybeRemovePath(FAKE_ARCHIVE_PATH); }); /** @@ -459,7 +459,7 @@ add_task(async function test_missing_binary_block() { /Could not find binary block/ ); - await IOUtils.remove(fakeRecoveryPath); + await maybeRemovePath(fakeRecoveryPath); }); /** diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_enable_disable_encryption.js b/browser/components/backup/tests/xpcshell/test_BackupService_enable_disable_encryption.js @@ -3,10 +3,6 @@ https://creativecommons.org/publicdomain/zero/1.0/ */ "use strict"; -const { AppConstants } = ChromeUtils.importESModule( - "resource://gre/modules/AppConstants.sys.mjs" -); - const TEST_PASSWORD = "This is some test password."; add_setup(async () => { diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_recoverFromSnapshotFolder.js b/browser/components/backup/tests/xpcshell/test_BackupService_recoverFromSnapshotFolder.js @@ -3,9 +3,6 @@ https://creativecommons.org/publicdomain/zero/1.0/ */ "use strict"; -const { AppConstants } = ChromeUtils.importESModule( - "resource://gre/modules/AppConstants.sys.mjs" -); const { ArchiveUtils } = ChromeUtils.importESModule( "resource:///modules/backup/ArchiveUtils.sys.mjs" ); diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_regeneration.js b/browser/components/backup/tests/xpcshell/test_BackupService_regeneration.js @@ -3,9 +3,6 @@ https://creativecommons.org/publicdomain/zero/1.0/ */ "use strict"; -const { setTimeout } = ChromeUtils.importESModule( - "resource://gre/modules/Timer.sys.mjs" -); const { NetUtil } = ChromeUtils.importESModule( "resource://gre/modules/NetUtil.sys.mjs" ); diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_resolveArchiveDestFolderPath.js b/browser/components/backup/tests/xpcshell/test_BackupService_resolveArchiveDestFolderPath.js @@ -3,10 +3,6 @@ https://creativecommons.org/publicdomain/zero/1.0/ */ "use strict"; -const { AppConstants } = ChromeUtils.importESModule( - "resource://gre/modules/AppConstants.sys.mjs" -); - const HOME_KEY = "Home"; let gTestRoot; let gFakeHomePath; diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_retryHeuristic.js b/browser/components/backup/tests/xpcshell/test_BackupService_retryHeuristic.js @@ -4,10 +4,8 @@ https://creativecommons.org/publicdomain/zero/1.0/ */ "use strict"; ChromeUtils.defineESModuleGetters(this, { - setTimeout: "resource://gre/modules/Timer.sys.mjs", BackupError: "resource:///modules/backup/BackupError.mjs", ERRORS: "chrome://browser/content/backup/backup-constants.mjs", - AppConstants: "resource://gre/modules/AppConstants.sys.mjs", }); const BACKUP_RETRY_LIMIT_PREF_NAME = "browser.backup.backup-retry-limit"; diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_scheduler.js b/browser/components/backup/tests/xpcshell/test_BackupService_scheduler.js @@ -54,7 +54,7 @@ add_task(async function test_init_uninitBackupScheduler() { sandbox.stub(idleService, "addIdleObserver"); sandbox.stub(idleService, "removeIdleObserver"); - await bs.initBackupScheduler(); + bs.initBackupScheduler(); Assert.ok( idleService.addIdleObserver.calledOnce, "addIdleObserver was called" diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_takeMeasurements.js b/browser/components/backup/tests/xpcshell/test_BackupService_takeMeasurements.js @@ -193,8 +193,9 @@ add_task(async function test_BackupService_location_on_device() { ); Services.telemetry.clearScalars(); - // The system "Docs" folder is considered the default location. - bs.setParentDirPath(Services.dirsvc.get("Docs", Ci.nsIFile).path); + // The default will be either the test machine's OneDrive folder, if present, + // or else the Docs folder. + bs.setParentDirPath(BackupService.DEFAULT_PARENT_DIR_PATH); await bs.takeMeasurements(); Assert.equal(