Last Comment Bug 710264 - Chrome profile migrator: runBatched does all its work in an async callback
: Chrome profile migrator: runBatched does all its work in an async callback
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
Mentors:
Depends on: 718608
Blocks: 505192
  Show dependency treegraph
 
Reported: 2011-12-13 09:37 PST by Mano (::mano, needinfo? for any questions; not reading general bugmail)
Modified: 2012-03-28 05:32 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (8.26 KB, patch)
2011-12-19 02:23 PST, Makoto Kato [:m_kato]
mak77: review-
Details | Diff | Review

Description Mano (::mano, needinfo? for any questions; not reading general bugmail) 2011-12-13 09:37:45 PST
_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] 2011-12-13 09:43:19 PST
(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] 2011-12-13 09:44:28 PST
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] 2011-12-13 09:49:53 PST
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] 2011-12-19 02:23:39 PST
Created attachment 582771 [details] [diff] [review]
fix
Comment 5 Marco Bonardo [::mak] 2011-12-19 12:09:06 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2011-12-21 08:49:25 PST
Makoto, would you mind if I take this? I'm working on some other fix that requires fixing this bug.
Comment 7 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2012-03-28 05:32:38 PDT
Fixed by bug 718608.

Note You need to log in before you can comment on or make changes to this bug.