Bug 1272652 - Firefox fails to import bookmarks from Chrome if it also imports a large history. r=gijs
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 24 May 2016 17:29:30 +0200
changeset 337688 59cf6bd00ec8f4783fcd0ca652df3a18b878a79d
parent 337687 199fac6509aa6aaa249a178d6bd39e87f3061a67
child 337689 13da4ea2b371b20490eaf301a9940d1e34005da3
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgijs
bugs1272652
milestone49.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1272652 - Firefox fails to import bookmarks from Chrome if it also imports a large history. r=gijs MozReview-Commit-ID: 3w5TIPi2S8d
browser/components/migration/ChromeProfileMigrator.js
browser/components/migration/MigrationUtils.jsm
browser/components/migration/tests/marionette/test_refresh_firefox.py
toolkit/modules/Sqlite.jsm
--- a/browser/components/migration/ChromeProfileMigrator.js
+++ b/browser/components/migration/ChromeProfileMigrator.js
@@ -26,17 +26,18 @@ Cu.import("resource://gre/modules/NetUti
 Cu.import("resource://gre/modules/FileUtils.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!
  */
@@ -285,64 +286,73 @@ function GetHistoryResource(aProfileFold
   let historyFile = aProfileFolder.clone();
   historyFile.append("History");
   if (!historyFile.exists())
     return null;
 
   return {
     type: MigrationUtils.resourceTypes.HISTORY,
 
-    migrate: function(aCallback) {
-      let dbConn = Services.storage.openUnsharedDatabase(historyFile);
-      let stmt = dbConn.createAsyncStatement(
-        "SELECT url, title, last_visit_time, typed_count FROM urls WHERE hidden = 0");
+    migrate(aCallback) {
+      Task.spawn(function* () {
+        let db = yield Sqlite.openConnection({
+          path: historyFile.path
+        });
 
-      stmt.executeAsync({
-        handleResult : function(aResults) {
-          let places = [];
-          for (let row = aResults.getNextRow(); row; row = aResults.getNextRow()) {
-            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;
+        let rows = yield db.execute(`SELECT url, title, last_visit_time, typed_count
+                                     FROM urls WHERE hidden = 0`);
+        yield db.close();
 
-              places.push({
-                uri: NetUtil.newURI(row.getResultByName("url")),
-                title: row.getResultByName("title"),
-                visits: [{
-                  transitionType: transType,
-                  visitDate: chromeTimeToDate(
-                               row.getResultByName(
-                                 "last_visit_time")) * 1000,
-                }],
-              });
-            } catch (e) {
-              Cu.reportError(e);
-            }
-          }
+        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;
 
-          try {
-            PlacesUtils.asyncHistory.updatePlaces(places);
+            places.push({
+              uri: NetUtil.newURI(row.getResultByName("url")),
+              title: row.getResultByName("title"),
+              visits: [{
+                transitionType: transType,
+                visitDate: chromeTimeToDate(
+                             row.getResultByName(
+                               "last_visit_time")) * 1000,
+              }],
+            });
           } catch (e) {
             Cu.reportError(e);
           }
-        },
-
-        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);
+        if (places.length > 0) {
+          yield new Promise((resolve, reject) => {
+            PlacesUtils.asyncHistory.updatePlaces(places, {
+              _success: false,
+              handleResult: function() {
+                // Importing any entry is considered a successful import.
+                this._success = true;
+              },
+              handleError: function() {},
+              handleCompletion: function() {
+                if (this._success) {
+                  resolve();
+                } else {
+                  reject(new Error("Couldn't add visits"));
+                }
+              }
+            });
+          });
         }
-      });
-      stmt.finalize();
+      }).then(() => { aCallback(true); },
+              ex => {
+                Cu.reportError(ex);
+                aCallback(false);
+              });
     }
   };
 }
 
 function GetCookiesResource(aProfileFolder) {
   let cookiesFile = aProfileFolder.clone();
   cookiesFile.append("Cookies");
   if (!cookiesFile.exists())
--- a/browser/components/migration/MigrationUtils.jsm
+++ b/browser/components/migration/MigrationUtils.jsm
@@ -14,16 +14,18 @@ Cu.import("resource://gre/modules/XPCOMU
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/AppConstants.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "BookmarkHTMLUtils",
                                   "resource://gre/modules/BookmarkHTMLUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "PromiseUtils",
+                                  "resource://gre/modules/PromiseUtils.jsm");
 
 var gMigrators = null;
 var gProfileStartup = null;
 var gMigrationBundle = null;
 
 XPCOMUtils.defineLazyGetter(this, "gAvailableMigratorKeys", function() {
   if (AppConstants.platform == "win") {
     return [
@@ -222,75 +224,88 @@ this.MigratorPrototype = {
   migrate: function MP_migrate(aItems, aStartup, aProfile) {
     let resources = this._getMaybeCachedResources(aProfile);
     if (resources.length == 0)
       throw new Error("migrate called for a non-existent source");
 
     if (aItems != Ci.nsIBrowserProfileMigrator.ALL)
       resources = resources.filter(r => aItems & r.type);
 
+    // Used to periodically give back control to the main-thread loop.
+    let unblockMainThread = function () {
+      return new Promise(resolve => {
+        Services.tm.mainThread.dispatch(resolve, Ci.nsIThread.DISPATCH_NORMAL);
+      });
+    };
+
     // Called either directly or through the bookmarks import callback.
-    function doMigrate() {
-      // TODO: use Map (for the items) and Set (for the resources)
-      // once they are iterable.
+    let doMigrate = Task.async(function*() {
       let resourcesGroupedByItems = new Map();
       resources.forEach(function(resource) {
-        if (resourcesGroupedByItems.has(resource.type))
-          resourcesGroupedByItems.get(resource.type).push(resource);
-        else
-          resourcesGroupedByItems.set(resource.type, [resource]);
+        if (!resourcesGroupedByItems.has(resource.type)) {
+          resourcesGroupedByItems.set(resource.type, new Set());
+        }
+        resourcesGroupedByItems.get(resource.type).add(resource)
       });
 
       if (resourcesGroupedByItems.size == 0)
         throw new Error("No items to import");
 
       let notify = function(aMsg, aItemType) {
         Services.obs.notifyObservers(null, aMsg, aItemType);
       }
 
       notify("Migration:Started");
       for (let [key, value] of resourcesGroupedByItems) {
-        // TODO: (bug 449811).
+        // Workaround bug 449811.
         let migrationType = key, itemResources = value;
 
         notify("Migration:ItemBeforeMigrate", migrationType);
 
         let itemSuccess = false;
         for (let res of itemResources) {
+          // Workaround bug 449811.
           let resource = res;
+          let completeDeferred = PromiseUtils.defer();
           let resourceDone = function(aSuccess) {
-            let resourceIndex = itemResources.indexOf(resource);
-            if (resourceIndex != -1) {
-              itemResources.splice(resourceIndex, 1);
-              itemSuccess |= aSuccess;
-              if (itemResources.length == 0) {
-                resourcesGroupedByItems.delete(migrationType);
-                notify(itemSuccess ?
-                       "Migration:ItemAfterMigrate" : "Migration:ItemError",
-                       migrationType);
-                if (resourcesGroupedByItems.size == 0)
-                  notify("Migration:Ended");
+            itemResources.delete(resource);
+            itemSuccess |= aSuccess;
+            if (itemResources.size == 0) {
+              notify(itemSuccess ?
+                     "Migration:ItemAfterMigrate" : "Migration:ItemError",
+                     migrationType);
+              resourcesGroupedByItems.delete(migrationType);
+              if (resourcesGroupedByItems.size == 0) {
+                notify("Migration:Ended");
               }
             }
+            completeDeferred.resolve();
           }
 
-          Services.tm.mainThread.dispatch(function() {
-            // If migrate throws, an error occurred, and the callback
-            // (itemMayBeDone) might haven't been called.
-            try {
-              resource.migrate(resourceDone);
-            }
-            catch(ex) {
-              Cu.reportError(ex);
-              resourceDone(false);
-            }
-          }, Ci.nsIThread.DISPATCH_NORMAL);
+          // If migrate throws, an error occurred, and the callback
+          // (itemMayBeDone) might haven't been called.
+          try {
+            resource.migrate(resourceDone);
+          }
+          catch(ex) {
+            Cu.reportError(ex);
+            resourceDone(false);
+          }
+
+          // Certain resources must be ran sequentially or they could fail,
+          // for example bookmarks and history (See bug 1272652).
+          if (migrationType == MigrationUtils.resourceTypes.BOOKMARKS ||
+              migrationType == MigrationUtils.resourceTypes.HISTORY) {
+            yield completeDeferred.promise;
+          }
+
+          yield unblockMainThread();
         }
       }
-    }
+    });
 
     if (MigrationUtils.isStartupMigration && !this.startupOnlyMigrator) {
       MigrationUtils.profileStartup.doStartup();
 
       // If we're about to migrate bookmarks, first import the default bookmarks.
       // Note We do not need to do so for the Firefox migrator
       // (=startupOnlyMigrator), as it just copies over the places database
       // from another profile.
--- a/browser/components/migration/tests/marionette/test_refresh_firefox.py
+++ b/browser/components/migration/tests/marionette/test_refresh_firefox.py
@@ -225,27 +225,34 @@ class TestFirefoxRefresh(MarionetteTestC
         self.assertEqual(formHistoryCount, 1, "There should be only 1 entry in the form history")
 
     def checkCookie(self):
         cookieInfo = self.runCode("""
           try {
             let cookieEnum = Services.cookies.getCookiesFromHost(arguments[0]);
             let cookie = null;
             while (cookieEnum.hasMoreElements()) {
+              let hostCookie = cookieEnum.getNext();
+              hostCookie.QueryInterface(Ci.nsICookie2);
+              // getCookiesFromHost returns any cookie from the BASE host.
+              if (hostCookie.rawHost != arguments[0])
+                continue;
               if (cookie != null) {
                 return "more than 1 cookie! That shouldn't happen!";
               }
-              cookie = cookieEnum.getNext();
-              cookie.QueryInterface(Ci.nsICookie2);
+              cookie = hostCookie;
             }
             return {path: cookie.path, name: cookie.name, value: cookie.value};
           } catch (ex) {
             return "got exception trying to fetch cookie: " + ex;
           }
         """, script_args=[self._cookieHost])
+        if not isinstance(cookieInfo, dict):
+            self.fail(cookieInfo)
+            return
         self.assertEqual(cookieInfo['path'], self._cookiePath)
         self.assertEqual(cookieInfo['value'], self._cookieValue)
         self.assertEqual(cookieInfo['name'], self._cookieName)
 
     def checkSession(self):
         tabURIs = self.runCode("""
           return [... gBrowser.browsers].map(b => b.currentURI && b.currentURI.spec)
         """)
--- a/toolkit/modules/Sqlite.jsm
+++ b/toolkit/modules/Sqlite.jsm
@@ -6,17 +6,17 @@
 
 this.EXPORTED_SYMBOLS = [
   "Sqlite",
 ];
 
 const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
 
 // The time to wait before considering a transaction stuck and rejecting it.
-const TRANSACTIONS_QUEUE_TIMEOUT_MS = 120000 // 2 minutes
+const TRANSACTIONS_QUEUE_TIMEOUT_MS = 240000 // 4 minutes
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Timer.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "AsyncShutdown",
                                   "resource://gre/modules/AsyncShutdown.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Services",
                                   "resource://gre/modules/Services.jsm");