Closed Bug 1496878 Opened 2 years ago Closed 2 years ago

Ensure the bookmark merger flags newer locally moved children for reupload

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 65
Tracking Status
firefox65 --- fixed

People

(Reporter: lina, Assigned: lina)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Depends on: 1505191
Whew, this turned into a yak shave! 🐮 Hopefully the patch clarifies the behavior.
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
Blocks: 1506287
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
https://hg.mozilla.org/mozilla-central/rev/be95dcc90365
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Depends on: 1512867
You need to log in before you can comment on or make changes to this bug.