1166853 - aLivemark is undefined in bookmarks.js, causes "Sync hang" while waiting for an async callback

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Marco Bonardo [:mak]]

This is actually my fault since I touched this code:

http://mxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/bookmarks.js#772

772       PlacesUtils.livemarks.addLivemark(livemarkObj).then(
773         aLivemark => { spinningCb(null, [Components.results.NS_OK, aLivemark]) },
774         () => { spinningCb(null, [Components.results.NS_ERROR_UNEXPECTED, aLivemark]) }
775       );

in the error case aLivemark is not defined, we can't pass it out :(

I have no idea why adding the livemark fails, but we should invoke spinningCb!

The fix is trivial, but I'm not sure how to write a test, since I'm not sure how to sync in a way that will fail addLivemark...

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1

If you have any idea/example of how I can reach that code point with a test, let me know.

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.1

with exception logging

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8608271 [details] [diff] [review]
patch v1

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

Test: in test_bookmark_store.js, test_bookmark_create, create and pass in a "livemark" record. Ensure that it has a feedUri, but will be rejected by addLivemark.

There seem to be lots of ways to do that. Two good options that I can see:

* Make its parent a livemark.
* Give it a malformed GUID -- put whitespace in it, for example.

Comment 4

[Marco Bonardo [:mak]]

ok, I will try to apply the suggestion in comment 3 to make a test for this.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Richard Newman [:rnewman] from comment #3)
> There seem to be lots of ways to do that. Two good options that I can see:
> 
> * Make its parent a livemark.
> * Give it a malformed GUID -- put whitespace in it, for example.

The latter would throw immediately, not reject. the former should work though.

Comment 6

[Marco Bonardo [:mak]]

looks like we already have such a test in test_livemark_invalid, but it doesn't catch the mistake...

Comment 7

[Marco Bonardo [:mak]]

That's because Sync pre-emptively detects trying to insert a livemark in a livemark and skips the record. So I need to find another way to make Bookmarks.jsm unhappy without Sync noticing that.

Comment 8

[Marco Bonardo [:mak]]

I officially don't know how to write a test. Looks like my failure is due to some race condition for which promiseItemGuid doesn't find a given itemId, could be due to spinning, or due to bug 1012597.
I'm basically stuck with broken data returned by promiseItemGuid, and I don't think it's useful to simulate that in a test.

Comment 9

[Marco Bonardo [:mak]]

see https://bugzilla.mozilla.org/show_bug.cgi?id=1012597#c15

Comment 10

[Richard Newman [:rnewman]]

I'm fine with you landing this without a test. Thanks for making the effort to do so!

Comment 11

[Marco Bonardo [:mak]]

so, care to review the patch? :)

Comment 12

[Marco Bonardo [:mak]]

is it ok to throw there, or should I early return? I'm not sure if that makes a difference for Sync.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8608275 [details] [diff] [review]
patch v1.1

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

You obsoleted the one I r+ed :)

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/1acb10da2d7e

Comment 15

[Marco Bonardo [:mak]]

very hard to verify :( I can do that though once I get a nightly with the fix and my Sync starts working again

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1acb10da2d7e

Comment 17

[Marco Bonardo [:mak]]

Comment on attachment 8608275 [details] [diff] [review]
patch v1.1

Approval Request Comment
[Feature/regressing bug #]: bug 969318
[User impact if declined]: Syncing bookmarks can hang forever while waiting for a callback that will never run
[Describe test coverage new/current, TreeHerder]: tested locally, on Nightly
[Risks and why]: low risk, trivial change
[String/UUID change made/needed]: none

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8608275 [details] [diff] [review]
patch v1.1

Not a great user experience, taking it.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/032f91456953

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/34df9b9beee3