987682 - DumpBookmarks() in bookmarks.jsm hangs due to invalid JSON data

Status: RESOLVED FIXED

(Testing Graveyard :: TPS, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The DumpBookmarks() method in bookmarks.jsm fails because of a broken promise handling which causes the JSON data to be empty (invalid). That's why an exception gets raised and cb.wait() is never called.

http://mxr.mozilla.org/mozilla-central/source/services/sync/tps/extensions/tps/resource/modules/bookmarks.jsm#23

The problem here is that the call to BookmarkJSONUtils.serializeNodeAsJSONToOutputStream() doesn't stop the execution of the task as is should given the yield keyword. So writer.value has still its initial value '', and JSON.parse('') causes an exception.

You can see that when adding dump() statements to writer.write() and after the yield line.

Marco, do you have an idea what could go wrong here?

Comment 1

[David Teller [:Yoric] - still alive but not very active]

By any chance, could we take the opportunity to get rid of this cb.wait()?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

TPS is handling everything synchronously. So there is no easy way to do that. Anything else which would help here?

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Why exactly is TPS handling everything synchronously, I wonder?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Jgriffin has written TPS. So he can tell more. As of now we don't have the time to entirely rewrite tps. We have to get all the tests working, and this is somewhat a blocker for me.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Here the minimized code which should return a serialized JSON string for the bookmarks. Instead it is empty.

Components.utils.import("resource://gre/modules/BookmarkJSONUtils.jsm");

let writer = {
 value: "",
 write: function PlacesItem__dump__write(aStr, aLen) { this.value += aStr; }
};

let options = PlacesUtils.history.getNewQueryOptions();
options.queryType = Ci.nsINavHistoryQueryOptions.QUERY_TYPE_BOOKMARKS;
let query = PlacesUtils.history.getNewQuery();
query.setFolders([PlacesUtils.placesRootId], 1);
let root = PlacesUtils.history.executeQuery(query, options).root;
root.containerOpen = true;

BookmarkJSONUtils.serializeNodeAsJSONToOutputStream(root, writer, true, false).then(() => {
  window.alert(writer.value);
});

Comment 6

[Marco Bonardo [:mak]]

Actually, I would like to completely kill serializeNodeAsJSONToOutputStream API, it sucks and we are not going to spend time on it.

In this case looks like you want to just dump all of the bookmarks, basically dump the bookmarks.json... you should then use the new getBookmarksTree() util and json.stringify the bookmarks object out of it

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sounds fair enough. I will remove it. The alternative works perfectly. Thank you Marco!

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

This patch fixes the hang and also makes sure that we do not hang anymore in case an exception gets thrown inside of the then method. Given that the output it not that important we should not call cb(ex) but simply log the error message to the console.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ups, kill the last sentence. It's not what the patch contains. If you want that please feel free to request. I would be happy with cb(ex) given that it should not fail.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The remaining hangs in cb.wait() I will get fixed in bug 981848.

Comment 11

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8396511 [details] [diff] [review]
Patch v1

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

It probably makes sense to log the error, but I don't have a strong opinion, since as you said, the error isn't really important to TPS.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/893c003daf13

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/893c003daf13

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Andrei, can you please verify this fix? I would like to get it backported, but I would like to see at least one other confirmation it works correctly. And given that you also have seen this bug yesterday... Thanks.

Comment 15

[Andrei Eftimie]

I confirm that I don't see any more hangs with this fix.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8396511 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): None
User impact if declined: None, but automated tests will hang
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): None
String or IDL/UUID changes made by this patch:None

The backport for beta would need a new patch given that the new method used here, will not be available. But for now we are not clear, if we need this backport at all.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a84920158dd4