Closed Bug 1359200 Opened 2 years ago Closed 2 years ago

Sync should not upload items that have changed between when upload begins and completes

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED WONTFIX

People

(Reporter: tcsc, Unassigned)

References

Details

There's a chance for corruption to occur if we do this. One option: 

- Fetch all records at upload start.
- When uploading, if the change counter is no longer the same, abort the batch.

If this lands before most servers have support for aborting batches, then we should only abort the batch if either we haven't sent the first POST, or 1. we have sent the first POST, 2. the server supports batching, and 3. it's the first batch in this upload.
Duplicate of this bug: 1313967
Priority: -- → P3
(In reply to Thom Chiovoloni [:tcsc] from comment #0)
> - Fetch all records at upload start.

Bug 1305563 takes this approach, which we should profile for large trees to make sure our memory usage is under control. Another idea is to have a (flat) outgoing buffer where we can stage all outgoing records, not just bookmarks.

However, we already buffer all incoming records in memory before we call `recordHandler`, and we haven't seen reports of Firefox janking or running out of memory doing this. I'm inclined to try fetching all records at upload start, for all engines, and avoiding the indirection of a separate buffer if we can.

Another option is a hybrid: fetch full records in batches, up to the max total records. (For bonus points, figure out the best way to group bookmark records: folders first, parents before children? Or maybe complete subtrees instead? For history, newest visits first, weighted by frecency?)
Priority: P3 → P2
For bookmarks, bug 1305563 now stages outgoing bookmarks in a temp table, so this shouldn't be as expensive as fetching and iterating over the entire tree. We also know the level, type, and position; maybe we can use those to set the `sortindex` and group into subtrees, so that older clients receive and apply records without reparenting.
Flags: needinfo?(tchiovoloni)
This is fixed in the buffered bookmarks engine, and isn't worth doing in the old bookmarks engine since it's not trivial and that engine shouldn't be long for this world anyway.
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(tchiovoloni)
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.