1426556 - Store the bookmarks collection sync ID in the mirror's `meta` table

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Lina Butler [:lina]]

See bug 1305563, comment 129. The mirror has a `meta` table that currently holds the last sync time. Let's store `services.sync.bookmarks.syncID` there, too, so that the two are linked.

This is related to bug 1426554, where we'd also store the sync ID in Places. If the mirror's sync ID doesn't match what's in Places (maybe the user copied the mirror database?), or Places doesn't match what's on the server, drop the mirror and reset the sync statuses.

It's not clear to me if we ever want to wipe the server in case of a mismatch, or always pull down what's already there and merge.

Comment 1

[Lina Butler [:lina]]

(In reply to Kit Cambridge (he/him) [:kitcambridge] from comment #0)
> It's not clear to me if we ever want to wipe the server in case of a
> mismatch, or always pull down what's already there and merge.

Actually, I think the only way the sync ID would change is after a wipe, so we shouldn't wipe again. Just pull everything down and merge.

Comment 2

[Lina Butler [:lina]]

This is going to be easier once bug 1199077 lands. We can change `ensureCurrentSyncID` to check if the mirror's sync ID is up-to-date, and reset if not. `resetSyncID` can reset the mirror unconditionally.

Comment 4

[Lina Butler [:lina]]

Attachment: Bug 1426556 - Store the bookmarks sync ID in the mirror.

Review commit: https://reviewboard.mozilla.org/r/230836/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230836/

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8961988 [details]
Bug 1426556 - Store the bookmarks sync ID in the mirror.

https://reviewboard.mozilla.org/r/230836/#review236322

::: toolkit/components/places/SyncedBookmarksMirror.jsm:1992
(Diff revision 1)
>   */
>  async function initializeMirrorDatabase(db) {
> -  // Key-value metadata table. Currently stores just the server collection
> -  // last modified time.
> +  // Key-value metadata table. Stores the server collection last modified time
> +  // and sync ID.
>    await db.execute(`CREATE TABLE mirror.meta(
> -    key INTEGER PRIMARY KEY,
> +    key TEXT PRIMARY KEY,

This is a backward-incompatible schema change, but I opted not to do a migration because we're pre-dogfooding, and no one (except us!) should have the pref flipped.

We'll all need to trash `${profile}/weave/bookmarks.sqlite` after this lands.

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8961988 [details]
Bug 1426556 - Store the bookmarks sync ID in the mirror.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/230836/diff/1-2/

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8961988 [details]
Bug 1426556 - Store the bookmarks sync ID in the mirror.

https://reviewboard.mozilla.org/r/230836/#review236674


Code analysis found 5 defects in this patch:
 - 5 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: services/sync/tests/unit/test_bookmark_engine.js:1116
(Diff revision 2)
> +  await bufferedEngine.initialize();
> +  let buf = await bufferedEngine._store.ensureOpenMirror();
> +
> +  info("Places and mirror don't have sync IDs");
> +  let syncID = await bufferedEngine.resetLocalSyncID();
> +  {

Error: Nested block is redundant. [eslint: no-lone-blocks]

::: services/sync/tests/unit/test_bookmark_engine.js:1135
(Diff revision 2)
> +  }
> +
> +  info("Places and mirror have matching sync ID");
> +  await bufferedEngine.setLastSync(123.45);
> +  await bufferedEngine.ensureCurrentSyncID(syncID);
> +  {

Error: Nested block is redundant. [eslint: no-lone-blocks]

::: services/sync/tests/unit/test_bookmark_engine.js:1157
(Diff revision 2)
> +  // Directly update the sync ID in the mirror, without resetting.
> +  await buf.db.execute(`UPDATE meta SET value = :value WHERE key = :key`,
> +                       { key: SyncedBookmarksMirror.META_KEY.SYNC_ID,
> +                         value: "syncIdAAAAAA" });
> +  await bufferedEngine.ensureCurrentSyncID(syncID);
> +  {

Error: Nested block is redundant. [eslint: no-lone-blocks]

::: services/sync/tests/unit/test_bookmark_engine.js:1176
(Diff revision 2)
> +      "Should reset high water mark on mirror sync ID mismatch");
> +  }
> +
> +  info("Places has sync ID; mirror missing sync ID");
> +  await buf.reset();
> +  {

Error: Nested block is redundant. [eslint: no-lone-blocks]

::: services/sync/tests/unit/test_bookmark_engine.js:1187
(Diff revision 2)
> +
> +  info("Places missing sync ID; mirror has sync ID");
> +  await buf.setCollectionLastModified(123.45);
> +  await PlacesSyncUtils.bookmarks.reset();
> +  let newSyncID = await bufferedEngine.ensureCurrentSyncID("syncIdBBBBBB");
> +  {

Error: Nested block is redundant. [eslint: no-lone-blocks]

Comment 8

[Lina Butler [:lina]]

Comment on attachment 8961988 [details]
Bug 1426556 - Store the bookmarks sync ID in the mirror.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/230836/diff/2-3/

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8961988 [details]
Bug 1426556 - Store the bookmarks sync ID in the mirror.

https://reviewboard.mozilla.org/r/230836/#review236878

Nice to see this hitting the end-game :)

::: toolkit/components/places/SyncedBookmarksMirror.jsm:263
(Diff revision 3)
> +
> +  /**
> +   * Ensures that the sync ID in the mirror is up-to-date with the server and
> +   * Places, and discards the mirror on mismatch.
> +   *
> +   * We store the same sync ID in Places and the mirror to "tie" them together,

This comment implies that this function will be storing the id in places, but AFAICT, it doesn't. I think changing "We" to "The bookmarks engine" or similar would do.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:279
(Diff revision 3)
> +    if (!newSyncId || typeof newSyncId != "string") {
> +      throw new TypeError("Invalid new bookmarks sync ID");
> +    }
> +    let existingSyncId = await this.getSyncId();
> +    if (existingSyncId == newSyncId) {
> +      MirrorLog.debug("Sync ID up-to-date in mirror", { existingSyncId });

nit: I think this might be better as trace

::: toolkit/components/places/SyncedBookmarksMirror.jsm:282
(Diff revision 3)
> +    let existingSyncId = await this.getSyncId();
> +    if (existingSyncId == newSyncId) {
> +      MirrorLog.debug("Sync ID up-to-date in mirror", { existingSyncId });
> +      return;
> +    }
> +    MirrorLog.debug("Sync ID changed from ${existingSyncId} to " +

and this at .info (not that there's a meaningful difference between debug and info in practice)

Comment 10

[Lina Butler [:lina]]

Comment on attachment 8961988 [details]
Bug 1426556 - Store the bookmarks sync ID in the mirror.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/230836/diff/3-4/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2b380031331
Store the bookmarks sync ID in the mirror. r=markh

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b2b380031331