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)
Firefox
Sync
Tracking
()
People
(Reporter: mak, Assigned: mak)
References
Details
Attachments
(1 file, 1 obsolete file)
1.30 KB,
patch
|
rnewman
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•9 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•9 years ago
|
||
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•9 years ago
|
||
with exception logging
Attachment #8608271 -
Attachment is obsolete: true
Attachment #8608271 -
Flags: review?(rnewman)
Attachment #8608275 -
Flags: review?(rnewman)
Assignee | ||
Updated•9 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 3•9 years ago
|
||
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•9 years ago
|
||
ok, I will try to apply the suggestion in comment 3 to make a test for this.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Points: --- → 3
Flags: firefox-backlog+
Updated•9 years ago
|
Iteration: --- → 41.1 - May 25
Flags: qe-verify?
Assignee | ||
Comment 5•9 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•9 years ago
|
||
looks like we already have such a test in test_livemark_invalid, but it doesn't catch the mistake...
Assignee | ||
Comment 7•9 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•9 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 | ||
Comment 9•9 years ago
|
||
see https://bugzilla.mozilla.org/show_bug.cgi?id=1012597#c15
Assignee | ||
Updated•9 years ago
|
Attachment #8608271 -
Attachment is obsolete: true
Comment 10•9 years ago
|
||
I'm fine with you landing this without a test. Thanks for making the effort to do so!
Assignee | ||
Comment 11•9 years ago
|
||
so, care to review the patch? :)
Assignee | ||
Comment 12•9 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 13•9 years ago
|
||
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•9 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•9 years ago
|
Flags: qe-verify? → qe-verify-
Comment 16•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1acb10da2d7e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 17•9 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 18•9 years ago
|
||
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+
Updated•6 years ago
|
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.
Description
•