Closed Bug 1426556 Opened 2 years ago Closed Last year

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

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: lina, Assigned: lina)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
(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.
Priority: -- → P2
Blocks: 1433177
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.
Mentor: kit
Depends on: 1199077
Duplicate of this bug: 1432681
Assignee: nobody → kit
Mentor: kit
Status: NEW → ASSIGNED
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 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 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)
Attachment #8961988 - Flags: review?(markh) → review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2b380031331
Store the bookmarks sync ID in the mirror. r=markh
https://hg.mozilla.org/mozilla-central/rev/b2b380031331
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.