We don't currently account for the sync status when deduping bookmarks, meaning we might dupe a NORMAL (already on the server) bookmark to another NORMAL bookmark. We correctly upload a tombstone for the old GUID, but I wonder if the correct behavior is to only dupe NEW to NORMAL. This is how I imagine form autofill will work, except autofill spells NORMAL as "SYNCING". A complication here is that `pullChanges` marks NEW bookmarks as NORMAL when fetching changes before the sync. I think we can split this function into `pullChanges` and `markNewBookmarksAsSyncing`, as in bug 1323333. Semantically, does this seem right? I *think* this means we could avoid including `hasDupe` in the record, too, though we'd still need it in the local GUID map.

(In reply to Kit Cambridge (he/him) [:kitcambridge] (UTC-7) from comment #0) > Semantically, does this seem right? I think so, yeah. > I *think* this means we could avoid > including `hasDupe` in the record, too hrm - I guess so, yeah. > though we'd still need it in the local GUID map. Right, but that has slightly different semantics IIUC (and the fact they are both .hasDupe probably accounts for some confusion about these 2 different concepts with the same name)

Comment on attachment 8883749 [details] Bug 1366888 - Replace the GUID map with `PlacesSyncUtils.bookmarks.findDupeGuid`. This patch still dedupes NORMAL to NORMAL; I didn't change that part yet (`syncStatus = :syncStatus OR 1` are placeholders). But here's how deduping might look without the GUID map, and preserving existing semantics.

While killing the guidmap sounds awesome, TBH I'd be reluctant to land such a change in 56 given we hope to land Ed's risky patch too. I guess I could be convinced otherwise, but that seems like asking for hurt. Would it be possible to have a short-term fix that checks the sync status after looking up the existing map and doing this kind of change later?

(and I'd be fine to wait if that wouldn't work for reasons I can't yet see ;)

I thought about adding this to the existing map, and decided against it: 1) For local dupes (bookmarks with the same attributes and parent title, but in different parts of the tree), we currently store the first GUID we see as a string object, and set `.hasDupe = true`. I don't think that would work here. Once we dedupe a bookmark, we want to remove its GUID from the map, but keep local dupes around as candidates. We'd need to change the GUID map to store a list of matching GUIDs, and remove duped GUIDs from that list. I admit this refactoring would be smaller than rewriting how duping works, though, and likely less risky. 2) The logic is still tricky to test independently of the engine. An advantage of factoring this out into PSU is that we can add tests for `findDupeGuid`, and nail down the semantics before we start on bug 1305563 and bug 1323333. (Two-phase would be lovely to get in to 57, but probably unrealistic with everything else on our plate this quarter). If we wait, we probably won't be any worse off than we are now. 3) I already had the code for this attached to bug 1323333, and this is a conservative approach to landing those changes in pieces. That's not a good reason by itself, though. :-) If I can add more test coverage for duping, would you be open to having your mind changed? Or would you still prefer to use the existing GUID map in 56, or wait until 57 to land this?

Rolling this up into the two-phase work.