commit ec78446178daf087581889fb5d8e40a84bbdec98
parent 1b3787c361452ef9c9e37d8ffd4c414135d547f5
Author: Jens Stutte <jstutte@mozilla.com>
Date: Thu, 16 Oct 2025 17:24:56 +0000
Bug 1992353 - Make LoginManagerStorage_json.terminate sound. r=credential-management-reviewers,joschmidt,bdk
Differential Revision: https://phabricator.services.mozilla.com/D267608
Diffstat:
4 files changed, 54 insertions(+), 23 deletions(-)
diff --git a/toolkit/components/passwordmgr/LoginManager.sys.mjs b/toolkit/components/passwordmgr/LoginManager.sys.mjs
@@ -11,6 +11,7 @@ const lazy = {};
ChromeUtils.defineESModuleGetters(lazy, {
LoginHelper: "resource://gre/modules/LoginHelper.sys.mjs",
+ AsyncShutdown: "resource://gre/modules/AsyncShutdown.sys.mjs",
});
ChromeUtils.defineLazyGetter(lazy, "log", () => {
@@ -64,6 +65,13 @@ LoginManager.prototype = {
// Cache references to current |this| in utility objects
this._observer._pwmgr = this;
+ this._shutdownBlocker = async () => {
+ return this.uninit();
+ };
+ lazy.AsyncShutdown.profileChangeTeardown.addBlocker(
+ "LoginManager",
+ this._shutdownBlocker
+ );
this._observer._init();
// Initialize storage so that asynchronous data loading can start.
@@ -71,14 +79,18 @@ LoginManager.prototype = {
this._initialized = true;
},
- uninit() {
- if (this._storage) {
- this._storage.terminate();
- this._storage = null;
+ async uninit() {
+ if (this._shutdownBlocker) {
+ lazy.AsyncShutdown.profileChangeTeardown.removeBlocker(
+ this._shutdownBlocker
+ );
+ delete this._shutdownBlocker;
}
+ // Note that this._storage has its own shutdown observer, so we do not
+ // need to finalize it here but can unlink it.
+ this._storage = null;
this._observer._uninit();
-
this._observer._pwmgr = null;
this._initialized = false;
},
@@ -114,7 +126,6 @@ LoginManager.prototype = {
return;
}
Services.obs.addObserver(this, "passwordmgr-storage-replace");
- Services.obs.addObserver(this, "xpcom-shutdown");
this._initialized = true;
},
@@ -123,7 +134,6 @@ LoginManager.prototype = {
return;
}
Services.obs.removeObserver(this, "passwordmgr-storage-replace");
- Services.obs.removeObserver(this, "xpcom-shutdown");
this._initialized = false;
},
@@ -134,11 +144,10 @@ LoginManager.prototype = {
// nsIObserver
observe(subject, topic, _data) {
- if (topic == "xpcom-shutdown") {
- this._pwmgr.uninit();
- } else if (topic == "passwordmgr-storage-replace") {
+ if (topic == "passwordmgr-storage-replace") {
+ // This notification is only issued via LoginTestUtils.reloadData().
(async () => {
- await this._pwmgr._storage.terminate();
+ await this._pwmgr._storage.testSaveForReplace();
this._pwmgr._initStorage();
await this._pwmgr.initializationPromise;
Services.obs.notifyObservers(
diff --git a/toolkit/components/passwordmgr/storage-geckoview.sys.mjs b/toolkit/components/passwordmgr/storage-geckoview.sys.mjs
@@ -44,10 +44,10 @@ export class LoginManagerStorage extends LoginManagerStorage_json {
}
/**
- * Internal method used by regression tests only. It is called before
- * replacing this storage module with a new instance.
+ * Internal method used by tests only. It is called before replacing
+ * this storage module with a new instance.
*/
- terminate() {}
+ testSaveForReplace() {}
async addLoginsAsync(_logins, _continueOnDuplicates = false) {
throw Components.Exception("", Cr.NS_ERROR_NOT_IMPLEMENTED);
diff --git a/toolkit/components/passwordmgr/storage-json.sys.mjs b/toolkit/components/passwordmgr/storage-json.sys.mjs
@@ -95,6 +95,8 @@ export class LoginManagerStorage_json {
if (loginsBackupEnabled) {
backupPath = PathUtils.join(profileDir, "logins-backup.json");
}
+ // Note that LoginStore is based on JSONFile which brings its own
+ // shutdown blocker to finalize properly, so we do not need one here.
this._store = new lazy.LoginStore(jsonPath, backupPath);
// The ProfileDataUpgrader can possibly set this pref. As we don't know
@@ -118,10 +120,11 @@ export class LoginManagerStorage_json {
}
/**
- * Internal method used by regression tests only. It is called before
- * replacing this storage module with a new instance.
+ * Internal method used by tests only. It is called before replacing
+ * this storage module with a new instance. It avoids to finalize the
+ * underlying DeferredTask as it is still needed for the next tests.
*/
- terminate() {
+ testSaveForReplace() {
this._store._saver.disarm();
return this._store._save();
}
diff --git a/toolkit/components/passwordmgr/storage-rust.sys.mjs b/toolkit/components/passwordmgr/storage-rust.sys.mjs
@@ -241,11 +241,11 @@ export class LoginManagerRustStorage {
this.#storageAdapter = new RustLoginsStoreAdapter(store);
this.log("Rust login storage ready.");
- // Interrupt sooner prior to the `profile-before-change` phase to allow
- // all the in-progress IOs to exit.
+ // All LoginManager storage backends must have their own shutdown
+ // blocker to ensure that they finalize properly.
lazy.AsyncShutdown.profileChangeTeardown.addBlocker(
"LoginManagerRustStorage: Interrupt IO operations on login store",
- () => this.terminate()
+ async () => this.finalize()
);
resolve(this);
@@ -262,11 +262,30 @@ export class LoginManagerRustStorage {
}
/**
- * Internal method used by regression tests only. It is called before
- * replacing this storage module with a new instance, and on shutdown
+ * Terminate all pending writes. After this call, the store can't be used.
*/
- terminate() {
+ async finalize() {
+ // TODO: Currently we do not mark the instance as closed, not sure if later
+ // calls would be rejected elsewhere.
+
+ // Note: This is a synchronous call.
this.#storageAdapter.shutdown();
+ return Promise.resolve();
+ }
+
+ /**
+ * Internal method used by tests only. It is called before replacing
+ * this storage module with a new instance.
+ */
+ testSaveForReplace() {
+ // Currently we only ever call this on LoginManagerStorage which is derived
+ // from LoginManagerStorage_json and there seems to be nothing that would
+ // want to call it here, but maybe once we entirely replace the JSON store
+ // with this one it would be called and we'd need to implement it.
+ throw Components.Exception(
+ "testSaveForReplace",
+ Cr.NS_ERROR_NOT_IMPLEMENTED
+ );
}
/**
