Bug 1305770 - make cookies and passwords import from Chrome without locking, r=mak draft
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Mon, 03 Oct 2016 16:17:06 +0100
changeset 422285 489a800ad1381bd6daa99040008fba24861d99ff
parent 422256 49fe455cac957808ed4a5d1685c3a1938dac1d31
child 533293 20c1af7a71601d5e50ffb02bfe2e02c38f0ceaa6
push id31728
push userbmo:gijskruitbosch+bugs@gmail.com
push dateFri, 07 Oct 2016 19:06:03 +0000
reviewersmak
bugs1305770
milestone52.0a1
Bug 1305770 - make cookies and passwords import from Chrome without locking, r=mak MozReview-Commit-ID: CbozmZHwAgz
browser/components/migration/ChromeProfileMigrator.js
browser/components/migration/MigrationUtils.jsm
--- a/browser/components/migration/ChromeProfileMigrator.js
+++ b/browser/components/migration/ChromeProfileMigrator.js
@@ -28,18 +28,16 @@ Cu.import("resource://gre/modules/osfile
 Cu.import("resource://gre/modules/Console.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource:///modules/MigrationUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "OSCrypto",
                                   "resource://gre/modules/OSCrypto.jsm");
-XPCOMUtils.defineLazyModuleGetter(this, "Sqlite",
-                                  "resource://gre/modules/Sqlite.jsm");
 /**
  * Get an nsIFile instance representing the expected location of user data
  * for this copy of Chrome/Chromium/Canary on different OSes.
  * @param subfoldersWin {Array} an array of subfolders to use for Windows
  * @param subfoldersOSX {Array} an array of subfolders to use for OS X
  * @param subfoldersUnix {Array} an array of subfolders to use for *nix systems
  * @returns {nsIFile} the place we expect data to live. Might not actually exist!
  */
@@ -103,17 +101,16 @@ function* insertBookmarkItems(parentGuid
       }
     } catch (e) {
       Cu.reportError(e);
       errorAccumulator(e);
     }
   }
 }
 
-
 function ChromeProfileMigrator() {
   let chromeUserDataFolder =
     getDataFolder(["Google", "Chrome"], ["Google", "Chrome"], ["google-chrome"]);
   this._chromeUserDataFolder = chromeUserDataFolder.exists() ?
     chromeUserDataFolder : null;
 }
 
 ChromeProfileMigrator.prototype = Object.create(MigratorPrototype);
@@ -306,69 +303,29 @@ function GetBookmarksResource(aProfileFo
 }
 
 function GetHistoryResource(aProfileFolder) {
   let historyFile = aProfileFolder.clone();
   historyFile.append("History");
   if (!historyFile.exists())
     return null;
 
-  function getRows(dbOptions) {
-    const RETRYLIMIT = 10;
-    const RETRYINTERVAL = 100;
-    return Task.spawn(function* innerGetRows() {
-      let rows = null;
-      for (let retryCount = RETRYLIMIT; retryCount && !rows; retryCount--) {
-        // Attempt to get the rows. If this succeeds, we will bail out of the loop,
-        // close the database in a failsafe way, and pass the rows back.
-        // If fetching the rows throws, we will wait RETRYINTERVAL ms
-        // and try again. This will repeat a maximum of RETRYLIMIT times.
-        let db;
-        let didOpen = false;
-        let exceptionSeen;
-        try {
-          db = yield Sqlite.openConnection(dbOptions);
-          didOpen = true;
-          rows = yield db.execute(`SELECT url, title, last_visit_time, typed_count
-                                   FROM urls WHERE hidden = 0`);
-        } catch (ex) {
-          if (!exceptionSeen) {
-            Cu.reportError(ex);
-          }
-          exceptionSeen = ex;
-        } finally {
-          try {
-            if (didOpen) {
-              yield db.close();
-            }
-          } catch (ex) {}
-        }
-        if (exceptionSeen) {
-          yield new Promise(resolve => setTimeout(resolve, RETRYINTERVAL));
-        }
-      }
-      if (!rows) {
-        throw new Error("Couldn't get rows from the Chrome history database.");
-      }
-      return rows;
-    });
-  }
-
   return {
     type: MigrationUtils.resourceTypes.HISTORY,
 
     migrate(aCallback) {
       Task.spawn(function* () {
         let dbOptions = {
           readOnly: true,
           ignoreLockingMode: true,
           path: historyFile.path
         };
 
-        let rows = yield getRows(dbOptions);
+        let rows = yield MigrationUtils.getRowsFromDBWithoutLocks(historyFile.path, "Chrome history",
+          `SELECT url, title, last_visit_time, typed_count FROM urls WHERE hidden = 0`);
         let places = [];
         for (let row of rows) {
           try {
             // if having typed_count, we changes transition type to typed.
             let transType = PlacesUtils.history.TRANSITION_LINK;
             if (row.getResultByName("typed_count") > 0)
               transType = PlacesUtils.history.TRANSITION_TYPED;
 
@@ -419,160 +376,140 @@ function GetCookiesResource(aProfileFold
   let cookiesFile = aProfileFolder.clone();
   cookiesFile.append("Cookies");
   if (!cookiesFile.exists())
     return null;
 
   return {
     type: MigrationUtils.resourceTypes.COOKIES,
 
-    migrate: function(aCallback) {
-      let dbConn = Services.storage.openUnsharedDatabase(cookiesFile);
+    migrate: Task.async(function* (aCallback) {
       // We don't support decrypting cookies yet so only import plaintext ones.
-      let stmt = dbConn.createAsyncStatement(`
-        SELECT host_key, name, value, path, expires_utc, secure, httponly, encrypted_value
+      let rows = yield MigrationUtils.getRowsFromDBWithoutLocks(cookiesFile.path, "Chrome cookies",
+       `SELECT host_key, name, value, path, expires_utc, secure, httponly, encrypted_value
         FROM cookies
-        WHERE length(encrypted_value) = 0`);
-
-      stmt.executeAsync({
-        handleResult : function(aResults) {
-          for (let row = aResults.getNextRow(); row; row = aResults.getNextRow()) {
-            let host_key = row.getResultByName("host_key");
-            if (host_key.match(/^\./)) {
-              // 1st character of host_key may be ".", so we have to remove it
-              host_key = host_key.substr(1);
-            }
+        WHERE length(encrypted_value) = 0`).catch(ex => {
+        Cu.reportError(ex);
+        aCallback(false);
+      });
+      // If the promise was rejected we will have already called aCallback,
+      // so we can just return here.
+      if (!rows) {
+        return;
+      }
 
-            try {
-              let expiresUtc =
-                chromeTimeToDate(row.getResultByName("expires_utc")) / 1000;
-              Services.cookies.add(host_key,
-                                   row.getResultByName("path"),
-                                   row.getResultByName("name"),
-                                   row.getResultByName("value"),
-                                   row.getResultByName("secure"),
-                                   row.getResultByName("httponly"),
-                                   false,
-                                   parseInt(expiresUtc),
-                                   {});
-            } catch (e) {
-              Cu.reportError(e);
-            }
-          }
-        },
+      for (let row of rows) {
+        let host_key = row.getResultByName("host_key");
+        if (host_key.match(/^\./)) {
+          // 1st character of host_key may be ".", so we have to remove it
+          host_key = host_key.substr(1);
+        }
 
-        handleError : function(aError) {
-          Cu.reportError("Async statement execution returned with '" +
-                         aError.result + "', '" + aError.message + "'");
-        },
-
-        handleCompletion : function(aReason) {
-          dbConn.asyncClose();
-          aCallback(aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED);
-        },
-      });
-      stmt.finalize();
-    }
-  }
+        try {
+          let expiresUtc =
+            chromeTimeToDate(row.getResultByName("expires_utc")) / 1000;
+          Services.cookies.add(host_key,
+                               row.getResultByName("path"),
+                               row.getResultByName("name"),
+                               row.getResultByName("value"),
+                               row.getResultByName("secure"),
+                               row.getResultByName("httponly"),
+                               false,
+                               parseInt(expiresUtc),
+                               {});
+        } catch (e) {
+          Cu.reportError(e);
+        }
+      }
+      aCallback(true);
+    }),
+  };
 }
 
 function GetWindowsPasswordsResource(aProfileFolder) {
   let loginFile = aProfileFolder.clone();
   loginFile.append("Login Data");
   if (!loginFile.exists())
     return null;
 
   return {
     type: MigrationUtils.resourceTypes.PASSWORDS,
 
-    migrate(aCallback) {
-      let dbConn = Services.storage.openUnsharedDatabase(loginFile);
-      let stmt = dbConn.createAsyncStatement(`
-        SELECT origin_url, action_url, username_element, username_value,
+    migrate: Task.async(function* (aCallback) {
+      let rows = yield MigrationUtils.getRowsFromDBWithoutLocks(loginFile.path, "Chrome passwords",
+       `SELECT origin_url, action_url, username_element, username_value,
         password_element, password_value, signon_realm, scheme, date_created,
-        times_used FROM logins WHERE blacklisted_by_user = 0`);
+        times_used FROM logins WHERE blacklisted_by_user = 0`).catch(ex => {
+        Cu.reportError(ex);
+        aCallback(false);
+      });
+      // If the promise was rejected we will have already called aCallback,
+      // so we can just return here.
+      if (!rows) {
+        return;
+      }
       let crypto = new OSCrypto();
 
-      stmt.executeAsync({
-        _rowToLoginInfo(row) {
-          let loginInfo = {
-            username: row.getResultByName("username_value"),
-            password: crypto.
-                      decryptData(crypto.arrayToString(row.getResultByName("password_value")),
-                                                       null),
-            hostName: NetUtil.newURI(row.getResultByName("origin_url")).prePath,
-            submitURL: null,
-            httpRealm: null,
-            usernameElement: row.getResultByName("username_element"),
-            passwordElement: row.getResultByName("password_element"),
-            timeCreated: chromeTimeToDate(row.getResultByName("date_created") + 0).getTime(),
-            timesUsed: row.getResultByName("times_used") + 0,
-          };
+      for (let row of rows) {
+        let loginInfo = {
+          username: row.getResultByName("username_value"),
+          password: crypto.
+                    decryptData(crypto.arrayToString(row.getResultByName("password_value")),
+                                                     null),
+          hostName: NetUtil.newURI(row.getResultByName("origin_url")).prePath,
+          submitURL: null,
+          httpRealm: null,
+          usernameElement: row.getResultByName("username_element"),
+          passwordElement: row.getResultByName("password_element"),
+          timeCreated: chromeTimeToDate(row.getResultByName("date_created") + 0).getTime(),
+          timesUsed: row.getResultByName("times_used") + 0,
+        };
 
+        try {
           switch (row.getResultByName("scheme")) {
             case AUTH_TYPE.SCHEME_HTML:
               loginInfo.submitURL = NetUtil.newURI(row.getResultByName("action_url")).prePath;
               break;
             case AUTH_TYPE.SCHEME_BASIC:
             case AUTH_TYPE.SCHEME_DIGEST:
               // signon_realm format is URIrealm, so we need remove URI
               loginInfo.httpRealm = row.getResultByName("signon_realm")
                                     .substring(loginInfo.hostName.length + 1);
               break;
             default:
               throw new Error("Login data scheme type not supported: " +
                               row.getResultByName("scheme"));
           }
-
-          return loginInfo;
-        },
-
-        handleResult(aResults) {
-          for (let row = aResults.getNextRow(); row; row = aResults.getNextRow()) {
-            try {
-              let loginInfo = this._rowToLoginInfo(row);
-              let login = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
+          let login = Cc["@mozilla.org/login-manager/loginInfo;1"].createInstance(Ci.nsILoginInfo);
 
-              login.init(loginInfo.hostName, loginInfo.submitURL, loginInfo.httpRealm,
-                         loginInfo.username, loginInfo.password, loginInfo.usernameElement,
-                         loginInfo.passwordElement);
-              login.QueryInterface(Ci.nsILoginMetaInfo);
-              login.timeCreated = loginInfo.timeCreated;
-              login.timeLastUsed = loginInfo.timeCreated;
-              login.timePasswordChanged = loginInfo.timeCreated;
-              login.timesUsed = loginInfo.timesUsed;
-
-              // Add the login only if there's not an existing entry
-              let logins = Services.logins.findLogins({}, login.hostname,
-                                                      login.formSubmitURL,
-                                                      login.httpRealm);
+          login.init(loginInfo.hostName, loginInfo.submitURL, loginInfo.httpRealm,
+                     loginInfo.username, loginInfo.password, loginInfo.usernameElement,
+                     loginInfo.passwordElement);
+          login.QueryInterface(Ci.nsILoginMetaInfo);
+          login.timeCreated = loginInfo.timeCreated;
+          login.timeLastUsed = loginInfo.timeCreated;
+          login.timePasswordChanged = loginInfo.timeCreated;
+          login.timesUsed = loginInfo.timesUsed;
 
-              // Bug 1187190: Password changes should be propagated depending on timestamps.
-              if (!logins.some(l => login.matches(l, true))) {
-                Services.logins.addLogin(login);
-              }
-            } catch (e) {
-              Cu.reportError(e);
-            }
-          }
-        },
+          // Add the login only if there's not an existing entry
+          let logins = Services.logins.findLogins({}, login.hostname,
+                                                  login.formSubmitURL,
+                                                  login.httpRealm);
 
-        handleError(aError) {
-          Cu.reportError("Async statement execution returned with '" +
-                         aError.result + "', '" + aError.message + "'");
-        },
-
-        handleCompletion(aReason) {
-          dbConn.asyncClose();
-          aCallback(aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED);
-          crypto.finalize();
-        },
-      });
-      stmt.finalize();
-    }
+          // Bug 1187190: Password changes should be propagated depending on timestamps.
+          if (!logins.some(l => login.matches(l, true))) {
+            Services.logins.addLogin(login);
+          }
+        } catch (e) {
+          Cu.reportError(e);
+        }
+      }
+      crypto.finalize();
+      aCallback(true);
+    }),
   };
 }
 
 ChromeProfileMigrator.prototype.classDescription = "Chrome Profile Migrator";
 ChromeProfileMigrator.prototype.contractID = "@mozilla.org/profile/migrator;1?app=browser&type=chrome";
 ChromeProfileMigrator.prototype.classID = Components.ID("{4cec1de4-1671-4fc3-a53e-6c539dc77a26}");
 
 
--- a/browser/components/migration/MigrationUtils.jsm
+++ b/browser/components/migration/MigrationUtils.jsm
@@ -18,16 +18,18 @@ Cu.import("resource://gre/modules/XPCOMU
 XPCOMUtils.defineLazyModuleGetter(this, "AutoMigrate",
                                   "resource:///modules/AutoMigrate.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "BookmarkHTMLUtils",
                                   "resource://gre/modules/BookmarkHTMLUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PromiseUtils",
                                   "resource://gre/modules/PromiseUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "Sqlite",
+                                  "resource://gre/modules/Sqlite.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "TelemetryStopwatch",
                                   "resource://gre/modules/TelemetryStopwatch.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "WindowsRegistry",
                                   "resource://gre/modules/WindowsRegistry.jsm");
 
 var gMigrators = null;
 var gProfileStartup = null;
 var gMigrationBundle = null;
@@ -533,16 +535,79 @@ this.MigrationUtils = Object.freeze({
   createImportedBookmarksFolder: Task.async(function* (sourceNameStr, parentGuid) {
     let source = this.getLocalizedString("sourceName" + sourceNameStr);
     let title = this.getLocalizedString("importedBookmarksFolder", [source]);
     return (yield PlacesUtils.bookmarks.insert({
       type: PlacesUtils.bookmarks.TYPE_FOLDER, parentGuid, title
     })).guid;
   }),
 
+  /**
+   * Get all the rows corresponding to a select query from a database, without
+   * requiring a lock on the database. If fetching data fails (because someone
+   * else tried to write to the DB at the same time, for example), we will
+   * retry the fetch after a 100ms timeout, up to 10 times.
+   *
+   * @param path
+   *        the file path to the database we want to open.
+   * @param description
+   *        a developer-readable string identifying what kind of database we're
+   *        trying to open.
+   * @param selectQuery
+   *        the SELECT query to use to fetch the rows.
+   *
+   * @return a promise that resolves to an array of rows. The promise will be
+   *         rejected if the read/fetch failed even after retrying.
+   */
+  getRowsFromDBWithoutLocks(path, description, selectQuery) {
+    let dbOptions = {
+      readOnly: true,
+      ignoreLockingMode: true,
+      path,
+    };
+
+    const RETRYLIMIT = 10;
+    const RETRYINTERVAL = 100;
+    return Task.spawn(function* innerGetRows() {
+      let rows = null;
+      for (let retryCount = RETRYLIMIT; retryCount && !rows; retryCount--) {
+        // Attempt to get the rows. If this succeeds, we will bail out of the loop,
+        // close the database in a failsafe way, and pass the rows back.
+        // If fetching the rows throws, we will wait RETRYINTERVAL ms
+        // and try again. This will repeat a maximum of RETRYLIMIT times.
+        let db;
+        let didOpen = false;
+        let exceptionSeen;
+        try {
+          db = yield Sqlite.openConnection(dbOptions);
+          didOpen = true;
+          rows = yield db.execute(selectQuery);
+        } catch (ex) {
+          if (!exceptionSeen) {
+            Cu.reportError(ex);
+          }
+          exceptionSeen = ex;
+        } finally {
+          try {
+            if (didOpen) {
+              yield db.close();
+            }
+          } catch (ex) {}
+        }
+        if (exceptionSeen) {
+          yield new Promise(resolve => setTimeout(resolve, RETRYINTERVAL));
+        }
+      }
+      if (!rows) {
+        throw new Error("Couldn't get rows from the " + description + " database.");
+      }
+      return rows;
+    });
+  },
+
   get _migrators() {
     return gMigrators ? gMigrators : gMigrators = new Map();
   },
 
   /*
    * Returns the migrator for the given source, if any data is available
    * for this source, or null otherwise.
    *