Bug 1356812 - Use telemetry to report unfixable corrupt Places databases. r=past, bsmedberg
MozReview-Commit-ID: EZKfMlnCKCf
--- a/toolkit/components/places/Database.cpp
+++ b/toolkit/components/places/Database.cpp
@@ -775,57 +775,66 @@ Database::BackupAndReplaceDatabaseFile(n
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) {
- rv = mMainConn->SpinningSynchronousClose();
- NS_ENSURE_SUCCESS(rv, ForceCrashAndReplaceDatabase(
- NS_LITERAL_CSTRING("Unable to close the corrupt database.")));
+ // startup, and report the problem through telemetry.
+ {
+ enum eCorruptDBReplaceStage : int8_t {
+ stage_closing = 0,
+ stage_removing,
+ stage_reopening,
+ stage_replaced
+ };
+ eCorruptDBReplaceStage stage = stage_closing;
+ auto guard = MakeScopeExit([&]() {
+ if (stage != stage_replaced) {
+ // Reaching this point means the database is corrupt and we failed to
+ // replace it. For this session part of the application related to
+ // bookmarks and history will misbehave. The frontend may show a
+ // "locked" notification to the user though.
+ // Set up a pref to try replacing the database at the next startup.
+ Preferences::SetBool(PREF_FORCE_DATABASE_REPLACEMENT, true);
+ }
+ // Report the corruption through telemetry.
+ Telemetry::Accumulate(Telemetry::PLACES_DATABASE_CORRUPTION_HANDLING_STAGE,
+ static_cast<int8_t>(stage));
+ });
+
+ // Close database connection if open.
+ if (mMainConn) {
+ rv = mMainConn->SpinningSynchronousClose();
+ NS_ENSURE_SUCCESS(rv, rv);
+ }
+
+ // Remove the broken database.
+ stage = stage_removing;
+ rv = databaseFile->Remove(false);
+ if (NS_FAILED(rv) && rv != NS_ERROR_FILE_TARGET_DOES_NOT_EXIST) {
+ return rv;
+ }
+
+ // Create a new database file.
+ // Use an unshared connection, it will consume more memory but avoid shared
+ // cache contentions across threads.
+ stage = stage_reopening;
+ rv = aStorage->OpenUnsharedDatabase(databaseFile, getter_AddRefs(mMainConn));
+ NS_ENSURE_SUCCESS(rv, rv);
+
+ stage = stage_replaced;
}
- // Remove the broken database.
- rv = databaseFile->Remove(false);
- 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, 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);
- // Ensure that prefs get saved, or we could crash before storing them.
- nsIPrefService* prefService = Preferences::GetService();
- if (prefService && NS_SUCCEEDED(static_cast<Preferences *>(prefService)->SavePrefFileBlocking())) {
- // 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::SetupDatabaseConnection(nsCOMPtr<mozIStorageService>& aStorage)
{
MOZ_ASSERT(NS_IsMainThread());
// 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.
--- a/toolkit/components/places/Database.h
+++ b/toolkit/components/places/Database.h
@@ -230,22 +230,16 @@ 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);
-
- /**
* Set up the connection environment through PRAGMAs.
* Will return NS_ERROR_FILE_CORRUPTED if any critical setting fails.
*
* @param aStorage
* mozStorage service instance.
*/
nsresult SetupDatabaseConnection(nsCOMPtr<mozIStorageService>& aStorage);
new file mode 100644
index 0000000000000000000000000000000000000000..b234246cac90b17d43d3955156d94276b7e3b503
GIT binary patch
literal 32772
zc%1Fm+i%)d90zdYq?C4JZF<Y@J1<k0312Y85TvRl#fh?#EMtz=R*|X``v8_Ow(Nr-
z+g_qc`$x97N&92^x~D!(+CQ+@{RlX1wkfTmX;Q22NBKg|g>x}F2EP8P9Z12tQ4(^=
zDx7m=QOV_$dyFXxvy_=d$|scK{QpdjQczw&aXyN=_bR>N4Uqd~`S$JHk5?o=7>F?X
z_K6>jhl7aw>8XNxzvDDrhq=x5w!=;}vCjgZZSA<urqf}&ovrO==Mj70JSwpv4+V3b
zhi-|b(J1k<g5*h0$gG7*(haERe2fwkL;rLI^^q?W6VVk(B9i$CVkiT7xYf!=7h$~1
zWq)4uMoF@MC`Ed*#+@%+wzJo6vzD{b+-tkcEU}PJUeZY`Q!iSTdl+Z?i02FOn{hk{
zJT8N1_>U&<xc02M`A9|`k&d44_+PrMOwXp8`(9C3=mE~*k8{8B==1!Nvf|y%=P#wj
zqN)~)g#w*{U@+iX+1BG>uP<esHVorn)H|-$=goSQ^o-M8yTL$EPh+F;w+3bL-qY`X
zd;0kJ`PmXXbIi0Kjh|49?)8GZfq$oJ+SRqH+EQCTxEF3HTSsA<&*zSokBZ*H!kz6x
z;hSJcM>c)9t36m>vrN0bs!i&)=Ire0Vj`w<(Nx)?*2BO{qBQDC-HXDLYQr`v1~+W_
z0IJn}Pb+unp~^)?<NkrInO3d4+U@#fuWT7-&zD^Ur(v|R8_7#vLYIaA^k@0^>GRK<
zSAV@YU-Ni8)~72S^F+j4di~=ir-BYX-4={VJf01!jp9_qrmba7dS8ZvPc5rftyF5f
z>iO2%s%MuQb+b~VYNK2+t$Nio-2SM<EQ@_H8Zyf?ZD!URW~E`)+2*$EH1<z6uz&Uj
zu0N~OAAf$YFd|P1siiUzJiOo7-%X-k!b7$}7bo4{p1s>tqI*d+`40~Q0KA?)rhG)X
zLb*<PoAQn_`|5s;@)qSX<pSjr<*G9K?thVTgL0E{i?U34m+~IveaZ)v4=GC&rp$gv
zOy|l|7AWffur~kz00000000000000000000000000000000000000000000000000
W000000000000000004k<yZje$IAS{h
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/unit/test_corrupt_telemetry.js
@@ -0,0 +1,31 @@
+/* Any copyright is dedicated to the Public Domain.
+ http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Tests that history initialization correctly handles a request to forcibly
+// replace the current database.
+
+add_task(async function() {
+ let profileDBPath = await OS.Path.join(OS.Constants.Path.profileDir, "places.sqlite");
+ await OS.File.remove(profileDBPath, {ignoreAbsent: true});
+ // Ensure that our database doesn't already exist.
+ Assert.ok(!(await OS.File.exists(profileDBPath)), "places.sqlite shouldn't exist");
+ let dir = await OS.File.getCurrentDirectory();
+ let src = OS.Path.join(dir, "corruptDB.sqlite");
+ await OS.File.copy(src, profileDBPath);
+ Assert.ok(await OS.File.exists(profileDBPath), "places.sqlite should exist");
+
+ let count = Services.telemetry
+ .getHistogramById("PLACES_DATABASE_CORRUPTION_HANDLING_STAGE")
+ .snapshot()
+ .counts[3];
+ Assert.equal(count, 0, "There should be no telemetry");
+
+ do_check_eq(PlacesUtils.history.databaseStatus,
+ PlacesUtils.history.DATABASE_STATUS_CORRUPT);
+
+ count = Services.telemetry
+ .getHistogramById("PLACES_DATABASE_CORRUPTION_HANDLING_STAGE")
+ .snapshot()
+ .counts[3];
+ Assert.equal(count, 1, "Telemetry should have been added");
+});
--- a/toolkit/components/places/tests/unit/xpcshell.ini
+++ b/toolkit/components/places/tests/unit/xpcshell.ini
@@ -3,16 +3,17 @@ head = head_bookmarks.js
firefox-appdir = browser
skip-if = toolkit == 'android'
support-files =
bookmarks.corrupt.html
bookmarks.json
bookmarks.preplaces.html
bookmarks_html_singleframe.html
bug476292.sqlite
+ corruptDB.sqlite
default.sqlite
livemark.xml
mobile_bookmarks_folder_import.json
mobile_bookmarks_folder_merge.json
mobile_bookmarks_multiple_folders.json
mobile_bookmarks_root_import.json
mobile_bookmarks_root_merge.json
nsDummyObserver.js
@@ -69,16 +70,17 @@ skip-if = (os == "win" && os_version ==
[test_bookmarks_html_import_tags.js]
[test_bookmarks_html_singleframe.js]
[test_bookmarks_restore_notification.js]
[test_bookmarks_setNullTitle.js]
[test_broken_folderShortcut_result.js]
[test_browserhistory.js]
[test_bug636917_isLivemark.js]
[test_childlessTags.js]
+[test_corrupt_telemetry.js]
[test_crash_476292.js]
[test_database_replaceOnStartup.js]
[test_download_history.js]
[test_frecency.js]
[test_frecency_decay.js]
[test_frecency_zero_updated.js]
[test_getChildIndex.js]
[test_history.js]
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -5006,16 +5006,25 @@
"CSP_REFERRER_DIRECTIVE": {
"record_in_processes": ["main", "content"],
"alert_emails": ["seceng-telemetry@mozilla.com"],
"bug_numbers": [1303685],
"expires_in_version": "60",
"kind": "boolean",
"description": "Whether a document with a CSP policy (report-only or enforcing) contains a referrer directive ('true') or not ('false')."
},
+ "PLACES_DATABASE_CORRUPTION_HANDLING_STAGE": {
+ "record_in_processes": ["main"],
+ "alert_emails": ["firefox-dev@mozilla.org"],
+ "expires_in_version": "never",
+ "kind": "enumerated",
+ "n_values": 6,
+ "releaseChannelCollection": "opt-out",
+ "description": "PLACES: stage reached when trying to fix a database corruption , see Places::Database::eCorruptDBReplaceStatus"
+ },
"PLACES_PAGES_COUNT": {
"record_in_processes": ["main", "content"],
"expires_in_version": "never",
"kind": "exponential",
"low": 1000,
"high": 150000,
"n_buckets": 20,
"releaseChannelCollection": "opt-out",