Bug 1451537 - Bump bookmarks mirror schema version and add a migration test. r=mak
authorKit Cambridge <kit@yakshaving.ninja>
Thu, 12 Apr 2018 17:38:08 -0700
changeset 467937 8dbdf2617258f1806e9277bb4750f263cae6f121
parent 467936 1fa6af3138d54a43e41f668e62ecc8cd22d34308
child 467938 46ea55337b33a0c5aa7b2cc71848139e7976f293
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1451537
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1451537 - Bump bookmarks mirror schema version and add a migration test. r=mak MozReview-Commit-ID: FKMGxauwSyi
toolkit/components/places/SyncedBookmarksMirror.jsm
toolkit/components/places/tests/sync/mirror_corrupt.sqlite
toolkit/components/places/tests/sync/mirror_v1.sqlite
toolkit/components/places/tests/sync/test_bookmark_mirror_migration.js
toolkit/components/places/tests/sync/xpcshell.ini
--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -80,17 +80,17 @@ XPCOMUtils.defineLazyGetter(this, "UserC
 const DB_URL_LENGTH_MAX = 65536;
 const DB_TITLE_LENGTH_MAX = 4096;
 const DB_DESCRIPTION_LENGTH_MAX = 256;
 
 const SQLITE_MAX_VARIABLE_NUMBER = 999;
 
 // The current mirror database schema version. Bump for migrations, then add
 // migration code to `migrateMirrorSchema`.
-const MIRROR_SCHEMA_VERSION = 1;
+const MIRROR_SCHEMA_VERSION = 2;
 
 // Use a shared jankYielder in these functions
 XPCOMUtils.defineLazyGetter(this, "maybeYield", () => Async.jankYielder());
 function yieldingIterator(collection) {
   return Async.yieldingIterator(collection, maybeYield);
 }
 
 /**
@@ -158,42 +158,41 @@ class SyncedBookmarksMirror {
    * @return {SyncedBookmarksMirror}
    *         A mirror ready for use.
    */
   static async open(options) {
     let db = await Sqlite.cloneStorageConnection({
       connection: PlacesUtils.history.DBConnection,
       readOnly: false,
     });
+    let whyFailed = "initialize";
     try {
+      await db.execute(`PRAGMA foreign_keys = ON`);
       try {
-        await db.execute(`ATTACH :mirrorPath AS mirror`,
-                         { mirrorPath: options.path });
+        await attachAndInitMirrorDatabase(db, options.path);
       } catch (ex) {
-        if (ex.errors && isDatabaseCorrupt(ex.errors[0])) {
+        if (isDatabaseCorrupt(ex)) {
           MirrorLog.warn("Error attaching mirror to Places; removing and " +
                          "recreating mirror", ex);
-          options.recordTelemetryEvent("mirror", "open", "error",
+          options.recordTelemetryEvent("mirror", "open", "retry",
                                        { why: "corrupt" });
+
+          whyFailed = "remove";
           await OS.File.remove(options.path);
-          await db.execute(`ATTACH :mirrorPath AS mirror`,
-                           { mirrorPath: options.path });
+
+          whyFailed = "replace";
+          await attachAndInitMirrorDatabase(db, options.path);
         } else {
           MirrorLog.warn("Unrecoverable error attaching mirror to Places", ex);
           throw ex;
         }
       }
-      await db.execute(`PRAGMA foreign_keys = ON`);
-      await db.executeTransaction(async function() {
-        await migrateMirrorSchema(db);
-        await initializeTempMirrorEntities(db);
-      });
     } catch (ex) {
       options.recordTelemetryEvent("mirror", "open", "error",
-                                   { why: "initialize" });
+                                   { why: whyFailed });
       await db.close();
       throw ex;
     }
     return new SyncedBookmarksMirror(db, options);
   }
 
   /**
    * Returns the newer of the bookmarks collection last modified time, or the
@@ -1876,42 +1875,97 @@ SyncedBookmarksMirror.META_KEY = {
   LAST_MODIFIED: "collection/lastModified",
   SYNC_ID: "collection/syncId",
 };
 
 /**
  * An error thrown when the merge can't proceed because the local or remote
  * tree is inconsistent.
  */
-SyncedBookmarksMirror.ConsistencyError =
-  class ConsistencyError extends Error {};
+class ConsistencyError extends Error {
+  constructor(message) {
+    super(message);
+    this.name = "ConsistencyError";
+  }
+}
+SyncedBookmarksMirror.ConsistencyError = ConsistencyError;
+
+/**
+ * An error thrown when the mirror database is corrupt, or can't be migrated to
+ * the latest schema version, and must be replaced.
+ */
+class DatabaseCorruptError extends Error {
+  constructor(message) {
+    super(message);
+    this.name = "DatabaseCorruptError";
+  }
+}
 
 // Indicates if the mirror should be replaced because the database file is
 // corrupt.
 function isDatabaseCorrupt(error) {
-  return error instanceof Ci.mozIStorageError &&
-         (error.result == Ci.mozIStorageError.CORRUPT ||
-          error.result == Ci.mozIStorageError.NOTADB);
+  if (error instanceof DatabaseCorruptError) {
+    return true;
+  }
+  if (error.errors) {
+    return error.errors.some(error =>
+      error instanceof Ci.mozIStorageError &&
+      (error.result == Ci.mozIStorageError.CORRUPT ||
+       error.result == Ci.mozIStorageError.NOTADB));
+  }
+  return false;
+}
+
+/**
+ * Attaches a cloned Places database connection to the mirror database,
+ * migrates the mirror schema to the latest version, and creates temporary
+ * tables, views, and triggers.
+ *
+ * @param {Sqlite.OpenedConnection} db
+ *        The Places database connection.
+ * @param {String} path
+ *        The full path to the mirror database file.
+ */
+async function attachAndInitMirrorDatabase(db, path) {
+  await db.execute(`ATTACH :path AS mirror`, { path });
+  try {
+    await db.executeTransaction(async function() {
+      let currentSchemaVersion = await db.getSchemaVersion("mirror");
+      if (currentSchemaVersion > 0) {
+        if (currentSchemaVersion < MIRROR_SCHEMA_VERSION) {
+          await migrateMirrorSchema(db, currentSchemaVersion);
+        }
+      } else {
+        await initializeMirrorDatabase(db);
+      }
+      // Downgrading from a newer profile to an older profile rolls back the
+      // schema version, but leaves all new columns in place. We'll run the
+      // migration logic again on the next upgrade.
+      await db.setSchemaVersion(MIRROR_SCHEMA_VERSION, "mirror");
+      await initializeTempMirrorEntities(db);
+    });
+  } catch (ex) {
+    await db.execute(`DETACH mirror`);
+    throw ex;
+  }
 }
 
 /**
  * Migrates the mirror database schema to the latest version.
  *
  * @param {Sqlite.OpenedConnection} db
  *        The mirror database connection.
+ * @param {Number} currentSchemaVersion
+ *        The current mirror database schema version.
  */
-async function migrateMirrorSchema(db) {
-  let currentSchemaVersion = await db.getSchemaVersion("mirror");
-  if (currentSchemaVersion < 1) {
-    await initializeMirrorDatabase(db);
+async function migrateMirrorSchema(db, currentSchemaVersion) {
+  if (currentSchemaVersion < 2) {
+    throw new DatabaseCorruptError(`Can't migrate from schema version ${
+      currentSchemaVersion}; too old`);
   }
-  // Downgrading from a newer profile to an older profile rolls back the
-  // schema version, but leaves all new columns in place. We'll run the
-  // migration logic again on the next upgrade.
-  await db.setSchemaVersion(MIRROR_SCHEMA_VERSION, "mirror");
 }
 
 /**
  * Initializes a new mirror database, creating persistent tables, indexes, and
  * roots.
  *
  * @param {Sqlite.OpenedConnection} db
  *        The mirror database connection.
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/sync/mirror_corrupt.sqlite
@@ -0,0 +1,1 @@
+Not a database!
new file mode 100644
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..f0b88536167a2afeae7facad33e828588607570e
GIT binary patch
literal 294912
zc%1Fk%WfQ5835pJU*d~o0U9E2ASo6W_DtsDVTI60ICf=h#<3Z@tz=eBJ6%pn>F&x@
zR|f(}<$%Nj@dzx~vEdbv*sz5MKtdoMfGs<ys`jNl9S24NHH+_)s=Dewm-^2+b*c|P
z-i@;;I7!lOD+|6+JFV60wQmPOtyY^Vj)mH#x#E}^xv!rcuQOG9=gWUx%KuTDfAPy&
zKFI(0<6HT6e)RSa^A~?A0ssI20000000000000000000000000000000001=(+Bf6
zmT%vzAH}_P^n*d#*>3fpME&BTzp!}_ZZyJRXKyQf7+e$u`+Mb0fAvXmyY}*Hb92i#
zZ`OaZlC_RIQPz6gFOT`NwvCOuyJ1j9Ru_XpoJHN8cF|=c{4hKS_VydW-qG%Ea1h=L
z55m38@UW`VUya*qRfFpkNo<9?VeyE~jl<22t?*7&y?9d42p={sdM=`CFW<T`%<-F5
zj%D)YF?S`$YDmKzFZ28E!Os1SgHM8w!cT_%KOPjNR~|5o7p-3vE~jX0u*0G>dcK|X
zvsYx>o1I&}eY^hCx2kOWSvq)>4bsbt8&~qa2(Mm~;P@b(wbH1UeK=loY!%AhUcV04
zEyS~=A7^pW8ykH&#NfeBV|)Lo5ghD4*xCBo+VtG=@^bwb>(zj(Cs(JLD+3*7^f^}O
zXm97^qwq7+{_dMW<4F|sqx5-{2Hm6`pTto+$l`8vCy0ANH|})ee)K5mwfpY`Z@ybK
z8rHslUU}^8-3sq*9PKuO_lD2tMNzwdKT01*!QK7+-Ed>?+NyDXD=KzP(doad`Lx)e
z*FX8~^((`Blt!&;_S>!OHRCEvH`?w0U`Sb<b)sroFW&c&Z9MYcRNLs)FU{qfau|*-
z$<ye?_mi|e(tELs?<Jjfl<u{<qfc!|{YPp1tQy&6w3D>jJH5lW9UZsQ^LZQ=^}DS!
zyPG6Wi{Gc8>FlIfrK5x0kx2R0U5ymex!>!&2#(LX=m)K06+RtAX<U2=yMto?K50FV
zf}+*stZr_Hn;)$fyCDo7Y=?X2D;KP;-+6y6Xq2Jh^o?wZE5hBw@O*|3hszP}ZIx5H
zwl;HP`Ge(pt@>K`KkXD5H(P@&sqUNA7HO_mKeG?()kp8;8`ZAr7N6$wn7Xp3%E;>Z
zA`ExU*oJ!E>I|YQ+xP2JH<rJ3^SUI;KAY?1dAj&}nJ@oimdexVB>&|A0000000000
z0000000000000000000000000003a}7R%G=gp&XO0000000000000000000000000
z00000000000DuWwDvsIwd$s&8`S0>y<)_6Z000000000000000000000000000000
z000000002|*WH@^Qf*~^VZJ_p5$Yz#aVKh4rf<!Rl@EF+MS1(Ie0scn)EhLL=k2G)
z%CjWt9JkW5@_Jc5KU$t9Np{hGu?WoOzp3Sa&;OkNA^&Z02><{9000000000000000
z00000000000000000000|JS}ay;2|aPU23~ZkA@sK$aw(<5pS)roJ$}vXCZ8);v>I
z=qATSi{brj6^MF+^9s|8MZdFor<VUM|9$@J{O83b0000000000000000000000000
z00000000000002|PknV}Wu-pooy47}-7KX^k~PmveWfU!&yu8b+)BqvtEQuY(d&O!
zrQPJX=y<$zy6kwN8}$aSYP(b>b~@pS0RR91000000000000000000000000000000
z0001h30o>prxQ*B000000000000000000000000000000000000000cY_U9@PB;kw
l00000000000000000000000000000000000005Y<e*q^{P^bU^
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/sync/test_bookmark_mirror_migration.js
@@ -0,0 +1,77 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Migrations between 1 and 2 discard the entire database.
+add_task(async function test_migrate_from_1_to_2() {
+  let dbFile = do_get_file("mirror_v1.sqlite");
+  let telemetryEvents = [];
+  let buf = await SyncedBookmarksMirror.open({
+    path: dbFile.path,
+    recordTelemetryEvent(object, method, value, extra) {
+      telemetryEvents.push({ object, method, value, extra });
+    },
+  });
+  deepEqual(telemetryEvents, [{
+    object: "mirror",
+    method: "open",
+    value: "retry",
+    extra: { why: "corrupt" },
+  }]);
+  await buf.finalize();
+});
+
+add_task(async function test_database_corrupt() {
+  let corruptFile = do_get_file("mirror_corrupt.sqlite");
+  let telemetryEvents = [];
+  let buf = await SyncedBookmarksMirror.open({
+    path: corruptFile.path,
+    recordTelemetryEvent(object, method, value, extra) {
+      telemetryEvents.push({ object, method, value, extra });
+    },
+  });
+  deepEqual(telemetryEvents, [{
+    object: "mirror",
+    method: "open",
+    value: "retry",
+    extra: { why: "corrupt" },
+  }]);
+  await buf.finalize();
+});
+
+add_task(async function test_database_readonly() {
+  let dbFile = do_get_file("mirror_v1.sqlite");
+  try {
+    await OS.File.setPermissions(dbFile.path, {
+      unixMode: 0o400,
+      winAttributes: { readOnly: true },
+    });
+  } catch (ex) {
+    if (ex instanceof OS.File.Error &&
+        ex.unixErrno == OS.Constants.libc.EPERM) {
+      info("Skipping test; can't change database mode to read-only");
+      return;
+    }
+    throw ex;
+  }
+  try {
+    let telemetryEvents = [];
+    await Assert.rejects(SyncedBookmarksMirror.open({
+      path: dbFile.path,
+      recordTelemetryEvent(object, method, value, extra) {
+        telemetryEvents.push({ object, method, value, extra });
+      },
+    }), ex => ex.errors && ex.errors[0].result == Ci.mozIStorageError.READONLY,
+      "Should not try to recreate read-only database");
+    deepEqual(telemetryEvents, [{
+      object: "mirror",
+      method: "open",
+      value: "error",
+      extra: { why: "initialize" },
+    }], "Should record event for read-only error");
+  } finally {
+    await OS.File.setPermissions(dbFile.path, {
+      unixMode: 0o644,
+      winAttributes: { readOnly: false },
+    });
+  }
+});
--- a/toolkit/components/places/tests/sync/xpcshell.ini
+++ b/toolkit/components/places/tests/sync/xpcshell.ini
@@ -1,18 +1,21 @@
 [DEFAULT]
 head = head_sync.js
 support-files =
   livemark.xml
   sync_utils_bookmarks.html
   sync_utils_bookmarks.json
+  mirror_corrupt.sqlite
+  mirror_v1.sqlite
 
 [test_bookmark_corruption.js]
 [test_bookmark_deduping.js]
 [test_bookmark_deletion.js]
 [test_bookmark_explicit_weakupload.js]
 [test_bookmark_haschanges.js]
 [test_bookmark_kinds.js]
 [test_bookmark_mirror_meta.js]
+[test_bookmark_mirror_migration.js]
 [test_bookmark_structure_changes.js]
 [test_bookmark_validation.js]
 [test_bookmark_value_changes.js]
 [test_sync_utils.js]