Ensure the bookmark merger flags newer locally moved children for reupload

RESOLVED FIXED in Firefox 65

Status

()

P2
normal
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: lina, Assigned: lina)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 months ago
Leif's fantastic analysis of the validation data from the new bookmarks engine (https://gist.github.com/irrationalagent/2dd2d50eb8d738b353fb1fb95144d0f5) shows that `parentid` differences are much more frequent for the new engine.

I looked into this in Dogear, and have a theory. :-) When we prefer a local move over a remote move, we give the merged parent a new structure state, but not the merged child. (We only do that for orphans). However, older Desktops and Android use the child's `parentid` to determine the parent, not the parent's `children`. Syncing with another Desktop using the new engine will still work, since it ignores `parentid`. However, syncing with an old Desktop, or with Android, will cause bad things to happen.

We still accidentally did the right thing for items where we took the local value state, since we didn't reset `syncChangeCounter` in that case. However, if we had a newer remote value with a new structure, we'd reset the local change flags and not upload the child...even if it had the right flag before merging.
Priority: -- → P2
(Assignee)

Updated

5 months ago
Depends on: 1505191
(Assignee)

Comment 1

5 months ago
Whew, this turned into a yak shave! 🐮 Hopefully the patch clarifies the behavior.
(Assignee)

Comment 2

5 months ago
This commit:

* Changes the merger to flag locally moved children for reupload, along
  with their parents, if the local move is newer.
* Replaces `valueState` and `structureState` in the merge states table
  with simpler flags. Rows with `useRemote` need to be updated in
  Places; rows with `shouldUpload` need change counter bumps.
* `itemsToMerge` now accounts for local-only items with a `LEFT JOIN`.
* Uses `shouldUpload` from the merged tree to set the change counters
  for all items, instead of relying on existing values and the
  `flagNewStructure` trigger to handle items with new structure.
* Fixes keyword reuploading to handle the above, and match the
  non-mirror behavior.
* Removes `structureCounts.new` from telemetry. Any merge with items
  added to the same folder on both sides will count as new structure,
  so it's not especially interesting.

MozReview-Commit-ID: EktIBQN9FSY

Depends on D11119
(Assignee)

Updated

5 months ago
Blocks: 1506287

Comment 3

4 months ago
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be95dcc90365
Ensure the bookmark merger flags newer locally moved children for reupload. r=tcsc

Comment 4

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/be95dcc90365
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
(Assignee)

Updated

4 months ago
Depends on: 1512867
You need to log in before you can comment on or make changes to this bug.