Bug 1356812 - Use telemetry to report unfixable corrupt Places databases. r=past, bsmedberg draft
authorMarco Bonardo <mbonardo@mozilla.com>
Fri, 23 Jun 2017 18:15:46 +0200
changeset 604895 46c21041ca476bce08844d1772a9564045e1e495
parent 604013 0893f6685e154aaa3252ed091abb16a2feb2806d
child 636321 9be0c019b7581a71e41e256afc22b87bd6869d3f
push id67220
push usermak77@bonardo.net
push dateThu, 06 Jul 2017 17:00:55 +0000
reviewerspast, bsmedberg
bugs1356812
milestone56.0a1
Bug 1356812 - Use telemetry to report unfixable corrupt Places databases. r=past, bsmedberg MozReview-Commit-ID: EZKfMlnCKCf
toolkit/components/places/Database.cpp
toolkit/components/places/Database.h
toolkit/components/places/tests/unit/corruptDB.sqlite
toolkit/components/places/tests/unit/test_corrupt_telemetry.js
toolkit/components/places/tests/unit/xpcshell.ini
toolkit/components/telemetry/Histograms.json
--- 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",