Bug 1582377 - Don't recreate existing bookmarks mirror indexes in schema migrations. r=tcsc a=lizzard
authorLina Cambridge <lina@yakshaving.ninja>
Thu, 19 Sep 2019 21:17:22 +0000
changeset 555294 db46e193a5c6b129453bf8e44466f8a31a3e80ad
parent 555293 30741417bbc39489400d0fd3d261033f7026418b
child 555295 23e4deb8581f5d6f88b198810be5b1415b4e4f5f
push id2165
push userffxbld-merge
push dateMon, 14 Oct 2019 16:30:58 +0000
treeherdermozilla-release@0eae18af659f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcsc, lizzard
bugs1582377
milestone70.0
Bug 1582377 - Don't recreate existing bookmarks mirror indexes in schema migrations. r=tcsc a=lizzard Downgrading a profile and syncing downgrades the bookmarks mirror schema version, but leaves the new indexes in place. This causes the migration logic to throw an "index already exists" error on upgrade. The fix is to add an `IF NOT EXISTS`, to avoid recreating the index. Differential Revision: https://phabricator.services.mozilla.com/D46434
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/test_bookmark_mirror_migration.js
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -1472,22 +1472,23 @@ async function attachAndInitMirrorDataba
 async function migrateMirrorSchema(db, currentSchemaVersion) {
   if (currentSchemaVersion < 5) {
     // The mirror was pref'd off by default for schema versions 1-4.
     throw new DatabaseCorruptError(
       `Can't migrate from schema version ${currentSchemaVersion}; too old`
     );
   }
   if (currentSchemaVersion < 6) {
-    await db.execute(`CREATE INDEX mirror.itemURLs ON items(urlId)`);
-    await db.execute(`CREATE INDEX mirror.itemKeywords ON items(keyword)
-                      WHERE keyword NOT NULL`);
+    await db.execute(`CREATE INDEX IF NOT EXISTS mirror.itemURLs ON
+                      items(urlId)`);
+    await db.execute(`CREATE INDEX IF NOT EXISTS mirror.itemKeywords ON
+                      items(keyword) WHERE keyword NOT NULL`);
   }
   if (currentSchemaVersion < 7) {
-    await db.execute(`CREATE INDEX mirror.structurePositions ON
+    await db.execute(`CREATE INDEX IF NOT EXISTS mirror.structurePositions ON
                       structure(parentGuid, position)`);
   }
 }
 
 /**
  * Initializes a new mirror database, creating persistent tables, indexes, and
  * roots.
  *
--- a/toolkit/components/places/tests/sync/test_bookmark_mirror_migration.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_mirror_migration.js
@@ -1,29 +1,74 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
 // Keep in sync with `SyncedBookmarksMirror.jsm`.
 const CURRENT_MIRROR_SCHEMA_VERSION = 7;
 
+// The oldest schema version that we support. Any databases with schemas older
+// than this will be dropped and recreated.
+const OLDEST_SUPPORTED_MIRROR_SCHEMA_VERSION = 5;
+
 async function getIndexNames(db, table, schema = "mirror") {
   let rows = await db.execute(`PRAGMA ${schema}.index_list(${table})`);
   let names = [];
   for (let row of rows) {
     // Column 4 is `c` if the index was created via `CREATE INDEX`, `u` if
     // via `UNIQUE`, and `pk` if via `PRIMARY KEY`.
     let wasCreated = row.getResultByIndex(3) == "c";
     if (wasCreated) {
       // Column 2 is the name of the index.
       names.push(row.getResultByIndex(1));
     }
   }
   return names.sort();
 }
 
+add_task(async function test_migrate_after_downgrade() {
+  await PlacesTestUtils.markBookmarksAsSynced();
+
+  let dbFile = await setupFixtureFile("mirror_v5.sqlite");
+  let oldBuf = await SyncedBookmarksMirror.open({
+    path: dbFile.path,
+    recordTelemetryEvent(object, method, value, extra) {},
+    recordStepTelemetry() {},
+    recordValidationTelemetry() {},
+  });
+
+  info("Downgrade schema version to oldest supported");
+  await oldBuf.db.setSchemaVersion(
+    OLDEST_SUPPORTED_MIRROR_SCHEMA_VERSION,
+    "mirror"
+  );
+  await oldBuf.finalize();
+
+  let buf = await SyncedBookmarksMirror.open({
+    path: dbFile.path,
+    recordTelemetryEvent(object, method, value, extra) {},
+    recordStepTelemetry() {},
+    recordValidationTelemetry() {},
+  });
+
+  // All migrations between `OLDEST_SUPPORTED_MIRROR_SCHEMA_VERSION` should
+  // be idempotent. When we downgrade, we roll back the schema version, but
+  // leave the schema changes in place, since we can't anticipate what a
+  // future version will change.
+  let schemaVersion = await buf.db.getSchemaVersion("mirror");
+  equal(
+    schemaVersion,
+    CURRENT_MIRROR_SCHEMA_VERSION,
+    "Should upgrade downgraded mirror schema"
+  );
+
+  await buf.finalize();
+  await PlacesUtils.bookmarks.eraseEverything();
+  await PlacesSyncUtils.bookmarks.reset();
+});
+
 // Migrations between 5 and 7 add three indexes.
 add_task(async function test_migrate_from_5_to_current() {
   await PlacesTestUtils.markBookmarksAsSynced();
 
   let dbFile = await setupFixtureFile("mirror_v5.sqlite");
   let buf = await SyncedBookmarksMirror.open({
     path: dbFile.path,
     recordTelemetryEvent(object, method, value, extra) {},