Bug 1667762 - Fix a race condition in logins backup deletion code that's preventing the backup from getting deleted. r=sfoster

Differential Revision: https://phabricator.services.mozilla.com/D91721

toolkit/components/passwordmgr/LoginStore.jsm

@@ -43,12 +43,20 @@ const EXPORTED_SYMBOLS = ["LoginStore"];
 // Globals
 
 const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
-
+ChromeUtils.defineModuleGetter(this, "OS", "resource://gre/modules/osfile.jsm");
 ChromeUtils.defineModuleGetter(
   this,
   "JSONFile",
   "resource://gre/modules/JSONFile.jsm"
 );
+const { XPCOMUtils } = ChromeUtils.import(
+  "resource://gre/modules/XPCOMUtils.jsm"
+);
+
+XPCOMUtils.defineLazyModuleGetters(this, {
+  FXA_PWDMGR_HOST: "resource://gre/modules/FxAccountsCommon.js",
+  FXA_PWDMGR_REALM: "resource://gre/modules/FxAccountsCommon.js",
+});
 
 /**
  * Current data version assigned by the code that last touched the data.
@@ -88,6 +96,54 @@ function LoginStore(aPath, aBackupPath = "") {
 LoginStore.prototype = Object.create(JSONFile.prototype);
 LoginStore.prototype.constructor = LoginStore;
 
+LoginStore.prototype._save = async function() {
+  await JSONFile.prototype._save.call(this);
+
+  if (this._options.backupTo) {
+    await this._backupHandler();
+  }
+};
+
+/**
+ * Delete logins backup file if the last saved login was removed using
+ * removeLogin() or if all logins were removed at once using removeAllLogins().
+ * Note that if the user has a fxa key stored as a login, we just update the
+ * backup to only store the key when the last saved user facing login is removed.
+ */
+LoginStore.prototype._backupHandler = async function() {
+  const logins = this._data.logins;
+  // Return early if more than one login is stored because the backup won't need
+  // updating in this case.
+  if (logins.length > 1) {
+    return;
+  }
+
+  // If one login is stored and it's a fxa sync key, we update the backup to store the
+  // key only.
+  if (
+    logins.length &&
+    logins[0].hostname == FXA_PWDMGR_HOST &&
+    logins[0].httpRealm == FXA_PWDMGR_REALM
+  ) {
+    try {
+      await OS.File.copy(this.path, this._options.backupTo);
+
+      // This notification is specifically sent out for a test.
+      Services.obs.notifyObservers(null, "logins-backup-updated");
+    } catch (ex) {
+      Cu.reportError(ex);
+    }
+  } else if (!logins.length) {
+    // If no logins are stored anymore, delete backup.
+    await OS.File.remove(this._options.backupTo, {
+      ignoreAbsent: true,
+    });
+
+    // This notification is specifically sent out for a test.
+    Services.obs.notifyObservers(null, "logins-backup-updated");
+  }
+};
+
 /**
  * Synchronously work on the data just loaded into memory.
  */

toolkit/components/passwordmgr/storage-json.js

@@ -35,11 +35,6 @@ XPCOMUtils.defineLazyServiceGetter(
   "nsIUUIDGenerator"
 );
 
-XPCOMUtils.defineLazyModuleGetters(this, {
-  FXA_PWDMGR_HOST: "resource://gre/modules/FxAccountsCommon.js",
-  FXA_PWDMGR_REALM: "resource://gre/modules/FxAccountsCommon.js",
-});
-
 class LoginManagerStorage_json {
   constructor() {
     this.__crypto = null; // nsILoginManagerCrypto service
@@ -292,7 +287,6 @@ class LoginManagerStorage_json {
 
     LoginHelper.notifyStorageChanged("removeLogin", storedLogin);
     this._recordEntryPresent();
-    this._updateLoginsBackup();
   }
 
   modifyLogin(oldLogin, newLoginData) {
@@ -663,7 +657,6 @@ class LoginManagerStorage_json {
     this._store.saveSoon();
 
     LoginHelper.notifyStorageChanged("removeAllLogins", null);
-    this._updateLoginsBackup();
   }
 
   findLogins(origin, formActionOrigin, httpRealm) {
@@ -849,47 +842,6 @@ class LoginManagerStorage_json {
       !!this._store.data.logins.length
     );
   }
-
-  // Delete logins backup file if the last saved login was removed using
-  // removeLogin() or if all logins were removed at once using removeAllLogins().
-  // Note that if the user has a fxa key stored as a login, we just update the
-  // backup to only store the key when the last saved user facing login is removed
-  // using removeLogin().
-  async _updateLoginsBackup() {
-    // If we are not maintaining a backup, return early.
-    if (!this._store._options.backupTo) {
-      return;
-    }
-
-    const logins = this._store.data.logins;
-    // Return early if more than one login is stored because the backup won't need
-    // updating in this case.
-    if (logins.length > 1) {
-      return;
-    }
-
-    // If one login is stored and it's a fxa key, we update the backup to store the fxa
-    // key only.
-    if (
-      logins.length &&
-      logins[0].hostname == FXA_PWDMGR_HOST &&
-      logins[0].httpRealm == FXA_PWDMGR_REALM
-    ) {
-      try {
-        await OS.File.copy(this._store.path, this._store._options.backupTo);
-      } catch (ex) {
-        Cu.reportError(ex);
-      }
-    } else if (!logins.length) {
-      // If no logins are stored anymore, delete backup.
-      await OS.File.remove(this._store._options.backupTo, {
-        ignoreAbsent: true,
-      });
-    }
-
-    // This notification is specifically sent out for a test.
-    Services.obs.notifyObservers(null, "logins-backup-updated");
-  }
 }
 
 XPCOMUtils.defineLazyGetter(LoginManagerStorage_json.prototype, "log", () => {