Closed Bug 632287 Opened 9 years ago Closed 9 years ago

Remote bookmark has the same GUID as a local folder


(Firefox :: Sync, defect)

Not set





(Reporter: philikon, Assigned: rnewman)



(Whiteboard: [fixed in fx-sync][fixed in services])


(1 file)

Just saw this on blizzard's machine: Sync was trying to sync down two bookmarks whose GUIDs were assigned to folders locally. Not surprisingly, it was failing trying to change the URIs of the folders (changeBookmarkURI).

I'm perplexed at how we could end up with a situation like this. I don't think the caching places IDs (bug 627490) could be at fault here because he hadn't used the restore-from-backup feature. One collision is extremely unlikely already, two no longer smell like coincidence. So... humm...

One safety belt we can put in place in BookmarkStore.update() is to check the type of the incoming record against the local one. If they mismatch, we probably want to delete the local record and then call BookmarkStore.create() with the incoming one.
Assignee: nobody → rnewman
Implements delete-then-add logic and a test for this.
Attachment #514293 - Flags: review?(philipp)
Note that, despite having a patch that will avoid a failure here, we still don't know the root cause!
Whiteboard: [has patch][needs review philikon]
Comment on attachment 514293 [details] [diff] [review]
Proposed patch. v1

>+function test_bug_632287() {
>+  _("Ensure that handling a record that changes type causes deletion " +
>+    "then re-adding.");

Suggestion: more descriptive function name, e.g. test_mismatching_types().

>+  try {
>+    engine._syncStartup();

Presumably this is so that _lazyMap and _childrenToOrder() are available? Would be good to add a comment for that.
Attachment #514293 - Flags: review?(philipp)
Attachment #514293 - Flags: review+
Attachment #514293 - Flags: approval2.0?
Whiteboard: [has patch][needs review philikon] → [has patch][has review]
Risk v. reward for this patch?
Reward: an apparently unpredictable and undesirable Sync edge case, which has been once or twice in the wild, won't cause a user-visible error.

Risk: small; adds one branch to the 'update' case for the bookmarks engine. Passes CrossWeave and tests.

Detail: apparently *somehow* a record can be a folder locally and a bookmark on another machine. Chance of two GUID collisions is near-zero. This Shouldn't Happen®, but this patch handles it.
Comment on attachment 514293 [details] [diff] [review]
Proposed patch. v1

Attachment #514293 - Flags: approval2.0? → approval2.0+
This was landed in fx-sync default branch:

Richard, can you please land this in 1.6.x as well? Thanks!
Whiteboard: [has patch][has review] → [fixed in fx-sync]
My bad, thanks for catching Philipp. Will land this morning.
Merged to s-c:
Whiteboard: [fixed in fx-sync] → [fixed in fx-sync][fixed in services]
Merged to m-c:
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 636486
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.