Closed Bug 1166853 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
mozilla41
Iteration:
41.1 - May 25
Tracking Status
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- fixed
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 1 obsolete file)

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...
Summary: aLivemark is undefined in bookmarks.js → aLivemark is undefined in bookmarks.js, hangs waiting to try applying a livemark
Attached patch patch v1 (obsolete) — Splinter Review
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)
Attached patch patch v1.1Splinter Review
with exception logging
Attachment #8608271 - Attachment is obsolete: true
Attachment #8608271 - Flags: review?(rnewman)
Attachment #8608275 - Flags: review?(rnewman)
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
ok, I will try to apply the suggestion in comment 3 to make a test for this.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Points: --- → 3
Flags: firefox-backlog+
Iteration: --- → 41.1 - May 25
Flags: qe-verify?
(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.
looks like we already have such a test in test_livemark_invalid, but it doesn't catch the mistake...
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.
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.
Attachment #8608271 - Attachment is obsolete: true
I'm fine with you landing this without a test. Thanks for making the effort to do so!
so, care to review the patch? :)
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+
very hard to verify :( I can do that though once I get a nightly with the fix and my Sync starts working again
Flags: qe-verify? → qe-verify-
https://hg.mozilla.org/mozilla-central/rev/1acb10da2d7e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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.