commit 2bd761adb4f088862a8cb692a3b56bab789d8307
parent a47dfff9a41b8c0e730eb3d9895e5e51b26930e2
Author: Duncan McIntosh <dmcintosh@mozilla.com>
Date: Mon, 3 Nov 2025 21:53:03 +0000
Bug 1997389 - Post: Test that BackupService handles features being disabled at startup. r=cdupuis
This is meant to keep the strict checks on the number of observer
notifications while making it possible to check startup behaviour.
initStatusObservers and uninitStatusObservers are made usable in tests,
since otherwise creating a new BackupService would add more
notifications every time something changes. The design is inspired by
initBackupScheduler and is called automatically for the global instance;
as such, no UI code needs to change.
The spurious infrastructure is provided for the next commit in the
stack, although it makes me wonder whether the strict counting is really
useful. (In this patch it also accounts for a Nimbus design choice.)
The removed tests are obsolete thanks to those in
test_BackupService_enabled.js.
Differential Revision: https://phabricator.services.mozilla.com/D270899
Diffstat:
3 files changed, 62 insertions(+), 114 deletions(-)
diff --git a/browser/components/backup/BackupService.sys.mjs b/browser/components/backup/BackupService.sys.mjs
@@ -1128,6 +1128,7 @@ export class BackupService extends EventTarget {
this.#instance.checkForPostRecovery();
this.#instance.initBackupScheduler();
+ this.#instance.initStatusObservers();
return this.#instance;
}
@@ -1157,7 +1158,6 @@ export class BackupService extends EventTarget {
constructor(backupResources = DefaultBackupResources) {
super();
lazy.logConsole.debug("Instantiated");
- this.#registerStatusObservers();
for (const resourceName in backupResources) {
let resource = backupResources[resourceName];
@@ -1178,8 +1178,6 @@ export class BackupService extends EventTarget {
}
}
}, BackupService.REGENERATION_DEBOUNCE_RATE_MS);
-
- this.#notifyStatusObservers();
}
/**
@@ -3841,7 +3839,7 @@ export class BackupService extends EventTarget {
}
case "quit-application-granted": {
this.uninitBackupScheduler();
- this.#unregisterStatusObservers();
+ this.uninitStatusObservers();
break;
}
case "passwordmgr-storage-changed": {
@@ -3892,7 +3890,11 @@ export class BackupService extends EventTarget {
}
}
- #registerStatusObservers() {
+ initStatusObservers() {
+ if (this.#statusPrefObserver != null) {
+ return;
+ }
+
// We don't use this.#observer since any changes to the prefs or nimbus should
// immediately reflect across any observers, instead of waiting on idle.
this.#statusPrefObserver = () => {
@@ -3904,9 +3906,10 @@ export class BackupService extends EventTarget {
Services.prefs.addObserver(pref, this.#statusPrefObserver);
}
lazy.NimbusFeatures.backupService.onUpdate(this.#statusPrefObserver);
+ this.#notifyStatusObservers();
}
- #unregisterStatusObservers() {
+ uninitStatusObservers() {
if (this.#statusPrefObserver == null) {
return;
}
diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_enabled.js b/browser/components/backup/tests/xpcshell/test_BackupService_enabled.js
@@ -43,8 +43,6 @@ add_setup(async () => {
Services.prefs.clearUserPref(BACKUP_DIR_PREF_NAME);
});
-
- BackupService.init();
});
add_task(async function test_archive_killswitch_enrollment() {
@@ -60,6 +58,9 @@ add_task(async function test_archive_killswitch_enrollment() {
async enable() {
await cleanupExperiment();
},
+ // Nimbus calls onUpdate if any experiments are running, meaning that the
+ // observer service will be notified twice, i.e. one spurious call.
+ startup: 1,
});
});
@@ -112,6 +113,9 @@ add_task(async function test_restore_killswitch_enrollment() {
async enable() {
await cleanupExperiment();
},
+ // Nimbus calls onUpdate if any experiments are running, meaning that the
+ // observer service will be notified twice, i.e. one spurious call.
+ startup: 1,
});
});
@@ -151,24 +155,20 @@ add_task(async function test_restore_selectable_profiles() {
});
});
-async function archiveTemplate({ internalReason, disable, enable }) {
+async function archiveTemplate({ internalReason, disable, enable, startup }) {
Services.telemetry.clearScalars();
Services.fog.testResetFOG();
- const bs = BackupService.get();
-
- Assert.equal(
- bs.archiveEnabledStatus.enabled,
- true,
- "BackupService is enabled at the start"
- );
+ let bs = new BackupService();
+ bs.initStatusObservers();
+ assertStatus("archive", bs.archiveEnabledStatus, true, null);
let calledCount = 0;
let callback = () => calledCount++;
Services.obs.addObserver(callback, "backup-service-status-updated");
- await disable();
- Assert.equal(calledCount, 1, "Observers were notified on disable");
+ let spurious = (await disable()) ?? 0;
+ Assert.equal(calledCount, 1 + spurious, "Observers were notified on disable");
assertStatus("archive", bs.archiveEnabledStatus, false, internalReason);
let backup = await bs.createBackup();
@@ -177,8 +177,12 @@ async function archiveTemplate({ internalReason, disable, enable }) {
"Creating a backup should fail when archiving is disabled."
);
- await enable();
- Assert.equal(calledCount, 2, "Observers were notified on re-enable");
+ spurious += (await enable()) ?? 0;
+ Assert.equal(
+ calledCount,
+ 2 + spurious,
+ "Observers were notified on re-enable"
+ );
assertStatus("archive", bs.archiveEnabledStatus, true, "reenabled");
backup = await bs.createBackup();
@@ -192,16 +196,29 @@ async function archiveTemplate({ internalReason, disable, enable }) {
);
await IOUtils.remove(backup.archivePath);
+ bs.uninitStatusObservers();
+
+ // Also check that it works at startup.
+ spurious += (startup ?? 0) + ((await disable()) ?? 0);
+ bs = new BackupService();
+ bs.initStatusObservers();
+ Assert.equal(calledCount, 3 + spurious, "Observers were notified at startup");
+ assertStatus("archive", bs.archiveEnabledStatus, false, internalReason);
+ await enable();
+ bs.uninitStatusObservers();
+
Services.obs.removeObserver(callback, "backup-service-status-updated");
}
-async function restoreTemplate({ internalReason, disable, enable }) {
+async function restoreTemplate({ internalReason, disable, enable, startup }) {
Services.telemetry.clearScalars();
Services.fog.testResetFOG();
- const bs = BackupService.get();
- const backup = await bs.createBackup();
+ let bs = new BackupService();
+ bs.initStatusObservers();
+ assertStatus("restore", bs.restoreEnabledStatus, true, null);
+ const backup = await bs.createBackup();
Assert.ok(
backup && backup.archivePath,
"Archive should have been created on disk."
@@ -211,8 +228,8 @@ async function restoreTemplate({ internalReason, disable, enable }) {
let callback = () => calledCount++;
Services.obs.addObserver(callback, "backup-service-status-updated");
- await disable();
- Assert.equal(calledCount, 1, "Observers were notified on disable");
+ let spurious = (await disable()) ?? 0;
+ Assert.equal(calledCount, 1 + spurious, "Observers were notified on disable");
assertStatus("restore", bs.restoreEnabledStatus, false, internalReason);
const recoveryDir = await IOUtils.createUniqueDirectory(
@@ -232,8 +249,12 @@ async function restoreTemplate({ internalReason, disable, enable }) {
"Recovery should throw when the restore is disabled."
);
- await enable();
- Assert.equal(calledCount, 2, "Observers were notified on re-enable");
+ spurious += (await enable()) ?? 0;
+ Assert.equal(
+ calledCount,
+ 2 + spurious,
+ "Observers were notified on re-enable"
+ );
assertStatus("restore", bs.restoreEnabledStatus, true, "reenabled");
let recoveredProfile = await bs.recoverFromBackupArchive(
@@ -252,6 +273,17 @@ async function restoreTemplate({ internalReason, disable, enable }) {
"Recovered profile directory should exist on disk."
);
+ bs.uninitStatusObservers();
+
+ // Also check that it works at startup.
+ spurious += (startup ?? 0) + ((await disable()) ?? 0);
+ bs = new BackupService();
+ bs.initStatusObservers();
+ Assert.equal(calledCount, 3 + spurious, "Observers were notified at startup");
+ assertStatus("restore", bs.restoreEnabledStatus, false, internalReason);
+ await enable();
+ bs.uninitStatusObservers();
+
Services.obs.removeObserver(callback, "backup-service-status-updated");
}
diff --git a/browser/components/backup/tests/xpcshell/test_BackupService_telemetry.js b/browser/components/backup/tests/xpcshell/test_BackupService_telemetry.js
@@ -288,90 +288,3 @@ add_task(async function test_idleDispatchPassesOptionsThrough() {
"Options were passed as-is into createBackup."
);
});
-
-add_task(async function test_backupDisableReason_reEnabled() {
- Services.fog.testResetFOG();
- let bs = new BackupService();
-
- Assert.equal(
- Glean.browserBackup.backupDisabledReason.testGetValue(),
- null,
- "No disable reason is reported before it is disabled."
- );
-
- let status = {
- enabled: true,
- };
- sinon.stub(bs, "archiveEnabledStatus").get(() => status);
-
- await bs.createBackup();
- Assert.equal(
- Glean.browserBackup.backupDisabledReason.testGetValue(),
- null,
- "No disable reason is reported on first backup when enabled."
- );
-
- status = {
- enabled: false,
- reason: "Stubbed out by test (#1)",
- internalReason: "* test *",
- };
-
- await bs.createBackup();
- Assert.equal(
- Glean.browserBackup.backupDisabledReason.testGetValue(),
- "* test *",
- "Disable reason is reported."
- );
-
- status = {
- enabled: true,
- };
-
- await bs.createBackup();
- Assert.equal(
- Glean.browserBackup.backupDisabledReason.testGetValue(),
- "reenabled",
- "Backup service reports that it has been reenabled."
- );
-});
-
-add_task(async function test_backupDisableReason_startup() {
- let sandbox = sinon.createSandbox();
- let status = {};
- sandbox
- .stub(BackupService.prototype, "archiveEnabledStatus")
- .get(() => status);
-
- status = {
- enabled: false,
- reason: "Stubbed out by test (#2)",
- internalReason: "* startup *",
- };
-
- Services.fog.testResetFOG();
- let bs = new BackupService();
- Assert.equal(
- Glean.browserBackup.backupDisabledReason.testGetValue(),
- "* startup *",
- "Backup service reports that is is disabled at startup."
- );
-
- await bs.createBackup();
- Assert.equal(
- Glean.browserBackup.backupDisabledReason.testGetValue(),
- "* startup *",
- "Backup service reports that is is disabled after creating a backup."
- );
-
- status = {
- enabled: true,
- };
-
- await bs.createBackup();
- Assert.equal(
- Glean.browserBackup.backupDisabledReason.testGetValue(),
- "reenabled",
- "Backup service reports that it is re-enabled."
- );
-});
