Bug 1272652 - Firefox fails to import bookmarks from Chrome if it also imports a large history. r=gijs draft
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 20 May 2016 17:00:43 +0200
changeset 370374 616e2fe71ab04404608f06475752e50c9d4b2b92
parent 370373 05adfc869721ffaa57a0b341bd56d78d9f5867f4
child 370375 2b4c4a72346cd2a4b8b76f3b24d99945c1eac8cd
push id19040
push usermak77@bonardo.net
push dateTue, 24 May 2016 15:30:51 +0000
reviewersgijs
bugs1272652
milestone49.0a1
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
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,18 +224,25 @@ 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() {
+    let doMigrate = Task.async(function*() {
       // TODO: use Map (for the items) and Set (for the resources)
       // once they are iterable.
       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]);
@@ -251,46 +260,55 @@ this.MigratorPrototype = {
         // TODO: (bug 449811).
         let migrationType = key, itemResources = value;
 
         notify("Migration:ItemBeforeMigrate", migrationType);
 
         let itemSuccess = false;
         for (let res of itemResources) {
           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");
               }
+              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 (key == MigrationUtils.resourceTypes.BOOKMARKS ||
+              key == 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/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");