Bug 1355414 - places.sqlite schema migration fails if an application has never used the bookmarks service. r=past
MozReview-Commit-ID: 13YXf2On75J
--- 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]