Bug 1355414 - places.sqlite schema migration fails if an application has never used the bookmarks service. r=past draft
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 11 Apr 2017 16:04:23 +0200
changeset 562037 be183a06da6f4e051112aaa8dc9f0f5d31a083b2
parent 561911 819a666afddc804b6099ee1b3cff3a0fdf35ec15
child 624155 00dade0fc582d6e395a598f75518273abf578207
push id53938
push usermak77@bonardo.net
push dateThu, 13 Apr 2017 11:54:13 +0000
reviewerspast
bugs1355414
milestone55.0a1
Bug 1355414 - places.sqlite schema migration fails if an application has never used the bookmarks service. r=past MozReview-Commit-ID: 13YXf2On75J
toolkit/components/places/Database.cpp
toolkit/components/places/Database.h
toolkit/components/places/tests/migration/test_current_from_v34_no_roots.js
toolkit/components/places/tests/migration/xpcshell.ini
--- a/toolkit/components/places/Database.cpp
+++ b/toolkit/components/places/Database.cpp
@@ -777,38 +777,56 @@ Database::BackupAndReplaceDatabaseFile(n
   // file at every try to access any Places service.  That is bad because it
   // would quickly fill the user's disk space without any notice.
   if (!hasRecentCorruptDB()) {
     nsCOMPtr<nsIFile> backup;
     (void)aStorage->BackupDatabaseFile(databaseFile, DATABASE_CORRUPT_FILENAME,
                                        profDir, getter_AddRefs(backup));
   }
 
+  // If anything fails from this point on, we have a stale connection or
+  // database file, and there's not much more we can do.
+  // The only thing we can try to do is to replace the database on the next
+  // start, and enforce a crash, so it gets reported to us.
+
   // Close database connection if open.
   if (mMainConn) {
-    // If there's any not finalized statement or this fails for any reason
-    // we won't be able to remove the database.
     rv = mMainConn->Close();
-    NS_ENSURE_SUCCESS(rv, rv);
+    NS_ENSURE_SUCCESS(rv, ForceCrashAndReplaceDatabase(
+      NS_LITERAL_CSTRING("Unable to close the corrupt database.")));
   }
 
   // Remove the broken database.
   rv = databaseFile->Remove(false);
-  NS_ENSURE_SUCCESS(rv, rv);
+  if (NS_FAILED(rv) && rv != NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) {
+    return ForceCrashAndReplaceDatabase(
+      NS_LITERAL_CSTRING("Unable to remove the corrupt database file."));
+  }
 
   // Create a new database file.
   // Use an unshared connection, it will consume more memory but avoid shared
   // cache contentions across threads.
   rv = aStorage->OpenUnsharedDatabase(databaseFile, getter_AddRefs(mMainConn));
-  NS_ENSURE_SUCCESS(rv, rv);
+  NS_ENSURE_SUCCESS(rv, ForceCrashAndReplaceDatabase(
+    NS_LITERAL_CSTRING("Unable to open a new database connection.")));
 
   return NS_OK;
 }
 
 nsresult
+Database::ForceCrashAndReplaceDatabase(const nsCString& aReason)
+{
+  Preferences::SetBool(PREF_FORCE_DATABASE_REPLACEMENT, true);
+  // We could force an application restart here, but we'd like to get these
+  // cases reported to us, so let's force a crash instead.
+  MOZ_CRASH_UNSAFE_OOL(aReason.get());
+  return NS_ERROR_FAILURE;
+}
+
+nsresult
 Database::InitSchema(bool* aDatabaseMigrated)
 {
   MOZ_ASSERT(NS_IsMainThread());
   *aDatabaseMigrated = false;
 
   // WARNING: any statement executed before setting the journal mode must be
   // finalized, since SQLite doesn't allow changing the journal mode if there
   // is any outstanding statement.
@@ -2054,17 +2072,33 @@ Database::MigrateV34Up() {
   return NS_OK;
 }
 
 nsresult
 Database::MigrateV35Up() {
   MOZ_ASSERT(NS_IsMainThread());
 
   int64_t mobileRootId = CreateMobileRoot();
-  if (mobileRootId <= 0) return NS_ERROR_FAILURE;
+  if (mobileRootId <= 0)  {
+    // Either the schema is broken or there isn't any root. The latter can
+    // happen if a consumer, for example Thunderbird, never used bookmarks.
+    // If there are no roots, this migration should not run.
+    nsCOMPtr<mozIStorageStatement> checkRootsStmt;
+    nsresult rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
+      "SELECT id FROM moz_bookmarks WHERE parent = 0"
+    ), getter_AddRefs(checkRootsStmt));
+    NS_ENSURE_SUCCESS(rv, rv);
+    mozStorageStatementScoper scoper(checkRootsStmt);
+    bool hasResult = false;
+    rv = checkRootsStmt->ExecuteStep(&hasResult);
+    if (NS_SUCCEEDED(rv) && !hasResult) {
+      return NS_OK;
+    }
+    return NS_ERROR_FAILURE;
+  }
 
   // At this point, we should have no more than two folders with the mobile
   // bookmarks anno: the new root, and the old folder if one exists. If, for
   // some reason, we have multiple folders with the anno, we append their
   // children to the new root.
   nsTArray<int64_t> folderIds;
   nsresult rv = GetItemsWithAnno(NS_LITERAL_CSTRING(MOBILE_ROOT_ANNO),
                                  nsINavBookmarksService::TYPE_FOLDER,
--- a/toolkit/components/places/Database.h
+++ b/toolkit/components/places/Database.h
@@ -229,16 +229,22 @@ protected:
    * one.
    *
    * @param aStorage
    *        mozStorage service instance.
    */
   nsresult BackupAndReplaceDatabaseFile(nsCOMPtr<mozIStorageService>& aStorage);
 
   /**
+   * This should be used as a last resort in case the database is corrupt and
+   * there's no way to fix it in-place.
+   */
+  nsresult ForceCrashAndReplaceDatabase(const nsCString& aReason);
+
+  /**
    * Initializes the database.  This performs any necessary migrations for the
    * database.  All migration is done inside a transaction that is rolled back
    * if any error occurs.
    * @param aDatabaseMigrated
    *        Whether a schema upgrade happened.
    */
   nsresult InitSchema(bool* aDatabaseMigrated);
 
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/migration/test_current_from_v34_no_roots.js
@@ -0,0 +1,21 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+add_task(function* setup() {
+  yield setupPlacesDatabase("places_v34.sqlite");
+  // Setup database contents to be migrated.
+  let path = OS.Path.join(OS.Constants.Path.profileDir, DB_FILENAME);
+  let db = yield Sqlite.openConnection({ path });
+  // Remove all the roots.
+  yield db.execute("DELETE FROM moz_bookmarks");
+  yield db.close();
+});
+
+add_task(function* database_is_valid() {
+  // Accessing the database for the first time triggers migration.
+  Assert.equal(PlacesUtils.history.databaseStatus,
+               PlacesUtils.history.DATABASE_STATUS_UPGRADED);
+
+  let db = yield PlacesUtils.promiseDBConnection();
+  Assert.equal((yield db.getSchemaVersion()), CURRENT_SCHEMA_VERSION);
+});
--- a/toolkit/components/places/tests/migration/xpcshell.ini
+++ b/toolkit/components/places/tests/migration/xpcshell.ini
@@ -29,10 +29,11 @@ support-files =
 [test_current_from_v11.js]
 [test_current_from_v19.js]
 [test_current_from_v24.js]
 [test_current_from_v25.js]
 [test_current_from_v26.js]
 [test_current_from_v27.js]
 [test_current_from_v31.js]
 [test_current_from_v34.js]
+[test_current_from_v34_no_roots.js]
 [test_current_from_v35.js]
 [test_current_from_v36.js]