1343985 - Investigate bookmark sync improvements necessary to support full-blown bookmarks management

Status: RESOLVED DUPLICATE of bug 1364644

(Firefox for Android Graveyard :: Android Sync, enhancement, P1)

Description

[:Grisha Kruglov]

Bug 1329127 tracks work around bookmarks management. We need to understand and plan for impact of that work on bookmark sync.

At this point, we shouldn't be corrupting data in transit \o/. However, the way we currently reconcile bookmark records is probably not good enough if both local and remote clients are making a lot of structural changes to the tree.

Bug 814801 is slowly paving the way towards a better future. Which aspects of it do we need to expedite?
Bug 1308337 is important for tracking impact out in the wild.

Comment 1

[:Grisha Kruglov]

It seems to me that it's possible we will never upload local modifications to records (or even new records) under a somewhat narrow set of conditions.

Generally, a change can happen before a sync, after a sync, or during a sync. The first two are trivial cases for which our timestamp-based "what changes since we last synced" decision making should be fine. Changes made during a sync are potentially problematic.

When we sync, we upload records which have changed since the lastSyncedTimestamp for a collection (sans those which we just downloaded and didn't need to merge with local changes). That timestamp is the X-Last-Modified response header value returned with the last POST during a previous upload.

As an aside, drifting local clock is enough to introduce erroneous behaviour. That is, if we sync at time X, then our wall clock is somehow changed to X-10, and we bookmark a record at local time X-5 (or make modifications), that bookmark or those changes will not be uploaded until its last-modified timestamp is bumped again at time >X (e.g. by a user edit in the future).

To narrow down the "changes during a sync" further, consider stages of a collection sync:
1) download "changed records since X"
2) merge those records with our local data
3) upload locally modified/added records since X and results of the merge from (2)

If modification happens during (3), after we fetched records from the local database and into memory but before the last POST request finished (in case of batching), that modification will not be uploaded during the current sync, and will not be uploaded during the next sync either. Consider that we fetched records for upload from the local database at time X, modification happens at time X+5, and on the last POST we get a header saying that Last-Modified time for the collection is X+10. Our "last-synced" timestamp will become X+10, and thus we'll miss changes that happened at X+5.

Comment 2

[Richard Newman [:rnewman]]

(In reply to :Grisha Kruglov from comment #1)
> It seems to me that it's possible we will never upload local modifications
> to records…

Yes. That's one of the reasons why it's bad to use timestamps for change tracking[1].


> Generally, a change can happen before a sync, after a sync, or during a
> sync. The first two are trivial cases for which our timestamp-based "what
> changes since we last synced" decision making should be fine.

Unless the client clock is wrong, in which case both "after" and "before" become meaningless concepts! We might upload an old record, or it might win a conflict, or we might not upload a change because it was transiently recorded as happening in 1989.


> Our "last-synced" timestamp
> will become X+10, and thus we'll miss changes that happened at X+5.

This also used to happen with racing clients: we'd miss changes written to the server between our last GET and our POST. We use XIUS to detect that. There isn't really such a remedy for local writes. Android doesn't have the sensible "Sync owns storage" architecture that iOS does.


[1] <https://160.twinql.com/syncing-and-storage-on-three-platforms/>

Comment 3

[:Grisha Kruglov]

The sync change tracker approach which desktop recently adopted (as doc'd in https://docs.google.com/document/d/1PnbPg4KX3oqrK31NB0-ht121viYgbTk696jKdv4KIAQ/edit#) should be reasonably straightforward to fit to Android's repository-based flow, and should solve the "modified during sync" problem and likely a good number of drifting local clock problems if we also switch to the change counter for decisions around _what_ needs to be uploaded. I say "if" here to account for the fact that it's possible to maintain a change counter while still relying on timestamps for uploads (counter>0 || timestamp>last-modified). This seems like an optimization around getting this out quickly, and might be too hacky once I sit down to actually think it through. At a glance it seems like a reasonable approach for the first stab at this.

As far as what's necessary to land alongside full bookmark management, some variation of change tracking seems appropriate. The screw ups here will be magnified by the fact that user is now modifying bookmark trees.

Comment 4

[Richard Newman [:rnewman]]

A switch to a change counter wouldn't be too difficult, and I'd love to see it happen. We already distinguish throughout BrowserProvider whether an operation is due to Sync or not. Just a lot of bookkeeping to check on, and of course a schema migration…

Comment 5

[:Grisha Kruglov]

Let's do this work in Bug 1364644.