Use asynchronous getCharsetForURI in BookmarkJSONUtils.jsm

RESOLVED FIXED in mozilla24

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: raymondlee, Assigned: marcos)

Tracking

Trunk
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Updated

5 years ago
Blocks: 699850
(Reporter)

Updated

5 years ago
Assignee: nobody → marcos
(Reporter)

Updated

5 years ago
Blocks: 818593
(Assignee)

Comment 1

5 years ago
Created attachment 739290 [details] [diff] [review]
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.
Attachment #739290 - Flags: review?(raymond)
Attachment #739290 - Flags: review?(mak77)
(Reporter)

Updated

5 years ago
Attachment #739290 - Flags: review?(raymond)
(Reporter)

Comment 2

5 years ago
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.
Attachment #739290 - Flags: review?(mak77) → feedback-
(Reporter)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
Created attachment 741107 [details] [diff] [review]
Use asynchronous getCharsetForURI in BookmarkJSONUtils.jsm
Attachment #739290 - Attachment is obsolete: true
Attachment #741107 - Flags: feedback?(raymond)
(Reporter)

Comment 4

5 years ago
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.
(Reporter)

Updated

5 years ago
Attachment #741107 - Flags: review?(mak77)
Attachment #741107 - Flags: feedback?(raymond)
Attachment #741107 - Flags: feedback+
(Reporter)

Updated

5 years ago
Blocks: 854761
(Reporter)

Comment 5

5 years ago
mak: could you review this patch please?
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
Attachment #741107 - Flags: review?(mak77) → review+
(Reporter)

Comment 7

5 years ago
Marcos: please ensure that the patch can apply cleanly to the latest m-c source.
(Assignee)

Comment 8

5 years ago
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.
(Reporter)

Comment 9

5 years ago
(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
(Assignee)

Comment 10

5 years ago
Created attachment 749636 [details] [diff] [review]
Updated patch to apply cleanly to the source. Regular patch format.
(Reporter)

Comment 11

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #749629 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #741107 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(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?
(Assignee)

Comment 16

5 years ago
Thanks Ryan. I'll look into the problem to see what may be causing the problem.
(Assignee)

Comment 17

5 years ago
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?
(Reporter)

Comment 18

5 years ago
(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
(Reporter)

Comment 19

5 years ago
(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?
Flags: needinfo?(mak77)
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) ...
Flags: needinfo?(mak77)
(Reporter)

Comment 21

5 years ago
(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
Flags: needinfo?
(Reporter)

Comment 22

5 years ago
Created attachment 753237 [details] [diff] [review]
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
Attachment #749636 - Attachment is obsolete: true
Flags: needinfo?
(Reporter)

Comment 23

5 years ago
Created attachment 753239 [details] [diff] [review]
v5

fixed some typo
Attachment #753237 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Keywords: checkin-needed
(Reporter)

Comment 24

5 years ago
(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
(Reporter)

Comment 25

5 years ago
Created attachment 754675 [details] [diff] [review]
Patch for checkin

Just updated the commit description
Attachment #753239 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/f7cf513cd008
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.