Closed Bug 817232 Opened 8 years ago Closed 8 years ago
Don't apply incoming deletions for special folders
Should a deletion for a root happen to be downloaded, don't apply it. This might be a factor in Bug 670069.
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.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #687350 - Flags: review?(gps)
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
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.
(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 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+
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: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
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.
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.
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.