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

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
3 years ago
6 hours ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
mozilla41
Points:
3
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(firefox38 wontfix, firefox38.0.5 wontfix, firefox39 fixed, firefox40 fixed, firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Updated

3 years ago
status-firefox38: --- → wontfix
status-firefox38.0.5: --- → wontfix
status-firefox39: --- → affected
status-firefox40: --- → affected
status-firefox41: --- → affected
Summary: aLivemark is undefined in bookmarks.js → aLivemark is undefined in bookmarks.js, hangs waiting to try applying a livemark
(Assignee)

Comment 1

3 years ago
Created attachment 8608271 [details] [diff] [review]
patch v1

If you have any idea/example of how I can reach that code point with a test, let me know.
Attachment #8608271 - Flags: review?(rnewman)
(Assignee)

Comment 2

3 years ago
Created attachment 8608275 [details] [diff] [review]
patch v1.1

with exception logging
Attachment #8608271 - Attachment is obsolete: true
Attachment #8608271 - Flags: review?(rnewman)
Attachment #8608275 - Flags: review?(rnewman)
(Assignee)

Updated

3 years ago
Summary: aLivemark is undefined in bookmarks.js, hangs waiting to try applying a livemark → aLivemark is undefined in bookmarks.js, causes "Sync hang" while waiting for an async callback
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.
Attachment #8608271 - Attachment is obsolete: false
(Assignee)

Comment 4

3 years ago
ok, I will try to apply the suggestion in comment 3 to make a test for this.
(Assignee)

Updated

3 years ago
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Points: --- → 3
Flags: firefox-backlog+

Updated

3 years ago
Iteration: --- → 41.1 - May 25
Flags: qe-verify?
(Assignee)

Comment 5

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

Comment 6

3 years ago
looks like we already have such a test in test_livemark_invalid, but it doesn't catch the mistake...
(Assignee)

Comment 7

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

Comment 8

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

Updated

3 years ago
Attachment #8608271 - Attachment is obsolete: true
I'm fine with you landing this without a test. Thanks for making the effort to do so!
(Assignee)

Comment 11

3 years ago
so, care to review the patch? :)
(Assignee)

Comment 12

3 years ago
is it ok to throw there, or should I early return? I'm not sure if that makes a difference for Sync.
Comment on attachment 8608275 [details] [diff] [review]
patch v1.1

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

You obsoleted the one I r+ed :)
Attachment #8608275 - Flags: review?(rnewman) → review+
(Assignee)

Comment 15

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

Updated

3 years ago
Flags: qe-verify? → qe-verify-
https://hg.mozilla.org/mozilla-central/rev/1acb10da2d7e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(Assignee)

Comment 17

3 years ago
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
Attachment #8608275 - Flags: approval-mozilla-beta?
Attachment #8608275 - Flags: approval-mozilla-aurora?
Comment on attachment 8608275 [details] [diff] [review]
patch v1.1

Not a great user experience, taking it.
Attachment #8608275 - Flags: approval-mozilla-beta?
Attachment #8608275 - Flags: approval-mozilla-beta+
Attachment #8608275 - Flags: approval-mozilla-aurora?
Attachment #8608275 - Flags: approval-mozilla-aurora+
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.