See discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1343985#c3. Implementation details to come.

Comment on attachment 8886750 [details] Bug 1364644 - Pre: Remove guidsSince RepositorySession interface Hi Richard, Attached patches are an early WIP of sync tracker for bookmarks. Would you mind giving this a brief look? Code obviously needs to be firmed up, tests written, etc., but it does generally seem to work. Database migration needs to be firmed up and clarified in comments/code. Current approach is to mark everything as "new", with change counters set to 0. It's essentially a "no-op" migration, in a sense that it won't trigger an upload of anything by itself. I think that's the best we can do, since we really don't know any better on Android. BrowserProvider is mostly bookkeeping around incrementing counters, with a functional change around record clean up. I've opted for deleting all non-synced records straight up. I've added a "tracker middleware" which wraps a local repository, keeping track of records flowing out and building a guid->counter map. Once everything is successfully uploaded, counters are decremented in a single transaction. It's a "all or nothing" approach, so nothing is decremented on any sync failures, which matches our batch-upload semantics. At worst, this will cause us to re-upload some records. Looking a step ahead to when batching uploads are enabled with high enough per-batch limits so that they're truly atomic, I think this is sufficient. Thanks!

Comment on attachment 8886749 [details] Bug 1364644 - Bookmark version tracking https://reviewboard.mozilla.org/r/157556/#review162682 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2258 (Diff revision 2) > - * Base on {@link #bulkDeleteByHistoryGUID}, we chunk the list to prevent 'too many SQL variables' in one statement. > + * into 'too many SQL variables' error. > */ > - private int bulkDeleteByBookmarkGUIDs(SQLiteDatabase db, List<String> bookmarkGUIDs, String table, String bookmarkGUIDColumn) { > - // Due to SQLite's maximum variable limitation, we need to chunk our delete statements. > - // For example, if there were 1200 GUIDs, this will perform 2 delete statements. > - int deleted = 0; > + private int bulkDeleteByBookmarkGUIDs(SQLiteDatabase db, Uri uri, List<String> bookmarkGUIDs) { > + // Due to SQLite's maximum variable limitation, we need to chunk our update statements. > + // For example, if there were 1200 GUIDs, this will perform 2 update statements. > + int updated = 0; "changed"

Comment on attachment 8886750 [details] Bug 1364644 - Pre: Remove guidsSince RepositorySession interface https://reviewboard.mozilla.org/r/157558/#review162684 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksDataAccessor.java:345 (Diff revision 2) > protected String[] getAllColumns() { > return BrowserContractHelpers.BookmarkColumns; > } > + > + private String dateModifiedOrSyncChangeCounterWhere(long timestamp) { > + return BrowserContract.TrackerSyncColumns.SYNC_CHANGE_COUNTER + " > 0 " + It might be feasible to switch over to just change counter checks, but the whole flow is built around "since a timestamp", including our RepositoryBundles, etc. Given that we'll have a mixed situation (timestamp-based vs counter-based) tracking for a while across repositories, it's probably not worth the churn now. We can re-asses this once (and if) all repositories support change counters.

Comment on attachment 8886748 [details] Bug 1364644 - Migrate bookmarks schema and records to add version columns https://reviewboard.mozilla.org/r/157554/#review164748 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:2015 (Diff revision 2) > + updateBookmarksTableAddSyncTrackerFields(db); > + } > + > + private void updateBookmarksTableAddSyncTrackerFields(final SQLiteDatabase db) { > + db.execSQL("ALTER TABLE " + TABLE_BOOKMARKS + > + " ADD COLUMN " + Bookmarks.SYNC_STATUS + " INTEGER NOT NULL DEFAULT 0"); Might as well make this `TINYINT` for clarity (here and above).

Comment on attachment 8886749 [details] Bug 1364644 - Bookmark version tracking https://reviewboard.mozilla.org/r/157556/#review164750 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:158 (Diff revision 2) > @RobocopTarget > public interface TrackerSyncColumns { > - String SYNC_STATUS = "syncStatus"; > + String SYNC_STATUS = "syncStatus"; // new: 0, normal: 1, unknown: 2 > String SYNC_CHANGE_COUNTER = "syncChangeCounter"; > + > + int SYNC_STATUS_NEW = 0; Document what these mean. In particular, what does `UNKNOWN` represent? ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1572 (Diff revision 2) > > String where = Bookmarks._ID + " IN (" + > " SELECT DISTINCT " + Bookmarks.PARENT + > " FROM " + TABLE_BOOKMARKS + > " WHERE " + selection + " )"; > - return db.update(TABLE_BOOKMARKS, values, where, selectionArgs); > + final int changed = db.update(TABLE_BOOKMARKS, values, where, selectionArgs); If `changed` is 0, why are you bumping the change counter? ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1613 (Diff revision 2) > > + // Set initial sync state and change counter based on origin of insertion. > + // It's important that insertions from Sync do not trigger another Sync to start. > + if (isCallerSync(uri)) { > + values.put(Bookmarks.SYNC_CHANGE_COUNTER, 0); > + values.put(Bookmarks.SYNC_STATUS, Bookmarks.SYNC_STATUS_NORMAL); Why `SYNC_STATUS_NORMAL` and not `SYNC_STATUS_SYNCED`? ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1645 (Diff revision 2) > // database schema foreign key constraint. > final long parentId = values.getAsLong(Bookmarks.PARENT); > - db.update(TABLE_BOOKMARKS, > - parentValues, > - Bookmarks._ID + " = ?", > - new String[] { String.valueOf(parentId) }); > + final String parentSelection = Bookmarks._ID + " = ?"; > + final String[] parentSelectionArgs = new String[] { String.valueOf(parentId) }; > + db.update(TABLE_BOOKMARKS, parentValues, parentSelection, parentSelectionArgs); > + incrementChangeCounter(db, uri, TABLE_BOOKMARKS, parentSelection, parentSelectionArgs); Why are you not incrementing the change counter inside the `update` call by amending `parentValues`? Is it just because `ContentValues` isn't able to express the increment? If so, take a look at DBUtils, specifically `UpdateOperation`, which allows you to avoid this restriction. You're doing two SQL calls when one might do… and if this is not inside an explicit transaction, you've lost atomicity. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1698 (Diff revision 2) > } > > beginWrite(db); > > int updated = db.update(TABLE_BOOKMARKS, values, inClause, null); > + if (!isCallerSync(uri)) { Again, check for 0 -- move this down below the next block. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1708 (Diff revision 2) > trace("No update on URI: " + uri); > return updated; > } > > if (isCallerSync(uri)) { > // Sync will handle timestamps on its own, so we don't perform the update here. Shouldn't you be doing some work here? ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2287 (Diff revision 2) > final List<String> chunkGUIDs = bookmarkGUIDs.subList(chunkStart, chunkEnd); > - deleted += db.update(table, > - values, > - DBUtils.computeSQLInClause(chunkGUIDs.size(), bookmarkGUIDColumn), > - chunkGUIDs.toArray(new String[chunkGUIDs.size()]) > - ); > + final String selection = DBUtils.computeSQLInClause(chunkGUIDs.size(), Bookmarks.GUID); > + final String[] selectionArgs = chunkGUIDs.toArray(new String[chunkGUIDs.size()]); > + updated += db.update(TABLE_BOOKMARKS, values, selection, selectionArgs); > + > + // TODO do we even need to bump a change counter here? Yes; you need to upload deletions. ::: mobile/android/base/java/org/mozilla/gecko/db/SharedBrowserDatabaseProvider.java:125 (Diff revision 2) > + return; > + } > + > + // For data types which support sync change tracking, we can safely drop all deleted records > + // that were never uploaded - that is, have a sync status 'new'. Other clients don't know > + // about them, and don't need to know that we deleted them locally. Oh boy. You should talk to Mark about tombstones needing to live longer to avoid resurrection…

Comment on attachment 8886750 [details] Bug 1364644 - Pre: Remove guidsSince RepositorySession interface https://reviewboard.mozilla.org/r/157558/#review164756 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/TrackerMiddlewareRepository.java:27 (Diff revision 2) > + > +/** > + * Tracker-aware middleware which is intended to wrap local repositories. > + * This middleware owns syncChangeCounter portion of Sync Tracker from the Sync side. > + * > + * Problem overview: Timestamp-oriented problem ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/TrackerMiddlewareRepository.java:44 (Diff revision 2) > + * whenever any modifications are made to the record from outside of Sync > + * - before changed/merged records are uploaded, and right after they're read from the database, we > + * make a note of change counters for each retrieved GUID > + * - once upload is finished successfully - that is, once this repository is fully synchronized - we > + * decrement syncChangeCounter for each processed GUID by the value we noted before the upload > + * - this is a "all or nothing" operation - either all processed records incomplete sentence ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/TrackerMiddlewareRepository.java:51 (Diff revision 2) > + * Before a sync, its change counter is at 7, indicating that 7 various modification were made since > + * last sync (or since its creation, if it wasn't synced yet). > + * Along with other records, X is uploaded to the server, and we make note of counter=7 here. > + * During the upload, a local modification is performed, bumping X's change counter to 8. > + * This modification is not uploaded, since the record at version 7 is already in memory. > + * After an upload, we decrement change counter by 7. What happens if the UI transaction spans the post-upload transaction? Options: UI: [ increment ] Sync: [ read ] [decrement ] UI: [ increment ] Sync: [ read ] [decrement ] UI: [ increment ] Sync: [ read ] [decrement ] Are you sure that all of these will result in no missing changes?

My main concern is about migration. We are moving from a situation in which a timestamp is the indicator for changes to one in which the database is. AFAICS you always migrate to NEW, which will result in every record being reuploaded, or something else horrific happening. I would expect the following: - The DB migration moves all existing records to UNKNOWN, if Sync is set up, and NORMAL if it is not. - At the start of a sync, the tracker looks at the last sync timestamp, and runs a query like this (not real SQL): UPDATE bookmarks SET syncStatus = dateCreated < $lastSync ? NORMAL : NEW, syncChanges = dateModified < $lastSync ? 0 : 1, WHERE syncStatus = UNKNOWN That is, for all migrated bookmarks, mark them as NEW if they were created since the last sync, and NORMAL if not, and give them a change counter of 1 if they changed since the last sync. That's a straightforward mapping of our timestamp-based approach to flags. Normally there will be no UNKNOWN bookmarks, because we'll never create more, so you can choose to drop that logic later.

(In reply to Richard Newman [:rnewman] from comment #14) > AFAICS you always migrate to NEW, which will result in every record being > reuploaded, or something else horrific happening. As written, syncing of records is currently driven by a combination timestamp and the change counter, and since records are migrated with counter=0 and timestamp isn't modified, nothing should be re-uploaded simply as a result of the migration itself. However, that leaves the status flags rather meaningless, since it'll be impossible to determine with accuracy if a record has been synced at some point prior. Your proposed migration path is probably the way to go about this. I'm also fairly sure we'll get states of some of the records wrong due to floating timestamps, bookmark creation date syncing being a very recent addition (Bug 1335198 landed in 55), etc., and so we should be cautions here and assume that a record has been synced if we're not entirely certain. To clarify my assumptions and understanding, I'll jot down my thoughts on this work: On Sync Status: - TLDR: what about simply maintaining sync status in this bug, but not acting on it? That is, address record tombstones separately from this work. - Sync Status is our indicator for whether a record has been uploaded to Sync, or if it only exists locally. Naturally, there are three states: "synced", "not synced", "unknown". Current names - new, normal - aren't as clear, so we might as well change them, leaving a comment describing what the nomenclature is in Desktop's implementation. - Sync Status is inherently an optimization around storing of record deletions. When we delete a record, we want to ensure that deletion is propagated to all devices that have a copy of this record, and so we keep around a "tombstone". We don't want to do this unnecessarily - the assumption here is that if no other client is aware of a given record, we don't need a tombstone for it when we delete it locally. - An alternative approach would be to just store tombstones forever, ensuring we can always accurately propagate deletions of records. This will have a negative impact on storage, with a theoretically unbounded , but it will ensure that we'll always be able to propagate our deletions to other clients. - Current Fennec approach is to store tombstones for a "long enough period of time", which is set to be 20 days or a bit longer, depending on how many bookmarks user deletes, and how frequently deletions happen (tombstones are deleted in small chunks, and as a side-effect of deletions). - This is inherently lossy, and it's unclear to me that assumptions that went into this decision still hold true. - IIRC, desktop's age threshold for tombstones is counted in years. - On mobile, due to device upgrade cycles, "store for a couple of years" == "store forever". - Decisions around storing of tombstones should be weighed against device impact (minimal, by today's standards) and likelihood of bad things happening if we drop tombstones - deleted records re-appearing on this or other devices - IIUC, due to the very nature of Sync in combination with our privacy policies, we're always bound to get this wrong, under a particular set of circumstances. And so the name of the game is "reduce likelihood", not "eliminate". - It might make sense to address tombstone deletions entirely separately from the rest of this work. Required changes spill beyond concerns of the tracker itself, and so it might make sense to take a holistic look at the problem, rather than make changes piecemeal. - Circling back to Sync Status flag, we should be very conservative with its use. That is, if unsure, prefer to re-upload records rather than increase chance of data inconsistencies. On Sync Change Counter: - TLDR: what about dropping syncStatus entirely, in lieu of two per-record counters that always go up: "version" and "lastSyncedVersion"? - Sync Change Counter is our indicator for how many times a record has been modified locally. - However, we coerce it into a "how many times a record has been modified since it was read from the database for the purposes of being uploaded during a sync" - We do so by decrementing it at the end of a successful sync by the value it was set to at the time of reading the record from a database during a sync. - This is also partially an optimization: our alternative is to store two counters for each record, total number of modifications ("version") and "last synced version". - We only care about counting versions of records so that we don't miss uploading them during a sync. - These two versions considered together could give us an equivalent of "sync status" - lastSyncedVersion>0 is equivalent to syncStatus==synced, etc - version>0 && lastSyncedVersion=0 is equivalent to syncStatus==new - version=0 && lastSyncedVersion=0 is equivalent to syncStatus==unknown - This poses a requirement that the "unknown" case never actually exists for records which may be modified by the user, since that will transition them into a "new" state. - As long as we have access to the "last synced" timestamp during a database migration which introduces these fields, we should be able to avoid the "unknown" state entirely. - It becomes possible then to drop the syncState field entirely in lieu of the above. - Overall I like the two version solution a bit more - it is nicely symmetric (browser modifies one version, sync another), and very unambiguous in its intent. Sync status and logic behind it becomes very explicit, instead of being abstracted away by a flag that's controlled from multiple places. - However, unless I'm missing something, the two approaches are logically (or could be made to be) equivalent. We're simply renaming our states and transitions between them. - Implementations are going to be quite similar as well. The main difference becomes that we'll always need to look at two columns during sync or deletion in order to derive "sync state", and our per-record local storage impact will be a little bit higher. Richard, you've probably considered this approach before. Neither Desktop's new tracker, nor iOS's sync implementation follow it. Did I hit on the reasons "why not" above, or is there something else I'm missing?

(In reply to :Grisha Kruglov from comment #15) > I'm also fairly sure we'll get states of some of the records wrong due > to floating timestamps, bookmark creation date syncing being a very recent > addition (Bug 1335198 landed in 55), etc., and so we should be cautions here > and assume that a record has been synced if we're not entirely certain. If at the point of migration we use the timestamp to set flags, the result should be exactly equivalent to if we'd synced with the old logic at that moment in time, no? "No more wrong than the old way" is the Sync motto :D > - Decisions around storing of tombstones should be weighed against device > impact (minimal, by today's standards) and likelihood of bad things > happening if we drop tombstones - deleted records re-appearing on this or > other devices I'm OK with keeping tombstones forever. I mean, we don't even clean up history on Android, so bookmark tombstones are a drop in the bucket. > - Circling back to Sync Status flag, we should be very conservative with its > use. That is, if unsure, prefer to re-upload records rather than increase > chance of data inconsistencies. Preferring to upload is quite likely to cause some inconsistencies… > Richard, you've probably considered this approach before. Neither Desktop's > new tracker, nor iOS's sync implementation follow it. Did I hit on the > reasons "why not" above, or is there something else I'm missing? In short: subtly different requirements/constraints, and solutions evolving over time. On iOS we have serial database access that passes through a Sync-aware storage interface. We started out with change tracking, not with timestamps. History simply flips `should_upload` when something is changed; it's always safe to download then upload, and worst-case we miss a visit that occurred during an upload (rare). Bookmarks tracks `sync_status` -- new, synced, changed. Missing changes (not additions!) that occur during an upload is the main known theoretical limitation of the approach. However, on iOS incoming records don't collide with the local tree -- local writes go to local, remote writes stop in the buffer -- so the window for error is very small. We came up with the idea of a change counter during subsequent desktop work. On desktop we have only a single bookmark store, so it's necessary to accommodate local writes that arrive during a sync. Two advancing versions is not necessarily exactly equivalent to a change counter. Depending on the database implementation and the possibility of races, a single change counter can suffer from competing writes, either rejecting a legitimate change, locking the row, or losing data, while two version counters might not. If the database locks at the row level, and there are no read-then-store accesses, then the two are probably equivalent. The two version approach is conceptually clearer if you think like me, but I don't know if that is true for everyone. A single change counter should always converge to zero, whereas the versioning approach will not (without periodic decrementing cleanup), and a single change counter is smaller (by one or two bytes per record per index). I'm not sure what the correct approach would be when a local record conflicts with an incoming record, or indeed when a remote record is applied in any case -- do we reset both versions to zero? I encourage you to continue with this line of thinking; it might be productive.

Pushed a first (largely untested) pass of the version tracking sync. Includes schema and data migration, version tracking (addressing review comments above), versioned middleware and sync plumbing changes to make things work. Still to do: tests, actually run through a bunch of scenarios and make sure this all works properly.

While testing the "sync, disconnect, sync" scenario, I notice that we seem to be excluding all folders from upload during "first" sync. It takes two syncs to actually upload all of the records. AFAICT this is also the current behavior (versioned syncing shouldn't have changed this), it's just very notice with the two versions.

(In reply to :Grisha Kruglov from comment #32) > And so (according to comments, anyway) none of our "account > is deleted" logic > will actually run if account is stored on an sd card and its pulled. Is this > a problem? Probably not. We have a fair amount of code to try to *recover from* the Android account deletion when the SD card is removed and reinserted!

Comment on attachment 8886748 [details] Bug 1364644 - Migrate bookmarks schema and records to add version columns https://reviewboard.mozilla.org/r/157554/#review166588 ::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserDatabaseHelperTest.java:128 (Diff revision 7) > + > + testBookmarkVersionMigrationChunkingForCount(db, DBUtils.SQLITE_MAX_VARIABLE_NUMBER - DEFAULT_BOOKMARKS_COUNT - 1); > + } > + > + @Test > + public void testBookmarkTimestampToVersionMigrationChunkingEqual() throws Exception { Currently the *Equal and *More chunking tests are failing, throwing a runtime exception - "too many variables". The strange thing is that they throw when the update statement has 999 variables in it. The "less" test passes, and all tests pass if I change the SQLITE_MAX_VARIABLE_NUMBER to 998. We have another test which invokes this kind of "chunking" logic - history visit deletion, https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderHistoryVisitsTest.java#220 Stepping through _that_ test, everything seems to be just fine when we're trying to delete 999 items at once. This leads me to believe that I'm defining the word "variable" wrong, and that the SET qualifiers in the `update` statement (we set syncVersion in this case) are actually counted against the total..? That would explain what I'm seeing, and difference between the working `delete` tests. If that's true, that other users of this chunking logic and update statements are also broken. BrowserProvider.bulkDeleteByBookmarkGUIDs comes to mind.

(In reply to :Grisha Kruglov from comment #49) > This leads me to believe that I'm defining the word "variable" wrong, and > that the SET qualifiers in the `update` statement (we set syncVersion in > this case) are actually counted against the total..? That would explain what > I'm seeing, and difference between the working `delete` tests. Indeed, that is the case, and should have been pretty obvious. For future references, statement such as UPDATE table SET field1 = 1, field2 = 1 WHERE id IN (?, ?, ?) has 5 variables for the purposes of SQLITE_MAX_VARIABLE_NUMBER. Our current logic in this patch series and elsewhere assumes that this would be counted as 3 variables. This might be implementation/version dependent, but erring on the side of possibly over-counting how many variables we're using should be safe.

(In reply to :Grisha Kruglov from comment #50) > has 5 variables for the purposes of SQLITE_MAX_VARIABLE_NUMBER. This disagrees very much with the docs: "Parameters" are things like '?': https://sqlite.org/lang_expr.html#varparam and > SQLITE_LIMIT_VARIABLE_NUMBER > The maximum index number of any parameter in an SQL statement. (Note that SQLITE_MAX_VARIABLE_NUMBER is merely the default value of sqlite3_limit(SQLITE_LIMIT_VARIABLE_NUMBER).) I'm going to read about this a little more…

That surprised me, too; the docs seemed clear that "variables" and "parameters" are bound. I filed bug 1384644 for Desktop just in case.

Correction to Comment 50: here's the statement that was failing to be prepared: UPDATE bookmarks SET syncVersion=? WHERE guid IN (?, ?, ?, ?, ... 999 of those, ... ?, ?, ?); It obviously has 1000 variables in it, and thus fails. I was thinking of it as "... SET syncVersion=1 ...", which isn't correct. And our similar logic elsewhere already seems to do the right thing, e.g.: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java#2249

Comment on attachment 8886748 [details] Bug 1364644 - Migrate bookmarks schema and records to add version columns https://reviewboard.mozilla.org/r/157554/#review167434 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:147 (Diff revision 8) > Bookmarks.IS_DELETED + " INTEGER NOT NULL DEFAULT 0, " + > + > + // Mark every new record as "needs to be synced" by default. > + Bookmarks.LOCAL_VERSION + " INTEGER NOT NULL DEFAULT 1, " + > + Bookmarks.SYNC_VERSION + " INTEGER NOT NULL DEFAULT 0, " + > + This currently doesn't have an index. We do query for "modified records" via localVersion>syncVersion during a sync, and I think an index on (localVersion,syncVersion) should help when there are a lot of bookmarks. Otherwise this query will probably perform two full traversals of the data... But, I haven't done perf testing to see if it'll worth the overhead. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:2039 (Diff revision 8) > + // As a result of this migration, each bookmark will be marked as either "synced", or > + // "needs to be synced". > + // "Synced" means that according to the timestamps present, we consider a record to be > + // up-to-date, not requiring an upload. > + // "Needs to be synced" means that the record was either never synced for the current account, > + // or was modified since it was last synced, requiring an upload. or it's in a weird state with strange or missing timestamps ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:2125 (Diff revision 8) > + return; > + } > + > + // Bookmarks were synced at some point. Iterate through all of them, find the ones that we > + // can mark as "synced". > + final Cursor allBookmarksCursor = db.query(TABLE_BOOKMARKS, This should just explicitely query for the records that we want to modify, and do all of the work below in SQL. Or this could even be an update statement which modifies the records according to a WHERE statement. The current approach _might_ be more performant for larger data sets. We only have an index on 'modified' to help us with the in-sql work.

Comment on attachment 8886749 [details] Bug 1364644 - Bookmark version tracking https://reviewboard.mozilla.org/r/157556/#review167438 ::: commit-message-2b67b:2 (Diff revision 9) > +Bug 1364644 - Bookmark version tracking r=rnewman > + Add a commit description of what this does and why. Describe separation between "from fennec" and "from sync", and how it relates to version tracking. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:801 (Diff revision 9) > public int updateInTransaction(Uri uri, ContentValues values, String selection, > String[] selectionArgs) { > trace("Calling update in transaction on URI: " + uri); > > + if (values.containsKey(BrowserContract.VersionColumns.LOCAL_VERSION) || values.containsKey(BrowserContract.VersionColumns.SYNC_VERSION)) { > + throw new IllegalArgumentException("Can not manually set record versions."); /s/set/update ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1615 (Diff revision 9) > values.put(Bookmarks.TITLE, ""); > } > > String url = values.getAsString(Bookmarks.URL); > > + values.put(Bookmarks.LOCAL_VERSION, 1); We don't _have_ to do this, since the default value for this is already set on the schema level, and we check that values won't have either of local or sync versions in it... ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1652 (Diff revision 9) > final long parentId = values.getAsLong(Bookmarks.PARENT); > - db.update(TABLE_BOOKMARKS, > - parentValues, > - Bookmarks._ID + " = ?", > - new String[] { String.valueOf(parentId) }); > + final String parentSelection = Bookmarks._ID + " = ?"; > + final String[] parentSelectionArgs = new String[] { String.valueOf(parentId) }; > + db.update(TABLE_BOOKMARKS, parentValues, parentSelection, parentSelectionArgs); > + // TODO DBUtils.updateArrays > + incrementLocalVersion(db, uri, TABLE_BOOKMARKS, parentSelection, parentSelectionArgs); (here and elsewhere) As per the TODO, these could be done in one statement instead of two. Given that we're in a transaction here, I imagine that this isn't strictly necessary, but desirable. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1712 (Diff revision 9) > trace("No update on URI: " + uri); > return updated; > } > > + // If the update is coming from Sync, we're done. Sync updates do not affect localVersion, > + // and Sync will handle timestamps of parent records on its own. "When versions are allowed to be updated" should be described somewhere in this file very explicitely. That is, callers are _never_ allowed to manually modify versions themselves. With an exception of version resets: - localVersion is only ever modified (incremented by 1) as a response to any changes to a record that occur from Fennec-proper. - syncVersion is only ever modified in bulk for a collection of records, never on individual basis, and always from a sync caller. For version resets: - localVersion and syncVersions are allowed to be reset to 1,0 in bulk for a collection of records, never on individual basis, and only from a sync caller. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1717 (Diff revision 9) > if (isCallerSync(uri)) { > - // Sync will handle timestamps on its own, so we don't perform the update here. > return updated; > } > > + incrementLocalVersion(db, uri, TABLE_BOOKMARKS, selection, selectionArgs); Combining the various updates that happen here might be beneficial, but will come at a cost of brevety/readability and possible subtle bugs around this logic. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2281 (Diff revision 9) > values.putNull(Bookmarks.DESCRIPTION); > values.putNull(Bookmarks.KEYWORD); > values.putNull(Bookmarks.TAGS); > values.putNull(Bookmarks.FAVICON_ID); > > // Bump the lastModified timestamp for sync to know to update bookmark records. change this comment, timestamps don't drive bookmark sync anymore. ::: mobile/android/base/java/org/mozilla/gecko/db/SharedBrowserDatabaseProvider.java:126 (Diff revision 9) > + } > + > + // For data types which support sync versioning, we can safely drop all deleted records > + // that were never uploaded. Other clients don't know about them, and don't need to know > + // that we deleted them locally. > + int deletedNew = db.delete(tableName, File and log here a follow-up to properly deal with tombstones (store them forever for bookmarks?)

Comment on attachment 8889707 [details] Bug 1364644 - Pre: Refactor bookmark and history sessions to allow for different superclasses https://reviewboard.mozilla.org/r/160768/#review167446 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2542 (Diff revision 5) > + // re-wrap data in, say, a list of ContentValues. > + final ConcurrentHashMap<String, Integer> syncVersionsForGuids = uncheckedCastSerializableToHashMap( > + data.getSerializable(BrowserContract.METHOD_PARAM_DATA) > + ); > + > + String table; See next patch in the series. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/VersionedMiddlewareRepository.java:26 (Diff revision 5) > +import java.util.concurrent.ConcurrentHashMap; > +import java.util.concurrent.ExecutorService; > + > +/** > + * Versioning-aware middleware which is intended to wrap local repositories. > + * This middleware owns syncVersion, counter-part of the localVersion owned by Fennec. ... "owns" with an exception of resetting syncVersion. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/VersionedMiddlewareRepository.java:44 (Diff revision 5) > + * - localVersion is incremented whenever any modifications are made to the record from Fennec proper; > + * that is, outside of Sync > + * - syncVersion is moved forward once we're sure we have uploaded a certain localVersion of the record > + * - before new/changed/merged records are uploaded, and right after they're read from the database, > + * we make a note of local versions for each retrieved GUID > + * - once upload is finished successfully - that is, once this repository is fully synchronized - we "fully synchronized" is technically incorrect given how that concept is used in the codebase... This really means that "the new state of the world as this local client sees it has been persisted to the server". ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/VersionedMiddlewareRepository.java:46 (Diff revision 5) > + * - syncVersion is moved forward once we're sure we have uploaded a certain localVersion of the record > + * - before new/changed/merged records are uploaded, and right after they're read from the database, > + * we make a note of local versions for each retrieved GUID > + * - once upload is finished successfully - that is, once this repository is fully synchronized - we > + * update syncVersion values for each processed GUID to the value we noted before the upload > + * - this is a "all or nothing" operation - either we move forward sync versions for all processed "move sync versions forward" ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/VersionedMiddlewareRepository.java:50 (Diff revision 5) > + * update syncVersion values for each processed GUID to the value we noted before the upload > + * - this is a "all or nothing" operation - either we move forward sync versions for all processed > + * records, or we don't move them at all > + * - records are expected to be uploaded in atomic batches during normal operation > + * - if batching mode is disabled and we encounter a partial upload, syncVersion won't move forward > + * and records will be eventually re-uploaded On one hand, this will cause unnecessary uploads of data when batching mode is disabled on the servers, as it has been lately. On another hand, this should help decrease likelyhood of screwing up data in case of broken/partial uploads (at a cost of device/server resources). ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/VersionedMiddlewareRepository.java:58 (Diff revision 5) > + * Along with other records, X is queued for an upload to the server, and we make note of localVersion=7. > + * During the upload, a local modification is performed, bumping X's localVersion to 8. > + * This modification is not uploaded, since copy of the record at version 7 is already in memory. > + * After an upload, we move syncVersion to 7. > + * Sync is finished, and X is now at localVersion=8, and syncVersion=7, indicating that there are > + * still changes to be uploaded. We should consider firing up a follow-up sync to catch up with these unsynced changes. They happen when user actions and sync coincide in time, and so it _might_ be users' expectation that the changes they've just made that we noted but didn't upload yet will be present on other devices. User is always able to sync manually, but perhaps we should try harder to keep our states across clients "in-sync". ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/VersionedMiddlewareRepository.java:191 (Diff revision 5) > + throw new IllegalStateException("Expected to receive a result after decrementing change counters"); > + } > + > + int changed = (int) result.getSerializable(BrowserContract.METHOD_RESULT); > + if (changed != this.localVersionsOfGuids.size()) { > + // TODO perhaps not worth throwing, but this shouldn't happen either... I think this should be a log. Operational behaviour if we throw is that everything crashes. Behaviour if we log an error and move on is that we upload more than was strictly necessary next time around. Need to ensure we're not swallowing actual bugs by just logging here... ::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderBookmarksTest.java:457 (Diff revision 5) > > + @Test > + public void testBulkUpdatingSyncVersions() throws Exception { > + final long rootId = getIdFromGuid(BrowserContract.Bookmarks.MOBILE_FOLDER_GUID); > + > + ConcurrentHashMap<String, Integer> guidsToVersionMap = new ConcurrentHashMap<>(); Ugh, this is annoying.

Comment on attachment 8889638 [details] Bug 1364644 - Versioned syncing of bookmarks https://reviewboard.mozilla.org/r/160662/#review167454 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java:415 (Diff revision 7) > protected static boolean isDeleted(Cursor cur) { > return RepoUtils.getLongFromCursor(cur, BrowserContract.SyncColumns.IS_DELETED) != 0; > } > > @Override > + public void fetchModified(long timestamp, RepositorySessionFetchRecordsDelegate delegate) { This has been moved up in the class hierarchy since bookmark and history session objects now have different implementations of this. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java:442 (Diff revision 7) > + > + if (retrievedRecord != null) { > + // In addition to the regular bookmark fields, we will need to know the local version of this record. > + // It is used by the VersionedMiddlewareRepository alongside with syncVersion to track records' sync state. > + final int localVersionCol = cur.getColumnIndexOrThrow(BrowserContract.VersionColumns.LOCAL_VERSION); > + retrievedRecord.localVersion = cur.getInt(localVersionCol); This should ensure that fetched localVersion is not 0L. It should _never_ be 0L (we start off at 1, and only increment it, other than resetting it back to 1 if sync is disconnected). And so 0L will mean a null value in the database, which should probably throw, because it indicates we screwed up somewhere? ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserHistoryDataAccessor.java:64 (Diff revision 7) > > + /** > + * History repository does not support version tracking yet. > + */ > + @Override > + public Cursor fetchModified() throws NullCursorException { The naming here is unfortunate... One might think that fetchModified operation on the history session isn't supported, which isn't the case.

Wow, self-review makes my life so much easier! :D (In reply to :Grisha Kruglov from comment #66) > This currently doesn't have an index. We do query for "modified records" via > localVersion>syncVersion during a sync, and I think an index on > (localVersion,syncVersion) should help when there are a lot of bookmarks. > Otherwise this query will probably perform two full traversals of the data... > > But, I haven't done perf testing to see if it'll worth the overhead. Indices are expensive: not only do they make the database file larger, but they make writes slower. If you're already finding records by GUID or ID (which I think are both indexed), in most cases, then I think a single table walk to pick up changes will be OK.

I do think I missed one important thing. During bookmark reconciliation, "merged" records are written into the db, updating whatever we currently have. Prior logic relied on modified timestamp being bumped during the update by BrowserProvider (lastModified is omitted from the CVs passed into the update call), and consequently the merged record being picked up during the upload. The new logic doesn't increment localVersion in such case, and thus a merged record won't get uploaded. This should be a pretty straightforward fix..

Comment on attachment 8886750 [details] Bug 1364644 - Pre: Remove guidsSince RepositorySession interface https://reviewboard.mozilla.org/r/157558/#review167528

Comment on attachment 8889636 [details] Bug 1364644 - Pre: Move change tracking responsibilities into repositories https://reviewboard.mozilla.org/r/160658/#review167530 ::: commit-message-11fe9:7 (Diff revision 3) > + > +As part of moving toward versioned syncing, we need to start deprecating and removing > +timestamp-based concepts from the codebase. > + > +The idea here is that we're fetching modified records, letting repositories determine what > +they consider to be modified. Repositories which support versioning will provide records The tricky part about this, though: `Repository` is a stateless thing. `RepositorySession` represents a single pairing of that `Repository` with a synchronizer. The repository is intended to be stateful, but not sync-aware. The `Repository` design is shaped to support syncing between arbitrary sources and sinks of the same record type -- e.g., to back up to a local file _and_ sync to a server. Your change here fundamentally alters that model: you're tying the state of the repository to an extant Sync account, with a remote state, and the change tracking is happening inside the repository. I don't disagree with this simplification of the aims of the model, but it does leave a bit of a semantic and API design chasm. You could imagine a slightly different look at this: imagine if your server version was the server modified time of the record (which is a version number that looks very much like a millisecond timestamp, but actually isn't). That might have some interesting properties. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/RepositorySession.java:94 (Diff revision 3) > + * support versioning, this should be refactored. See Bug 1383894. > + * > + * @param timestamp Fallback for timestamp-based record tracking > + * @param delegate > + */ > + public abstract void fetchModified(long timestamp, RepositorySessionFetchRecordsDelegate delegate); I would rather not mix these two methods together. I mean, the server repository will always require a fetch by modified time, and it implements this interface!

Comment on attachment 8889637 [details] Bug 1364644 - Pre: don't swallow runtime exceptions in the BrowserProvider's call interface https://reviewboard.mozilla.org/r/160660/#review167532 I'm not sure what this is for, but it seems harmless.

Comment on attachment 8886749 [details] Bug 1364644 - Bookmark version tracking https://reviewboard.mozilla.org/r/157556/#review167534 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1711 (Diff revision 9) > if (updated == 0) { > trace("No update on URI: " + uri); > return updated; > } > > + // If the update is coming from Sync, we're done. Sync updates do not affect localVersion, Why don't Sync updates affect localVersion? Surely if we're writing into the DB from Sync, we should force `localVersion` to be the new server version, otherwise we won't detect local changes? ::: mobile/android/tests/background/junit4/src/org/mozilla/gecko/db/BrowserProviderBookmarksTest.java:356 (Diff revision 9) > + assertVersionsForGuid(guid1, 2, 0); > + > + assertVersionsForGuid(BrowserContract.Bookmarks.MOBILE_FOLDER_GUID, 2, 0); > + > + updateBookmarkTitleByGuid(bookmarksTestSyncUri, guid2, "just title"); > + assertVersionsForGuid(guid2, 2, 1); I don't think that this is right. You updated the bookmark, presumably after pulling up local changes. As soon as we're done applying changes from Sync, we're declaring that the local record and the remote record are the same, and have reflected remote changes -- same version. If we needed to reconcile conflicts, we would write the computed record into the DB, and we'd make sure that the local version was ahead of the sync version (modifying both in the course of the sync). Right? Or are you simply not at the point of doing this yet in this patch?

Comment on attachment 8889638 [details] Bug 1364644 - Versioned syncing of bookmarks https://reviewboard.mozilla.org/r/160662/#review167536 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserRepositorySession.java:481 (Diff revision 7) > // logging and side-effects to occur, and is no more expensive than simply > // bumping the modified time. > Logger.debug(LOG_TAG, "Replacing existing " + existingRecord.guid + > (toStore.deleted ? " with deleted record " : " with record ") + > toStore.guid); > Record replaced = replace(toStore, existingRecord); Somewhere here, I think we might need to indicate that localVersion should be bumped - or, allow BrowserProvider to bump it when updates are coming from sync, and rely on tracker to ensure it won't get re-uploaded erroneously (which should be covered already). Otherwise, an incoming record that was merged with a local one and then stored, in a way that a new record state is produced which needs to be synced up, will not be picked up by localVersion>syncVersion change tracking. IIUC prior/current behaviour relied on modified timestamp being bumped for the record on write.

Comment on attachment 8889707 [details] Bug 1364644 - Pre: Refactor bookmark and history sessions to allow for different superclasses https://reviewboard.mozilla.org/r/160768/#review167540 ::: commit-message-8d28c:7 (Diff revision 4) > +Once all records have been uploaded, middleware moves syncVersion for every > +record to its corresponding localVersion as it was read prior to the upload. This only makes sense if all incoming records have been applied and versions altered to match before changed records have been fetched. Otherwise you'll lose the fact of local changes before applying the incoming. That's an important guarantee for this API to rely on. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/VersionedMiddlewareRepository.java:47 (Diff revision 4) > + * - before new/changed/merged records are uploaded, and right after they're read from the database, > + * we make a note of local versions for each retrieved GUID > + * - once upload is finished successfully - that is, once this repository is fully synchronized - we > + * update syncVersion values for each processed GUID to the value we noted before the upload > + * - this is a "all or nothing" operation - either we move forward sync versions for all processed > + * records, or we don't move them at all … per atomic upload, surely? ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/VersionedMiddlewareRepository.java:50 (Diff revision 4) > + * update syncVersion values for each processed GUID to the value we noted before the upload > + * - this is a "all or nothing" operation - either we move forward sync versions for all processed > + * records, or we don't move them at all > + * - records are expected to be uploaded in atomic batches during normal operation > + * - if batching mode is disabled and we encounter a partial upload, syncVersion won't move forward > + * and records will be eventually re-uploaded I don't think that's a necessary restriction. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/VersionedMiddlewareRepository.java:58 (Diff revision 4) > + * Along with other records, X is queued for an upload to the server, and we make note of localVersion=7. > + * During the upload, a local modification is performed, bumping X's localVersion to 8. > + * This modification is not uploaded, since copy of the record at version 7 is already in memory. > + * After an upload, we move syncVersion to 7. > + * Sync is finished, and X is now at localVersion=8, and syncVersion=7, indicating that there are > + * still changes to be uploaded. In order for this to work, the local version has to be bumped to match the synced version on download. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/VersionedMiddlewareRepository.java:168 (Diff revision 4) > + this.inner.fetchAll(new VersionedRepositorySessionFetchRecordsDelegate(delegate)); > + } > + > + @Override > + public void store(Record record) throws NoStoreDelegateException { > + this.inner.store(record); So inner here has to be responsible for both reconciling conflicts and advancing versions.

Comment on attachment 8889638 [details] Bug 1364644 - Versioned syncing of bookmarks https://reviewboard.mozilla.org/r/160662/#review167546 Things I don't see so far: - Advancing the local version when we apply a new remote record. - Detecting a conflict during fetch (local version greater than previous sync version). - Detecting races during reconciling (local version changed since conflict detected). - Handling reconciled conflicts by bumping the local version to be higher than the sync version. Am I missing something?

Comment on attachment 8890679 [details] Bug 1364644 - Post: remove AndroidBrowser prefix from class names https://reviewboard.mozilla.org/r/161858/#review167550

(In reply to Richard Newman [:rnewman] from comment #78) > Things I don't see so far: > > - Advancing the local version when we apply a new remote record. Not sure what you mean by this. When a new record is applied from sync, its versions are set as (1,1) by the BrowserProvider. Surely that's enough? > - Detecting a conflict during fetch (local version greater than previous > sync version). We largely have this in place already for the local/remote modified timestamps (although incomplete), so a changeover should be somewhat straightforward. > - Detecting races during reconciling (local version changed since conflict > detected). That seems like something which should happen within our ContentProvider layer. That is, caller provides new values, along with an assertion of what the current versions should be to ensure data loss doesn't happen. In case conflict is detected, the change doesn't happen, and the caller is notified. What happens next? ISTM that tracking this record for upload exclusion and making sure its local version is ahead of the sync version is the thing to do. This will ensure that we don't upload local unmerged changes, as that will mean erasing any unapplied server changes for this record. It also ensures that our conflicting record will be merged/uploaded during the next sync. On a ContentProvider level, we'll need to be careful about the whole "read then conditionally write" thing. > - Handling reconciled conflicts by bumping the local version to be higher > than the sync version. Yup, as per Comment 71 (although I did miss more than just one important thing). Thankfully I did add storeDelegate.onRecordStoreReconciled recently, which should come in very handy here for keeping track of the correct versions in the middleware.

(In reply to :Grisha Kruglov from comment #80) > > - Advancing the local version when we apply a new remote record. > > Not sure what you mean by this. When a new record is applied from sync, its > versions are set as (1,1) by the BrowserProvider. Surely that's enough? Sorry, I meant "new" in the sense of "we just downloaded a new changed version of an existing local record". As far as I can tell, this is the lifecycle with your current patch: Create local: 1 | 0 Upload: 1 | 1 Download modified: 1 | 2 Change local: 2 | 2 << oops! We won't spot the change. > > - Detecting races during reconciling (local version changed since conflict > > detected). > > That seems like something which should happen within our ContentProvider > layer. That is, caller provides new values, along with an assertion of what > the current versions should be to ensure data loss doesn't happen. Perhaps, but… > In case conflict is detected, the change doesn't happen, and the caller is > notified. What happens next? ISTM that tracking this record for upload > exclusion and making sure its local version is ahead of the sync version is > the thing to do. I would suggest re-reconciling. After all, you have the remote record that you just compared against the previous local record; just do it again and try to apply it until you succeed. (In theory you could do three-way merge!) > This will ensure that we don't upload local unmerged changes, as that will > mean erasing any unapplied server changes for this record. It also ensures > that our conflicting record will be merged/uploaded during the next sync. 'cept you have to worry about moves, deletions, added children…

(In reply to Richard Newman [:rnewman] from comment #81) > (In reply to :Grisha Kruglov from comment #80) > > > > - Advancing the local version when we apply a new remote record. > > > > Not sure what you mean by this. When a new record is applied from sync, its > > versions are set as (1,1) by the BrowserProvider. Surely that's enough? > > Sorry, I meant "new" in the sense of "we just downloaded a new changed > version of an existing local record". > > As far as I can tell, this is the lifecycle with your current patch: > > Create local: 1 | 0 > Upload: 1 | 1 > Download modified: 1 | 2 > Change local: 2 | 2 << oops! We won't spot the change. I don't think think that's correct. Here is what I think will currently happen in that scenario: l.v. s.v. Create local: 1 | 0 Upload: 1 | 1 Download modified: 1 | 1 -> record is merged, but versions are left untouched Change local: 2 | 1 Perhaps a clearer name would have been "syncedVersion", meaning that it reflects state of the record vis-a-vis fennec's interaction with sync, and is defined as <=localVersion. For merges which don't require an upload, this seems acceptable, even though it does not strictly reflect the reality of things (actual record changed, but not its versions). For merges which do require an upload, we do need to bump the localVersion, as was pointed out elsewhere. > > > - Detecting races during reconciling (local version changed since conflict > > > detected). > > > > That seems like something which should happen within our ContentProvider > > layer. That is, caller provides new values, along with an assertion of what > > the current versions should be to ensure data loss doesn't happen. > I would suggest re-reconciling. After all, you have the remote record that > you just compared against the previous local record; just do it again and > try to apply it until you succeed. > > (In theory you could do three-way merge!) Makes sense! > > This will ensure that we don't upload local unmerged changes, as that will > > mean erasing any unapplied server changes for this record. It also ensures > > that our conflicting record will be merged/uploaded during the next sync. > > 'cept you have to worry about moves, deletions, added children… You're right; a lot of the changes are compound operations on multiple records. What I described above is a sure way to actually corrupt a tree!

Pushed up patches which address comments around version tracking while processing downloaded bookmarks. It's a largely straightforward transformation of prior "bump a timestamp" logic so that records are picked up for an upload, with some additional bookkeeping, race detection, and intelligence around what was actually merged and what was simply replaced.

Comment on attachment 8886748 [details] Bug 1364644 - Migrate bookmarks schema and records to add version columns https://reviewboard.mozilla.org/r/157554/#review167826 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:73 (Diff revisions 2 - 8) > +public class BrowserDatabaseHelper extends SQLiteOpenHelper { > private static final String LOGTAG = "GeckoBrowserDBHelper"; > > // Replace the Bug number below with your Bug that is conducting a DB upgrade, as to force a merge conflict with any > // other patches that require a DB upgrade. > public static final int DATABASE_VERSION = 38; // Bug 1364644 Make sure all the patches that change the DB schema are all together and bump the version. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:152 (Diff revisions 2 - 8) > + > "FOREIGN KEY (" + Bookmarks.PARENT + ") REFERENCES " + > TABLE_BOOKMARKS + "(" + Bookmarks._ID + ")" + > ");"); > > db.execSQL("CREATE INDEX bookmarks_url_index ON " + TABLE_BOOKMARKS + "(" I'd be very interested to know if the migration is more efficient if you drop all the indices on `bookmarks`, migrate the schema, update each row, and then recreate the indices again. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:2072 (Diff revisions 2 - 8) > + > + // - recordCreated > lastBookmarkSync > + // -> needs to be uploaded > + > + // - recordCreated == lastBookmarkSync > + // -> needs to be uploaded (we assume that it wasn't) N.B., it's not exactly "needs to be uploaded", but "needs to be considered as changed". I'd also go so far as to say that creation time doesn't matter in this specific case -- the modified time is probably enough, no? ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:2087 (Diff revisions 2 - 8) > + // -> assume that our timestamp-based syncing did the right thing and uploaded this record > + > + // Again, since our default version values indicate that a record needs to be synced, the > + // actual data migration is simple: it's only concerned with "synced" records. > + > + // First, we need to find out if sync is setup. set up ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:2112 (Diff revisions 2 - 8) > + } catch (IOException | NonObjectJSONException e) { > + Log.e(LOGTAG, "Could not process sync SharedPreferences. Skipping bookmark version migration.", e); > + return; > + } > + > + final long lastSyncTimestamp = synchronizerConfiguration.localBundle.getTimestamp(); Log this! ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:2146 (Diff revisions 2 - 8) > + > + // Build a list of bookmark GUIDs that need to be marked as "synced". > + final ArrayList<String> guidsToMarkAsSynced = new ArrayList<>(); > + final int guidCol = allBookmarksCursor.getColumnIndexOrThrow(Bookmarks.GUID); > + final int dateCreatedCol = allBookmarksCursor.getColumnIndexOrThrow(Bookmarks.DATE_CREATED); > + final int dateModifiedCol = allBookmarksCursor.getColumnIndexOrThrow(Bookmarks.DATE_MODIFIED); You realize that these 100 lines of code can all be replaced by a single raw SQL query, right? ``` UPDATE bookmarks SET sync_version = 0 WHERE modified is not null and -- Should never happen? modified < ? ``` ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:2152 (Diff revisions 2 - 8) > + > + do { > + long dateCreated = allBookmarksCursor.getLong(dateCreatedCol); > + long dateModified = allBookmarksCursor.getLong(dateModifiedCol); > + > + // CursorWindow's getLong returns 0L for columns which are null. Use `cursor.isNull(i)` instead. Remember that the Android cursor implementation makes few guarantees. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:2159 (Diff revisions 2 - 8) > + // We'll mark these records as "needs an upload", and hope for the best. > + if (dateCreated == 0L || dateModified == 0L) { > + continue; > + } > + > + if (dateCreated < lastSyncTimestamp && dateModified < lastSyncTimestamp) { Don't check the creation date here. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:2172 (Diff revisions 2 - 8) > + guidsToMarkAsSynced.add(guid); > + } > + } while (allBookmarksCursor.moveToNext()); > + > + final int bookmarksToMarkAsSyncedCount = guidsToMarkAsSynced.size(); > + if (bookmarksToMarkAsSyncedCount == 0) { Just do `isEmpty` and move the size calculation down to where you use it.

(In reply to Richard Newman [:rnewman] from comment #12) > Comment on attachment 8886749 [details] > Bug 1364644 - Bookmark version tracking > > https://reviewboard.mozilla.org/r/157556/#review164750 > > ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:158 > (Diff revision 2) > > @RobocopTarget > > public interface TrackerSyncColumns { > > - String SYNC_STATUS = "syncStatus"; > > + String SYNC_STATUS = "syncStatus"; // new: 0, normal: 1, unknown: 2 > > String SYNC_CHANGE_COUNTER = "syncChangeCounter"; > > + > > + int SYNC_STATUS_NEW = 0; > > Document what these mean. In particular, what does `UNKNOWN` represent? > > ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1572 > (Diff revision 2) > > > > String where = Bookmarks._ID + " IN (" + > > " SELECT DISTINCT " + Bookmarks.PARENT + > > " FROM " + TABLE_BOOKMARKS + > > " WHERE " + selection + " )"; > > - return db.update(TABLE_BOOKMARKS, values, where, selectionArgs); > > + final int changed = db.update(TABLE_BOOKMARKS, values, where, selectionArgs); > > If `changed` is 0, why are you bumping the change counter? > > ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1613 > (Diff revision 2) > > > > + // Set initial sync state and change counter based on origin of insertion. > > + // It's important that insertions from Sync do not trigger another Sync to start. > > + if (isCallerSync(uri)) { > > + values.put(Bookmarks.SYNC_CHANGE_COUNTER, 0); > > + values.put(Bookmarks.SYNC_STATUS, Bookmarks.SYNC_STATUS_NORMAL); > > Why `SYNC_STATUS_NORMAL` and not `SYNC_STATUS_SYNCED`? > > ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1645 > (Diff revision 2) > > // database schema foreign key constraint. > > final long parentId = values.getAsLong(Bookmarks.PARENT); > > - db.update(TABLE_BOOKMARKS, > > - parentValues, > > - Bookmarks._ID + " = ?", > > - new String[] { String.valueOf(parentId) }); > > + final String parentSelection = Bookmarks._ID + " = ?"; > > + final String[] parentSelectionArgs = new String[] { String.valueOf(parentId) }; > > + db.update(TABLE_BOOKMARKS, parentValues, parentSelection, parentSelectionArgs); > > + incrementChangeCounter(db, uri, TABLE_BOOKMARKS, parentSelection, parentSelectionArgs); > > Why are you not incrementing the change counter inside the `update` call by > amending `parentValues`? > > Is it just because `ContentValues` isn't able to express the increment? If > so, take a look at DBUtils, specifically `UpdateOperation`, which allows you > to avoid this restriction. > > You're doing two SQL calls when one might do… and if this is not inside an > explicit transaction, you've lost atomicity. > > ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1698 > (Diff revision 2) > > } > > > > beginWrite(db); > > > > int updated = db.update(TABLE_BOOKMARKS, values, inClause, null); > > + if (!isCallerSync(uri)) { > > Again, check for 0 -- move this down below the next block. > > ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1708 > (Diff revision 2) > > trace("No update on URI: " + uri); > > return updated; > > } > > > > if (isCallerSync(uri)) { > > // Sync will handle timestamps on its own, so we don't perform the update here. > > Shouldn't you be doing some work here? > > ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2287 > (Diff revision 2) > > final List<String> chunkGUIDs = bookmarkGUIDs.subList(chunkStart, chunkEnd); > > - deleted += db.update(table, > > - values, > > - DBUtils.computeSQLInClause(chunkGUIDs.size(), bookmarkGUIDColumn), > > - chunkGUIDs.toArray(new String[chunkGUIDs.size()]) > > - ); > > + final String selection = DBUtils.computeSQLInClause(chunkGUIDs.size(), Bookmarks.GUID); > > + final String[] selectionArgs = chunkGUIDs.toArray(new String[chunkGUIDs.size()]); > > + updated += db.update(TABLE_BOOKMARKS, values, selection, selectionArgs); > > + > > + // TODO do we even need to bump a change counter here? > > Yes; you need to upload deletions. > > ::: > mobile/android/base/java/org/mozilla/gecko/db/SharedBrowserDatabaseProvider. > java:125 > (Diff revision 2) > > + return; > > + } > > + > > + // For data types which support sync change tracking, we can safely drop all deleted records > > + // that were never uploaded - that is, have a sync status 'new'. Other clients don't know > > + // about them, and don't need to know that we deleted them locally. > > Oh boy. You should talk to Mark about tombstones needing to live longer to > avoid resurrection… I wana get support and directly

Comment on attachment 8889707 [details] Bug 1364644 - Pre: Refactor bookmark and history sessions to allow for different superclasses https://reviewboard.mozilla.org/r/160768/#review168060

Comment on attachment 8886748 [details] Bug 1364644 - Migrate bookmarks schema and records to add version columns https://reviewboard.mozilla.org/r/157554/#review168428 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:2071 (Diff revisions 8 - 10) > - // For each record, we need to figure out if it needs to be uploaded. > + // For each record, we need to figure out if it needs to be considered as changed. > + // Even though we have both created and modified timestamp, timestamp-based sync looks only > + // at modified timestamp - we do the same here, to stay consistent with prior semantics. > > - // - recordCreated > lastBookmarkSync > - // -> needs to be uploaded > + // - recordModified >= lastBookmarkSync > + // -> consider it changed since last sync This is an interesting conundrum, no? Every record downloaded from the server on a fresh device will have lastModified = lastSync. Isn't the safest thing to do to assume that a record was added by Sync, not that a user bookmarked a record at the exact instant that a sync download finished?

Comment on attachment 8889636 [details] Bug 1364644 - Pre: Move change tracking responsibilities into repositories https://reviewboard.mozilla.org/r/160658/#review168440

Comment on attachment 8889707 [details] Bug 1364644 - Pre: Refactor bookmark and history sessions to allow for different superclasses https://reviewboard.mozilla.org/r/160768/#review168450 ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/middleware/VersionedMiddlewareRepository.java:106 (Diff revision 7) > + session, this.repository > + )); > + } > + } > + > + private class VersionedMiddlewareRepositorySession extends MiddlewareRepositorySession { I'm still not sure why this is middleware rather than diving into the class hierarchy somewhere around `StoreTrackingRepositorySession`. I mean, at some point you hook this into `AndroidBrowserBookmarksRepositorySession`, which should _be_ a versioned repo, right?

Comment on attachment 8889638 [details] Bug 1364644 - Versioned syncing of bookmarks https://reviewboard.mozilla.org/r/160662/#review168458 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1718 (Diff revisions 7 - 10) > - // If the update is coming from Sync, we're done. Sync updates do not affect localVersion, > - // and Sync will handle timestamps of parent records on its own. > + // If the update is coming from Sync, check if it explicitly asked to increment localVersion. > + // Otherwise, Sync updates do not affect localVersion, and Sync will handle timestamps of > + // parent records on its own, which means we're done. > if (isCallerSync(uri)) { > + if (shouldIncrementLocalVersionFromSync(uri)) { > + incrementLocalVersion(db, uri, TABLE_BOOKMARKS, selection, selectionArgs); Do you really mean increment here? Isn't the logic actually to bring local up to equal remote? ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java:440 (Diff revisions 7 - 10) > protected Record retrieveDuringFetch(Cursor cur) throws NoGuidForIdException, NullCursorException, ParentNotFoundException { > final Record retrievedRecord = retrieveRecord(cur, true); > > if (retrievedRecord != null) { > - // In addition to the regular bookmark fields, we will need to know the local version of this record. > - // It is used by the VersionedMiddlewareRepository alongside with syncVersion to track records' sync state. > + // In addition to the regular bookmark fields, we will need to know local and sync versions of > + // this record. They're used by VersionedMiddlewareRepository to track sync state state of records, state state ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java:449 (Diff revisions 7 - 10) > > return retrievedRecord; > } > > + private Record populateVersionFields(@Nullable Record record, Cursor cur) { > + if (record != null) { Early return.

The final remaining piece of the puzzle: make version tracking sync logic live somewhere in a RepositorySession inheritance, as opposed to a middleware, which as Richard rightly argued probably isn't a good long-term place for this functionality. In order to actually achieve this, ISTM we need to pull apart our inheritance structure a bit. Here's the current class hierarchy for sessions: RepositorySession -> StoreTrackingRepositorySession --> PassworsdRepositorySession --> TabsRepositorySession --> AndroidBrowserRepositorySession ---> AndroidBrowserBookmarksRepositorySession ---> AndroidBrowserHistoryRepositorySession And we'll need to end up with the following: RepositorySession -> StoreTrackingRepositorySession --> PassworsdRepositorySession --> TabsRepositorySession --> HistoryRepositorySession --> VersionedRepositorySession ---> BookmarksRepositorySession The crux of this work is, quite literally, a textbook example of "tease apart inheritance refactoring" (see Fowler's Refactoring). The goal is to kill off AndroidBrowserRepositorySession base class. Roughly, it's doing two types of things: fetching and storing. Both of these operations have a very similar shape for both history and bookmarks, but actually do different things. And so, we do two things: - replace the base class with two separate inheritance chains - abstract FetchingSessionHelper and StoringSessionHelper which define general shape of operations, along with their two concrete implementations for history and bookmarks - in actual history/bookmark sessions, don't extend the base class anymore, and instead delegate fetch and store operations to instances of the above Generally this is all fairly mechanical, just needs some patience and a good test suite. Now, what does all of this effort actually buy us? - cleaner separation between history and bookmark session. We won't completely untangle this mess, but it will be cleaner and hopefully less confusing at a glance. - this should make future work on improving bookmark syncing easier, if only because I'll be more familiar with this code, but also because we'll have better "lines in the sand". - there's been enough churn in the AndroidBrowserRepositorySession that things are starting to look pretty clumsy. - a neater path forward for other repositories to move to versioned syncing. What if _not_ do this refactoring and stick with the middleware approach? What does _that_ buy? - saves me from this work! For now... Likely we'd need to do this sooner rather than later anyway if we want to significantly improving bookmark sync - less churn around the likely follow-up bug fixes. Our test suite is probably going to miss things here and there. - get to land this series sooner! Also: - I took a first stab at the refactoring, and "it doesn't look too bad". There's a chance that if I didn't take time to write this post, I'd have a largely correct version by now. - After talking to product, we've moved the Full Bookmark Management release to 57, which removes time pressure and gives me time to actually do this work now and follow through in the 57 nightly cycle, as opposed to "sometime later, maybe"

Pushed up the refactor (which is nice to have, but wasn't not strictly necessary), and a new, simpler iteration of the versioned sync which seems to account for all of the review feedback. Works well locally, let's see how it does on try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=c447a7541a11fc75b9a1e95b5854343d20b6d0ec

Comment on attachment 8886750 [details] Bug 1364644 - Pre: Remove guidsSince RepositorySession interface https://reviewboard.mozilla.org/r/157552/#review171926 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1678 (Diff revision 18) > // database schema foreign key constraint. > final long parentId = values.getAsLong(Bookmarks.PARENT); > - db.update(TABLE_BOOKMARKS, > - parentValues, > - Bookmarks._ID + " = ?", > - new String[] { String.valueOf(parentId) }); > + final String parentSelection = Bookmarks._ID + " = ?"; > + final String[] parentSelectionArgs = new String[] { String.valueOf(parentId) }; > + db.update(TABLE_BOOKMARKS, parentValues, parentSelection, parentSelectionArgs); > + // TODO DBUtils.updateArrays TODO ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1762 (Diff revision 18) > final long lastModified = values.getAsLong(Bookmarks.DATE_MODIFIED); > final ContentValues parentValues = new ContentValues(); > parentValues.put(Bookmarks.DATE_MODIFIED, lastModified); > > // Bump old/new parent's lastModified timestamps. > - updated += db.update(TABLE_BOOKMARKS, parentValues, > + final String parentSelection = Bookmarks._ID + " in (?, ?)"; While we're here, s/in/IN ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1765 (Diff revision 18) > > // Bump old/new parent's lastModified timestamps. > - updated += db.update(TABLE_BOOKMARKS, parentValues, > - Bookmarks._ID + " in (?, ?)", > - new String[] { String.valueOf(oldParentId), String.valueOf(newParentId) }); > + final String parentSelection = Bookmarks._ID + " in (?, ?)"; > + final String[] parentSelectionArgs = new String[] { String.valueOf(oldParentId), String.valueOf(newParentId) }; > + updated += db.update(TABLE_BOOKMARKS, parentValues, parentSelection, parentSelectionArgs); > + // TODO replace this with DBUtils.updateArrays TODO ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2326 (Diff revision 18) > chunkEnd = bookmarkGUIDs.size(); > } > final List<String> chunkGUIDs = bookmarkGUIDs.subList(chunkStart, chunkEnd); > - deleted += db.update(table, > - values, > - DBUtils.computeSQLInClause(chunkGUIDs.size(), bookmarkGUIDColumn), > + final String selection = DBUtils.computeSQLInClause(chunkGUIDs.size(), Bookmarks.GUID); > + final String[] selectionArgs = chunkGUIDs.toArray(new String[chunkGUIDs.size()]); > + // TODO DBUtils.updateArrays TODO ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2619 (Diff revision 18) > + > + return changed; > + } > + > + private boolean updateBookmarkByGuidAssertingLocalVersion(Uri uri, SQLiteDatabase db, Bundle extras) { > + String table; Pull this down after the switch -- it's a constant value! We either do `TABLE_BOOKMARKS` or we throw. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2651 (Diff revision 18) > + // obtaining an EXCLUSIVE lock will prevent this at a cost of concurrency. > + // and since we'll be running this query quite a bit during sync, that cost might be great. > + db.beginTransactionNonExclusive(); > + try { > + final Cursor c = db.query(table, new String[] {BrowserContract.VersionColumns.LOCAL_VERSION}, BrowserContract.SyncColumns.GUID + " = ?", new String[]{guid}, null, null, null); > + if (c.getCount() != 1) { Implement this check for a very unlikely situation by calling `moveToNext` after extracting the data. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2705 (Diff revision 18) > + // re-wrap data in, say, a list of ContentValues. > + final ConcurrentHashMap<String, Integer> syncVersionsForGuids = uncheckedCastSerializableToHashMap( > + data.getSerializable(BrowserContract.METHOD_PARAM_DATA) > + ); > + > + String table; Same comment. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:3082 (Diff revision 18) > > return null; > } > + > + /** > + * A note on record version tracking, applicable to records that support it. s/records/repositories ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:3086 (Diff revision 18) > + /** > + * A note on record version tracking, applicable to records that support it. > + * (bookmarks as of Bug 1364644; same logic must apply to any record types in the future) > + * - localVersion is always incremented by 1. No other change is allowed. > + * - any modifications from Fennec increment localVersion. > + * - modifications from Sync do no increment localVersion, unless an explicit flag is passed in. do no -> do not ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:3088 (Diff revision 18) > + * (bookmarks as of Bug 1364644; same logic must apply to any record types in the future) > + * - localVersion is always incremented by 1. No other change is allowed. > + * - any modifications from Fennec increment localVersion. > + * - modifications from Sync do no increment localVersion, unless an explicit flag is passed in. > + * - syncVersion is updated "in bulk", and set to some value that's expected to be <= localVersion. > + * - localVersion and syncVersion may be reset to (1,0) - usually on sync disconnect. Not so much usually, as always on disconnect, always on reset, and at no other time. ::: mobile/android/base/java/org/mozilla/gecko/db/SharedBrowserDatabaseProvider.java:116 (Diff revision 18) > inClause = DBUtils.computeSQLInClauseFromLongs(cursor, CommonColumns._ID); > } finally { > cursor.close(); > } > > - db.delete(tableName, inClause, null); > + int deletedExpired = db.delete(tableName, inClause, null); `final` everywhere. ::: mobile/android/base/java/org/mozilla/gecko/db/SharedBrowserDatabaseProvider.java:124 (Diff revision 18) > + // For data types which support sync versioning, we can safely drop all deleted records > + // that were never uploaded. Other clients don't know about them, and don't need to know > + // that we deleted them locally. Except that your scheme does not distinguish between "needs upload" and "new" -- when node reassigned we'll mark all records as needing upload (`(1, 0)`), and then here we'll forget deletions that happened since the last sync. I think this logic is wrong: when an item is marked as deleted, genuinely delete it if it was never uploaded. That is: ``` DELETE FROM bookmarks WHERE guid = ? AND is_deleted = 0 AND sync_version = 0; UPDATE bookmarks SET is_deleted = 1 WHERE guid = ? AND sync_version <> 0; ``` ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/VersioningDelegateHelper.java:165 (Diff revision 18) > + } > + > + @Override > + public void onRecordStoreReconciled(String guid, Integer newVersion) { > + if (newVersion == null) { > + throw new IllegalArgumentException("Received null localVersioned after reconciling a versioned record."); s/localVersioned/localVersion ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksSessionHelper.java:1 (Diff revision 18) > +package org.mozilla.gecko.sync.repositories.android; License block. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksSessionHelper.java:38 (Diff revision 18) > +public class BookmarksSessionHelper extends SessionHelper implements BookmarksInsertionManager.BookmarkInserter { > + private static final String LOG_TAG = "BookmarksSessionHelper"; > + > + private final BookmarksDataAccessor dbAccessor; > + > + // TODO: synchronization for these. … ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksSessionHelper.java:57 (Diff revision 18) > + private static final int DEFAULT_INSERTION_FLUSH_THRESHOLD = 50; > + > + /** > + * = A note about folder mapping = > + * > + * Note that _none_ of Places's folders actually have a special GUID. They're all Is this still true? ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/HistorySessionHelper.java:1 (Diff revision 18) > +package org.mozilla.gecko.sync.repositories.android; License block (and for any other files missing them) ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/HistorySessionHelper.java:56 (Diff revision 18) > + * @param existingRecord the existing record. Use this to decide how to process the deletion. > + */ > + @Override > + /* package-private */ void storeRecordDeletion(RepositorySessionStoreDelegate storeDelegate, Record record, Record existingRecord) { > + // TODO: we ought to mark the record as deleted rather than purging it, > + // in order to support syncing to multiple destinations. Bug 722607. You can kill this comment. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/SessionHelper.java:197 (Diff revision 18) > + // Check that the record is a valid type. > + // Fennec only supports bookmarks and folders. All other types of records, > + // including livemarks and queries, are simply ignored. > + // See Bug 708149. This might be resolved by Fennec changing its database > + // schema, or by Sync storing non-applied records in its own private database. This hasn't been true for six years. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/SessionHelper.java:211 (Diff revision 18) > + // TODO: lift these into the session. > + // Temporary: this matches prior syncing semantics, in which only > + // the relationship between the local and remote record is considered. > + // In the future we'll track these two timestamps and use them to > + // determine which records have changed, and thus process incoming > + // records more efficiently. Is this comment still valid? ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/SessionHelper.java:297 (Diff revision 18) > + trace("Incoming record " + record.guid + " dupes to local record " + existingRecord.guid); > + > + // Populate more expensive fields prior to reconciling. > + existingRecord = transformRecord(existingRecord); > + > + // TODO for versioned records (bookmarks): TODO ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/SessionHelper.java:331 (Diff revision 18) > + reconcileAttempt += 1; > + > + } while (reconcileAttempt <= maxReconcileAttempts); > + > + if (reconcileAttempt > 1) { > + Logger.debug(LOG_TAG, "Stored after reconcile attempt #" + reconcileAttempt); This should be an `else` below the next block.

Richard will be taking a look at this and then it should be closed soon after.

(In reply to Julie McCracken (:julie) from comment #126) > Richard will be taking a look at this and then it should be closed soon > after. Yup, grisha promised me an updated version before I leave on PTO!

Comment on attachment 8886748 [details] Bug 1364644 - Migrate bookmarks schema and records to add version columns https://reviewboard.mozilla.org/r/157554/#review168428 > This is an interesting conundrum, no? > > Every record downloaded from the server on a fresh device will have lastModified = lastSync. > > Isn't the safest thing to do to assume that a record was added by Sync, not that a user bookmarked a record at the exact instant that a sync download finished? Unfortunately, this isn't the case. We don't even pass in the last_modified explicitely in our bookmark inserter. On a fresh sync we end up with a gradually increasing modified timestamps.

(In reply to :Grisha Kruglov from comment #128) > Unfortunately, this isn't the case. We don't even pass in the last_modified > explicitely in our bookmark inserter. On a fresh sync we end up with a > gradually increasing modified timestamps. Reviewboard ate my follow up comment. The more accurate way to state this is that _some_ of the records might be have modified=lastSynced timestamp at the end of a sync, and for those records it's much more likely that they were inserted/updated during the sync vs. inserted by the user at an exactly correct moment. And so, for those records it's probably safe to not require an upload. However, timestamp-based syncing uploads records that have modified>=lastSynced. So the choice is either "upload too many records we don't strictly need to" vs "miss out uploading some of the records in a highly unlikely scenario". Law of large numbers suggests that whatever we pick here, we're bound to hit _both_ of the edge cases, and so it won't be great for some of the users - or our servers. Current patches mark records with modified<=lastSynced as "don't upload", but I'm open to flipping this back to modified<lastModified.

(In reply to :Grisha Kruglov from comment #137) > So the choice is either "upload too many records we don't strictly need to" > vs "miss out uploading some of the records in a highly unlikely scenario". > Law of large numbers suggests that whatever we pick here, we're bound to hit > _both_ of the edge cases, and so it won't be great for some of the users - > or our servers. But in any case, that modified=synced user change could have occurred 50msec before (during the sync), in which case it's going to be incorrectly ignored regardless. > Current patches mark records with modified<=lastSynced as "don't upload", > but I'm open to flipping this back to modified<lastModified. I think that's the correct choice.

Comment on attachment 8886748 [details] Bug 1364644 - Migrate bookmarks schema and records to add version columns https://reviewboard.mozilla.org/r/157554/#review174044

Comment on attachment 8886749 [details] Bug 1364644 - Bookmark version tracking https://reviewboard.mozilla.org/r/157556/#review174046 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2878 (Diff revisions 9 - 15) > + * (bookmarks as of Bug 1364644; same logic must apply to any record types in the future) > + * - localVersion is always incremented by 1. No other change is allowed. > + * - any modifications from Fennec increment localVersion. > + * - modifications from Sync do not increment localVersion, unless an explicit flag is passed in. > + * - syncVersion is updated "in bulk", and set to some value that's expected to be <= localVersion. > + * - localVersion and syncVersion may jump backwards to (1,0) - always on reset, which happens And also to `(2,1)`?

Comment on attachment 8889638 [details] Bug 1364644 - Versioned syncing of bookmarks https://reviewboard.mozilla.org/r/160662/#review174048 ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:1734 (Diff revisions 10 - 16) > > beginWrite(db); > > - int updated = db.update(TABLE_BOOKMARKS, values, inClause, null); > + int updated; > + // If the update is coming from Sync, check if it explicitly asked to increment localVersion. > + if (!isCallerSync(uri) || (isCallerSync(uri) && shouldIncrementLocalVersionFromSync(uri))) { You can remove the second `isCallerSync`: ``` if (!isCallerSync(uri) || shouldIncrement…(uri)) { ``` ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2595 (Diff revisions 10 - 16) > + // Whenever we encounter node-reassignment, or otherwise need to re-upload our records, records > + // are reset to a "sync needed and record exists elsewhere" state: > + // - localVersion=2 > + // - syncVersion=1 > + > + // syncVersion>0 indicates that a record was synced at some point. synced with the current Firefox Account. In theory you could reset all records to `(1,0)` after the device signs out of account A and into account B (B != A), dropping all tombstones -- there should be no overlap between the two (hah!), and if there is it probably isn't correct to delete records that happen to share a GUID. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2618 (Diff revisions 10 - 16) > + throw new IllegalStateException("Attempting resetting sync versions for non-versioned record types"); > + } > + > + final ContentValues resetVersionsValues = new ContentValues(); > + > + if (!TextUtils.isEmpty(uri.getQueryParameter(BrowserContract.PARAM_RESET_VERSIONS_TO_SYNCED))) { Gah, content providers. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2629 (Diff revisions 10 - 16) > + } > + > + // Reset versions for data types which support versioning. Currently that's just bookmarks. > + // See Bug 1383894. > + int changed = 0; > + changed += db.update(TABLE_BOOKMARKS, resetVersionsValues, null, null); Err… `return db.update(…);`? ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java:2665 (Diff revisions 10 - 16) > // We don't care if others are reading the database at the same time. > // We'll still implicitly acquire an EXCLUSIVE lock right when we'll be executing > // an UPDATE, but at least that time window will be less than if we acquired it > // for running SELECT as well. > - // TODO iirc SELECTs overlapping with UPDATES have undefined behaviour as far as > - // data visibility goes, and so I wonder if this might lead to strange behaviour elsewhere. > + // Note that SELECTs overlapping with UPDATES have undefined behaviour as far as > + // data visibility goes acorss transactions on the same connection, and so this might lead across

Comment on attachment 8889707 [details] Bug 1364644 - Pre: Refactor bookmark and history sessions to allow for different superclasses https://reviewboard.mozilla.org/r/160768/#review174054 r+ with comments addressed. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksSessionHelper.java:1 (Diff revision 9) > +package org.mozilla.gecko.sync.repositories.android; License block. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksSessionHelper.java:35 (Diff revision 9) > +public class BookmarksSessionHelper extends SessionHelper implements BookmarksInsertionManager.BookmarkInserter { > + private static final String LOG_TAG = "BookmarksSessionHelper"; > + > + private final AndroidBrowserBookmarksDataAccessor dbAccessor; > + > + // TODO: synchronization for these. TODO ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/BookmarksSessionHelper.java:270 (Diff revision 9) > + > + /** > + * Incoming bookmark records might be missing dateAdded field, because clients started to sync it > + * only in version 55. However, record's lastModified value is a good upper bound. In order to > + * encourage modern clients to re-upload the record with an earlier local dateAdded value, > + * we bump record's lastModified value if we perform any substitutions. Comment and function body refers to the old scheme. Bump the version, not the timestamp! ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/SessionHelper.java:1 (Diff revision 9) > +package org.mozilla.gecko.sync.repositories.android; License block. (And check your other files.) ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/SessionHelper.java:242 (Diff revision 9) > + if (!remotelyModified) { > + trace("Ignoring deleted record from the past."); > + return; > + } > + > + boolean locallyModified = existingRecord.lastModified > lastLocalRetrieval; We can now do this without using `lastModified`. ::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/SessionHelper.java:300 (Diff revision 9) > + // This section of code will only run if the incoming record is not > + // marked as deleted, so we never want to just drop ours from the database: > + // we need to upload it later. > + // Allowing deleted items to propagate through `replace` allows normal > + // logging and side-effects to occur, and is no more expensive than simply > + // bumping the modified time. Comment needs to be updated.

Well, there's a lot of code here, so I've almost certainly missed some stuff. If you're confident, get it into testing and hammer on it.

Comment on attachment 8889707 [details] Bug 1364644 - Pre: Refactor bookmark and history sessions to allow for different superclasses https://reviewboard.mozilla.org/r/160768/#review174054 > Comment and function body refers to the old scheme. Bump the version, not the timestamp! Addressed this in the "version syncing" patch, by allowing sync to pass in "insert as modified" flag for certain records while inserting them, and check for it on the BrowserProvider side. Tests updated, etc.

Comment on attachment 8886748 [details] Bug 1364644 - Migrate bookmarks schema and records to add version columns https://reviewboard.mozilla.org/r/157554/#review167826 > I'd be very interested to know if the migration is more efficient if you drop all the indices on `bookmarks`, migrate the schema, update each row, and then recreate the indices again. I doubt that it will be in its current iteration. The only index relevant to the migration is on 'modified', which we query in migration's UPDATE. And there are no indecies on any of the version fields, and so we won't be rebuilding them during the migration either.

Pushed by gkruglov@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3846782bf831 Pre: Remove guidsSince RepositorySession interface r=rnewman https://hg.mozilla.org/integration/autoland/rev/2f69b4cb829c Pre: Move change tracking responsibilities into repositories r=rnewman https://hg.mozilla.org/integration/autoland/rev/210d26d08294 Pre: don't swallow runtime exceptions in the BrowserProvider's call interface r=rnewman https://hg.mozilla.org/integration/autoland/rev/d7e7927feaf3 Pre: Refactor bookmark and history sessions to allow for different superclasses r=rnewman https://hg.mozilla.org/integration/autoland/rev/866bf0a5a6b6 Migrate bookmarks schema and records to add version columns r=rnewman https://hg.mozilla.org/integration/autoland/rev/b5631dd590ae Bookmark version tracking r=rnewman https://hg.mozilla.org/integration/autoland/rev/517c934fdec5 Versioned syncing of bookmarks r=rnewman https://hg.mozilla.org/integration/autoland/rev/139c4e2e2d0e Post: remove AndroidBrowser prefix from class names r=rnewman

https://hg.mozilla.org/mozilla-central/rev/3846782bf831 https://hg.mozilla.org/mozilla-central/rev/2f69b4cb829c https://hg.mozilla.org/mozilla-central/rev/210d26d08294 https://hg.mozilla.org/mozilla-central/rev/d7e7927feaf3 https://hg.mozilla.org/mozilla-central/rev/866bf0a5a6b6 https://hg.mozilla.org/mozilla-central/rev/b5631dd590ae https://hg.mozilla.org/mozilla-central/rev/517c934fdec5 https://hg.mozilla.org/mozilla-central/rev/139c4e2e2d0e