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

RESOLVED FIXED

Status

()

Firefox
Migration
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mano, Assigned: mano)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

_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.
Blocks: 505192
(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.

Updated

6 years ago
Assignee: nobody → m_kato
Created attachment 582771 [details] [diff] [review]
fix

Updated

6 years ago
Attachment #582771 - Flags: review?(mak77)
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
Attachment #582771 - Flags: review?(mak77) → review-
Makoto, would you mind if I take this? I'm working on some other fix that requires fixing this bug.
Fixed by bug 718608.
Assignee: m_kato → mano
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Depends on: 718608
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.