860625 - Use asynchronous getCharsetForURI in BookmarkJSONUtils.jsm

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Raymond Lee [:raymondlee]]

https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/BookmarkJSONUtils.jsm#628

Comment 1

[Marcos Aruj]

Attachment: Use asynchronous getCharsetForURI in BookmarkJSONUtils.jsm

Hi guys,

Here's a patch for this bug. Let me know if there are any issues.

Cheers,

Marcos.

Comment 2

[Raymond Lee [:raymondlee]]

Comment on attachment 739290 [details] [diff] [review]
Use asynchronous getCharsetForURI in BookmarkJSONUtils.jsm

@@ -497,29 +497,31 @@ let BookmarkNode = {
    * @param   aResolveShortcuts
    *          Converts folder shortcuts into actual folders.
    * @param   aExcludeItems
    *          An array of item ids that should not be written to the backup.
    */
   serializeAsJSONToOutputStream: function BN_serializeAsJSONToOutputStream(
     aNode, aStream, aIsUICommand, aResolveShortcuts, aExcludeItems) {
 
-    // Serialize to stream
-    let array = [];
-    if (this._appendConvertedNode(aNode, null, array, aIsUICommand,
-                                  aResolveShortcuts, aExcludeItems)) {
-      let json = JSON.stringify(array[0]);
-      aStream.write(json, json.length);
-    } else {
-      throw Cr.NS_ERROR_UNEXPECTED;
-    }
+    Task.spawn(function() {
+      // Serialize to stream
+      let array = [];
+      if (yield this._appendConvertedNode(aNode, null, array, aIsUICommand,
+                                          aResolveShortcuts, aExcludeItems)) {
+        let json = JSON.stringify(array[0]);
+        aStream.write(json, json.length);
+      } else {
+        throw Cr.NS_ERROR_UNEXPECTED;
+      }
+    }.bind(this));

You should |return Task.spawn| here, otherwise, the callers won't get the |Promise| object.

+    }.bind(this));
   },
 
   _appendConvertedNode: function BN__appendConvertedNode(
-    bNode, aIndex, aArray, aIsUICommand, aResolveShortcuts, aExcludeItems) {
+      bNode, aIndex, aArray, aIsUICommand, aResolveShortcuts, aExcludeItems) {

Please restore the indentation.

@@ -537,17 +539,17 @@ let BookmarkNode = {
       // This will throw if we try to serialize an invalid url and it does
       // not make sense saving a wrong or corrupt uri node.
       try {
         NetUtil.newURI(bNode.uri);
       } catch (ex) {
         return false;
       }
 
-      this._addURIProperties(bNode, node, aIsUICommand);
+      Task.spawn(function() { yield this._addURIProperties(bNode, node, aIsUICommand); }.bind(this));

If you do this here, the _appendConvertedNode() wouldn't wait for the above code to complete and return true or false.  You should use return Task.spawn to write the code and use throw new Task.Result for the booleans.

@@ -556,18 +558,20 @@ let BookmarkNode = {
       if ((parent && parent.itemId == PlacesUtils.tagsFolderId) ||
           (grandParent && grandParent.itemId == PlacesUtils.tagsFolderId))
         return false;
 
       this._addSeparatorProperties(bNode, node);
     }
 
     if (!node.feedURI && node.type == PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER) {
-      return this._appendConvertedComplexNode(node, bNode, aArray, aIsUICommand,
-                                              aResolveShortcuts, aExcludeItems);
+      return Task.spawn(function() {
+        throw new Task.Result(yield this._appendConvertedComplexNode(node, bNode, aArray, aIsUICommand,
+                                                                     aResolveShortcuts, aExcludeItems));
+      }.bind(this));

You can just use yield here if the _appendConvertedNode() method is wrapped by return Task.spawn


@@ -685,19 +689,21 @@ let BookmarkNode = {
       let wasOpen = aSourceNode.containerOpen;
       if (!wasOpen)
         aSourceNode.containerOpen = true;
       let cc = aSourceNode.childCount;
       for (let i = 0; i < cc; ++i) {
         let childNode = aSourceNode.getChild(i);
         if (aExcludeItems && aExcludeItems.indexOf(childNode.itemId) != -1)
           continue;
-        this._appendConvertedNode(aSourceNode.getChild(i), i, children,
-                                  aIsUICommand, aResolveShortcuts,
-                                  aExcludeItems);
+        Task.spawn(function() {
+          yield this._appendConvertedNode(aSourceNode.getChild(i), i, children,
+                                          aIsUICommand, aResolveShortcuts,
+                                          aExcludeItems);
+        }.bind(this));

The _appendConvertedComplexNode would return a boolean before the above code complete, please fix that.

Comment 3

[Marcos Aruj]

Attachment: Use asynchronous getCharsetForURI in BookmarkJSONUtils.jsm

Comment 4

[Raymond Lee [:raymondlee]]

Comment on attachment 741107 [details] [diff] [review]
Use asynchronous getCharsetForURI in BookmarkJSONUtils.jsm

> let BookmarkNode = {
>   /**
>    * Serializes the given node (and all its descendents) as JSON
>@@ -497,81 +497,85 @@ let BookmarkNode = {
>    * @param   aResolveShortcuts
>    *          Converts folder shortcuts into actual folders.
>    * @param   aExcludeItems
>    *          An array of item ids that should not be written to the backup.
>    */
>   serializeAsJSONToOutputStream: function BN_serializeAsJSONToOutputStream(
>       aNode, aStream, aIsUICommand, aResolveShortcuts, aExcludeItems) {
> 

Please add @return

Looks good to me.

Comment 5

[Raymond Lee [:raymondlee]]

mak: could you review this patch please?

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 741107 [details] [diff] [review]
Use asynchronous getCharsetForURI in BookmarkJSONUtils.jsm

Review of attachment 741107 [details] [diff] [review]:
-----------------------------------------------------------------

It looks fine to me, thanks

Comment 7

[Raymond Lee [:raymondlee]]

Marcos: please ensure that the patch can apply cleanly to the latest m-c source.

Comment 8

[Marcos Aruj]

Attachment: Updated patch to apply cleanly to the source

Hi guys,
Here's an updated to patch that applies cleanly to the latest sources.

Let me know if everything works.

Thanks.

Comment 9

[Raymond Lee [:raymondlee]]

(In reply to Marcos Aruj from comment #8)
> Created attachment 749629 [details] [diff] [review]
> Updated patch to apply cleanly to the source
> 
> Hi guys,
> Here's an updated to patch that applies cleanly to the latest sources.
> 
> Let me know if everything works.
> 
> Thanks.

I can't apply this patch for some reasons. The format doesn't look like the first one?

Raymonds-MacBook-Pro:mozilla-central raymondlee$ hg import --no-commit bug-860625.patch 
applying bug-860625.patch
unable to find 'components/places/BookmarkJSONUtils.jsm' for patching
4 out of 4 hunks FAILED -- saving rejects to file components/places/BookmarkJSONUtils.jsm.rej
abort: patch failed to apply

Comment 10

[Marcos Aruj]

Attachment: Updated patch to apply cleanly to the source. Regular patch format.

Comment 11

[Raymond Lee [:raymondlee]]

(In reply to Marcos Aruj from comment #10)
> Created attachment 749636 [details] [diff] [review]
> Updated patch to apply cleanly to the source. Regular patch format.

I have pushed to try.
https://tbpl.mozilla.org/?tree=Try&rev=074fe733eeac

Please go to the link to check the results. Once everything looks ok, just add "checkin-needed" in keywords field.

Comment 12

[Raymond Lee [:raymondlee]]

ubuntu64 always has orange on it
https://tbpl.mozilla.org/?tree=Try&rev=6a3e07b945a9
https://tbpl.mozilla.org/?tree=Try&rev=29dd565fc834

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f1f058843cc

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for Linux64 xpcshell failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a1084d464ba

https://tbpl.mozilla.org/php/getParsedLog.php?id=23162814&tree=Mozilla-Inbound

Comment 15

[Ryan VanderMeulen [:RyanVM]]

(In reply to Raymond Lee [:raymondlee] from comment #12)
> ubuntu64 always has orange on it
> https://tbpl.mozilla.org/?tree=Try&rev=6a3e07b945a9
> https://tbpl.mozilla.org/?tree=Try&rev=29dd565fc834

Uh, what?

Comment 16

[Marcos Aruj]

Thanks Ryan. I'll look into the problem to see what may be causing the problem.

Comment 17

[Marcos Aruj]

Hi guys,

I've tried building mozilla with this patch on my Ubuntu64 bit system and I get no errors. Not sure what may be causing the problems on the TRY server. However, I see this in a comment right above the test that fails (3rd line):

//XXX not working on Linux unit boxes. Could be filestats caching issue.
//do_check_true(profileBookmarksHTMLFile.lastModifiedTime > lastMod);
do_check_neq(profileBookmarksHTMLFile.fileSize, fileSize);

Maybe the problem is caused by the same thing, since it is working correctly on other OS. Wyt?

Comment 18

[Raymond Lee [:raymondlee]]

(In reply to Marcos Aruj from comment #17)
> Hi guys,
> 
> I've tried building mozilla with this patch on my Ubuntu64 bit system and I
> get no errors. Not sure what may be causing the problems on the TRY server.
> However, I see this in a comment right above the test that fails (3rd line):
> 
> //XXX not working on Linux unit boxes. Could be filestats caching issue.
> //do_check_true(profileBookmarksHTMLFile.lastModifiedTime > lastMod);
> do_check_neq(profileBookmarksHTMLFile.fileSize, fileSize);
> 
> Maybe the problem is caused by the same thing, since it is working correctly
> on other OS. Wyt?

I have commented that line out and submitted to try.  Lets see what happens.
https://tbpl.mozilla.org/?tree=Try&rev=915e98383d4a

Comment 19

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #18)
> (In reply to Marcos Aruj from comment #17)
> > Hi guys,
> > 
> > I've tried building mozilla with this patch on my Ubuntu64 bit system and I
> > get no errors. Not sure what may be causing the problems on the TRY server.
> > However, I see this in a comment right above the test that fails (3rd line):
> > 
> > //XXX not working on Linux unit boxes. Could be filestats caching issue.
> > //do_check_true(profileBookmarksHTMLFile.lastModifiedTime > lastMod);
> > do_check_neq(profileBookmarksHTMLFile.fileSize, fileSize);
> > 
> > Maybe the problem is caused by the same thing, since it is working correctly
> > on other OS. Wyt?
> 
> I have commented that line out and submitted to try.  Lets see what happens.
> https://tbpl.mozilla.org/?tree=Try&rev=915e98383d4a

The tests passed with that line commented out. 

Mak: any suggestions what to do?

Comment 20

[Marco Bonardo [:mak]]

just skip those tests on linux, if there was already a known problem with stats caching it is likely causing the issue, file size and flastmod are both cached afaik. You should be able to skip that part using this:

let isLinux = "@mozilla.org/gnome-gconf-service;1" in Components.classes;
if (!isLinux) ...

Comment 21

[Raymond Lee [:raymondlee]]

(In reply to Marco Bonardo [:mak] from comment #20)
> just skip those tests on linux, if there was already a known problem with
> stats caching it is likely causing the issue, file size and flastmod are
> both cached afaik. You should be able to skip that part using this:
> 
> let isLinux = "@mozilla.org/gnome-gconf-service;1" in Components.classes;
> if (!isLinux) ...

I have submitted a patch with the above check to try.
https://tbpl.mozilla.org/?tree=Try&rev=99fea66e6cf7

Marcos: please check the results when it's done

Comment 22

[Raymond Lee [:raymondlee]]

Attachment: v4

Please note that the commented out line doesn't work on Mac OSX as well, therefore, I have added another check to the patch.

// do_check_true(profileBookmarksHTMLFile.lastModifiedTime > lastMod);

Pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=7610519db5a5

Comment 23

[Raymond Lee [:raymondlee]]

Attachment: v5

fixed some typo

Comment 24

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #22)
> Created attachment 753237 [details] [diff] [review]
> v4
> 
> Pushed to try
> https://tbpl.mozilla.org/?tree=Try&rev=7610519db5a5

Passed try

Comment 25

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Just updated the commit description

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7cf513cd008

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f7cf513cd008