Closed
Bug 817232
Opened 12 years ago
Closed 12 years ago
Don't apply incoming deletions for special folders
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: rnewman, Assigned: rnewman)
Details
Attachments
(1 file)
3.18 KB,
patch
|
gps
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Should a deletion for a root happen to be downloaded, don't apply it. This might be a factor in Bug 670069.
Assignee | ||
Comment 1•12 years ago
|
||
If you take out the short-circuits, applying that deletion causes the same stack as in Bug 670069. Hmmm. I have an open question out to mak to see if there's a way to ask Places to ensure that special folders exist, recreating them if they got deleted somehow.
Comment 2•12 years ago
|
||
See bug 791562... I think we came to similar conclusions about what is corrupting databases... If by special folders you mean the roots, removing or moving a root is the perfect way to kill the bookmarks service, thus I'm adding protection to them so nothing can touch them
Comment 3•12 years ago
|
||
And IIRC we have a bug about being able to come back to life from a missing root... but it's lost in the past history.
Assignee | ||
Comment 4•12 years ago
|
||
(In reply to Marco Bonardo [:mak] from comment #2) > See bug 791562... I think we came to similar conclusions about what is > corrupting databases... If by special folders you mean the roots, removing > or moving a root is the perfect way to kill the bookmarks service Still confusing, because Sync shouldn't be able to get into this situation. It means a deliberate deletion has happened somewhere (e.g., Fennec), or there's a weird bug we don't know about. > thus I'm > adding protection to them so nothing can touch them Good.
Comment 5•12 years ago
|
||
Comment on attachment 687350 [details] [diff] [review] Proposed patch. v1 Review of attachment 687350 [details] [diff] [review]: ----------------------------------------------------------------- Hesitant r+ for testing shortcomings. I'm not sure how easy that is to address. ::: services/sync/tests/unit/test_bookmark_store.js @@ +22,5 @@ > + record.deleted = true; > + store.applyIncoming(record); > + > + // This will fail painfully in getItemType if the deletion worked. > + engine._buildGUIDMap(); This seems a bit fragile to me. I would prefer for the test to test pre- and post-conditions explicitly. i.e. that folder exists before and after the delete record is processed.
Attachment #687350 -
Flags: review?(gps) → review+
Assignee | ||
Comment 6•12 years ago
|
||
Landed with additional test checks: https://hg.mozilla.org/integration/mozilla-inbound/rev/44799b79143d I'll request uplift when this lands. Comment for release drivers: we don't know how, but evidence seems to suggest that Places bookmark roots are being deleted. Sync (on desktop or mobile) might be the vehicle or cause for this. Right now Sync will apply a deletion if one arrives on the server, and we don't want that to occur. This is a very simple patch to reject those deletions, and log accordingly.
status-firefox17:
--- → affected
status-firefox18:
--- → affected
status-firefox19:
--- → affected
tracking-firefox18:
--- → ?
tracking-firefox19:
--- → ?
Target Milestone: --- → mozilla20
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/44799b79143d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 8•12 years ago
|
||
Not a recent regression from my read of the bug (no need to track), but we'd definitely consider an uplift based upon a low risk evaluation. Please nominate as soon as you get the chance.
Assignee | ||
Comment 9•12 years ago
|
||
Comment on attachment 687350 [details] [diff] [review] Proposed patch. v1 [Approval Request Comment] Bug caused by (feature/regressing bug #): -- N/A; guarding against potentially harmful behavior. User impact if declined: -- Possible cataclysmic bookmark damage from unknown source. Testing completed (on m-c, etc.): -- Landed on m-c; there's automated test coverage for this. Still waiting for TPS to get fixed so we can get real-world tests, but my everyday Nightly profile will break hard if the patch is bad! Risk to taking this patch (and alternatives if risky): -- Zero; covers an edge case only. String or UUID changes made by this patch: -- None.
Attachment #687350 -
Flags: approval-mozilla-beta?
Attachment #687350 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Attachment #687350 -
Flags: approval-mozilla-beta?
Attachment #687350 -
Flags: approval-mozilla-beta+
Attachment #687350 -
Flags: approval-mozilla-aurora?
Attachment #687350 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 10•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/b5fab1c304d2 https://hg.mozilla.org/releases/mozilla-aurora/rev/b89a1215566a
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
•