_migrateBookmarks : function Chrome_migrateBookmarks() { this._notifyStart(MIGRATE_BOOKMARKS); try { PlacesUtils.bookmarks.runInBatchMode({ _self : this, runBatched : function (aUserData) { let migrator = this._self; let file = Cc[LOCAL_FILE_CID].createInstance(Ci.nsILocalFile); file.initWithPath(migrator._paths.bookmarks); NetUtil.asyncFetch(file, function(aInputStream, aResultCode) { ... } This makes the batch useless. Similar issue seem to apply to migrateHistory.

(In reply to Mano from comment #0) > Similar issue seem to apply to migrateHistory. afaik migrateHistory doesn't use a batch (it can't)

hm, ugh something wrong happened here, I explicitly said to not use a batch, jdm made a patch not using a batch but looks like makoto kato pushed a previous version of the patch.

So, to clarify, we should batch bookmarks, but not history, since updatePlaces API is async it can't be batched.

Comment on attachment 582771 [details] [diff] [review] fix Review of attachment 582771 [details] [diff] [review]: ----------------------------------------------------------------- the error handling is still messed up with all the callbacks, the external try catch won't catch internal exceptions, thus we may never notify error or completion in some cases. I would like that to be a bit more robust. ::: browser/components/migration/src/ChromeProfileMigrator.js @@ +191,4 @@ > > + NetUtil.asyncFetch(file, function(aInputStream, aResultCode) { > + if (!Components.isSuccessCode(aResultCode)) { > + migrator._notifyCompleted(MIGRATE_BOOKMARKS); should we _notifyError here? can Chrome have a profile without a Bookmarks file if there is no bookmark? @@ +201,5 @@ > + { charset : "UTF-8" }); > + > + PlacesUtils.bookmarks.runInBatchMode({ > + _self : migrator, > + _roots : JSON.parse(bookmarkJSON).roots, the parse may fail if the file is malformed/invalid JSON, I'm not sure if readInputStreamToString may fail too. In both cases the external try/catch can't catch these failures, and we won't notify error nor completion. the error handling inside the callback should be a bit more robust. @@ +203,5 @@ > + PlacesUtils.bookmarks.runInBatchMode({ > + _self : migrator, > + _roots : JSON.parse(bookmarkJSON).roots, > + runBatched : function (aUserData) { > + let migrator = this._self; nit: we don't use aUserData, so you may avoid defining it @@ +209,4 @@ > > // Importing bookmark bar items > if (roots.bookmark_bar.children && > + roots.bookmark_bar.children.length > 0) { bogus indentation change? @@ +305,4 @@ > > + handleCompletion : function(aReason) { > + this._db.asyncClose(); > + this._self._notifyCompleted(MIGRATE_HISTORY); While here, I noticed we should probably check aReason and if it's an error we should _notifyError too, since we were unable to read the source database

Makoto, would you mind if I take this? I'm working on some other fix that requires fixing this bug.

Fixed by bug 718608.