710264 - Chrome profile migrator: runBatched does all its work in an async callback

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Asaf Romano (gone)]

_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.

Comment 1

[Marco Bonardo [:mak]]

(In reply to Mano from comment #0)
> Similar issue seem to apply to migrateHistory.

afaik migrateHistory doesn't use a batch (it can't)

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[Marco Bonardo [:mak]]

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

Comment 4

[Makoto Kato [:m_kato]]

Attachment: fix

Comment 5

[Marco Bonardo [:mak]]

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

Comment 6

[Asaf Romano (gone)]

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

Comment 7

[Asaf Romano (gone)]

Fixed by bug 718608.