RC: LocalDB: Attempt DB recoveries when corrupt + metrics.

Bug: 773382
Change-Id: Ibaf68d29185611ffbd3563330ab7c3d23ca4f7e8
Reviewed-on: https://chromium-review.googlesource.com/1106053
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569732}

chrome/browser/resource_coordinator/leveldb_site_characteristics_database.cc

@@ -6,8 +6,11 @@
 
 #include <string>
 
+#include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/metrics/histogram_functions.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/task_runner_util.h"
 #include "base/threading/thread_restrictions.h"
 #include "third_party/leveldatabase/env_chromium.h"
@@ -15,6 +18,74 @@
 
 namespace resource_coordinator {
 
+namespace {
+
+const char kInitStatusHistogramLabel[] =
+    "TabManager.Discarding.LocalDatabase.DatabaseInit";
+const char kInitStatusAfterRepairHistogramLabel[] =
+    "TabManager.Discarding.LocalDatabase.DatabaseInitAfterRepair";
+const char kInitStatusAfterDeleteHistogramLabel[] =
+    "TabManager.Discarding.LocalDatabase.DatabaseInitAfterDelete";
+
+enum class InitStatus {
+  kInitStatusOk,
+  kInitStatusCorruption,
+  kInitStatusIOError,
+  kInitStatusUnknownError,
+  kInitStatusMax
+};
+
+// Report the database's initialization status metrics.
+void ReportInitStatus(const char* histogram_name,
+                      const leveldb::Status& status) {
+  if (status.ok()) {
+    base::UmaHistogramEnumeration(histogram_name, InitStatus::kInitStatusOk,
+                                  InitStatus::kInitStatusMax);
+  } else if (status.IsCorruption()) {
+    base::UmaHistogramEnumeration(histogram_name,
+                                  InitStatus::kInitStatusCorruption,
+                                  InitStatus::kInitStatusMax);
+  } else if (status.IsIOError()) {
+    base::UmaHistogramEnumeration(histogram_name,
+                                  InitStatus::kInitStatusIOError,
+                                  InitStatus::kInitStatusMax);
+  } else {
+    base::UmaHistogramEnumeration(histogram_name,
+                                  InitStatus::kInitStatusUnknownError,
+                                  InitStatus::kInitStatusMax);
+  }
+}
+
+void ReportRepairResult(bool repair_succeeded) {
+  UMA_HISTOGRAM_BOOLEAN("TabManager.Discarding.LocalDatabase.DatabaseRepair",
+                        repair_succeeded);
+}
+
+// Attempt to repair the database stored in |db_path|.
+bool RepairDatabase(const std::string& db_path) {
+  leveldb_env::Options options;
+  options.reuse_logs = false;
+  options.max_open_files = 0;
+  if (!leveldb::RepairDB(db_path, options).ok())
+    return false;
+  return true;
+}
+
+bool ShouldAttemptDbRepair(const leveldb::Status& status) {
+  // A corrupt database might be repaired (some data might be loss but it's
+  // better than losing everything).
+  if (status.IsCorruption())
+    return true;
+  // An I/O error might be caused by a missing manifest, it's sometime possible
+  // to repair this (some data might be loss).
+  if (status.IsIOError())
+    return true;
+
+  return false;
+}
+
+}  // namespace
+
 // Helper class used to run all the blocking operations posted by
 // LocalSiteCharacteristicDatabase on a TaskScheduler sequence with the
 // |MayBlock()| trait.
@@ -47,6 +118,8 @@ class LevelDBSiteCharacteristicsDatabase::AsyncHelper {
       const std::vector<url::Origin>& site_origin);
   void ClearDatabase();
 
+  bool DBIsInitialized() { return db_ != nullptr; }
+
  private:
   static const std::string& SerializeOrigin(const url::Origin& origin) {
     return origin.host();
@@ -73,12 +146,34 @@ void LevelDBSiteCharacteristicsDatabase::AsyncHelper::OpenOrCreateDatabase() {
   options.create_if_missing = true;
   leveldb::Status status =
       leveldb_env::OpenDB(options, db_path_.AsUTF8Unsafe(), &db_);
-  // TODO(sebmarchand): Do more validation here, try to repair the database if
-  // it's corrupt and report some metrics.
-  if (!status.ok()) {
-    LOG(ERROR) << "Unable to open the Site Characteristics database: "
-               << status.ToString();
-    db_.reset();
+
+  ReportInitStatus(kInitStatusHistogramLabel, status);
+
+  if (status.ok())
+    return;
+
+  db_.reset();
+
+  if (!ShouldAttemptDbRepair(status))
+    return;
+
+  if (RepairDatabase(db_path_.AsUTF8Unsafe())) {
+    ReportRepairResult(true);
+    status = leveldb_env::OpenDB(options, db_path_.AsUTF8Unsafe(), &db_);
+    ReportInitStatus(kInitStatusAfterRepairHistogramLabel, status);
+    if (status.ok())
+      return;
+  } else {
+    ReportRepairResult(false);
+  }
+
+  db_.reset();
+  // Delete the database and try to open it one last time.
+  if (base::DeleteFile(db_path_, true /* recursive */)) {
+    status = leveldb_env::OpenDB(options, db_path_.AsUTF8Unsafe(), &db_);
+    ReportInitStatus(kInitStatusAfterDeleteHistogramLabel, status);
+    if (!status.ok())
+      db_.reset();
   }
 }
 
@@ -212,4 +307,8 @@ void LevelDBSiteCharacteristicsDatabase::ClearDatabase() {
           base::Unretained(async_helper_.get())));
 }
 
+bool LevelDBSiteCharacteristicsDatabase::DatabaseIsInitializedForTesting() {
+  return async_helper_->DBIsInitialized();
+}
+
 }  // namespace resource_coordinator

chrome/browser/resource_coordinator/leveldb_site_characteristics_database.h

@@ -43,6 +43,8 @@ class LevelDBSiteCharacteristicsDatabase
       const std::vector<url::Origin>& site_origins) override;
   void ClearDatabase() override;
 
+  bool DatabaseIsInitializedForTesting();
+
  private:
   class AsyncHelper;
 

chrome/browser/resource_coordinator/leveldb_site_characteristics_database_unittest.cc

@@ -8,9 +8,11 @@
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/metrics/histogram_tester.h"
 #include "base/test/scoped_task_environment.h"
 #include "chrome/browser/resource_coordinator/site_characteristics.pb.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/leveldatabase/leveldb_chrome.h"
 #include "url/gurl.h"
 
 namespace resource_coordinator {
@@ -41,10 +43,7 @@ class LevelDBSiteCharacteristicsDatabaseTest : public ::testing::Test {
 
   void SetUp() override {
     EXPECT_TRUE(temp_dir_.CreateUniqueTempDir());
-    db_ = std::make_unique<LevelDBSiteCharacteristicsDatabase>(
-        temp_dir_.GetPath());
-    WaitForAsyncOperationsToComplete();
-    EXPECT_TRUE(db_);
+    OpenDB();
   }
 
   void TearDown() override {
@@ -53,6 +52,13 @@ class LevelDBSiteCharacteristicsDatabaseTest : public ::testing::Test {
     EXPECT_TRUE(temp_dir_.Delete());
   }
 
+  void OpenDB() {
+    db_ = std::make_unique<LevelDBSiteCharacteristicsDatabase>(
+        temp_dir_.GetPath());
+    WaitForAsyncOperationsToComplete();
+    EXPECT_TRUE(db_);
+  }
+
  protected:
   // Try to read an entry from the database, returns true if the entry is
   // present and false otherwise. |receiving_proto| will receive the protobuf
@@ -74,6 +80,24 @@ class LevelDBSiteCharacteristicsDatabaseTest : public ::testing::Test {
     return success;
   }
 
+  // Add some entries to the database and returns a vector with their origins.
+  std::vector<url::Origin> AddDummyEntriesToDB() {
+    const size_t kEntryCount = 10;
+    std::vector<url::Origin> site_origins;
+    for (size_t i = 0; i < kEntryCount; ++i) {
+      SiteCharacteristicsProto proto_temp;
+      std::string origin_str = base::StringPrintf("http://%zu.com", i);
+      InitSiteCharacteristicProto(&proto_temp,
+                                  static_cast<::google::protobuf::int64>(i));
+      EXPECT_TRUE(proto_temp.IsInitialized());
+      url::Origin origin = url::Origin::Create(GURL(origin_str));
+      db_->WriteSiteCharacteristicsIntoDB(origin, proto_temp);
+      site_origins.emplace_back(origin);
+    }
+    WaitForAsyncOperationsToComplete();
+    return site_origins;
+  }
+
   void WaitForAsyncOperationsToComplete() { task_env_.RunUntilIdle(); }
 
   const url::Origin kDummyOrigin = url::Origin::Create(GURL("http://foo.com"));
@@ -100,25 +124,11 @@ TEST_F(LevelDBSiteCharacteristicsDatabaseTest, InitAndStoreSiteCharacteristic) {
 }
 
 TEST_F(LevelDBSiteCharacteristicsDatabaseTest, RemoveEntries) {
-  // Add multiple origins to the database.
-  const size_t kEntryCount = 10;
-  std::vector<url::Origin> site_origins;
-  for (size_t i = 0; i < kEntryCount; ++i) {
-    SiteCharacteristicsProto proto_temp;
-    std::string origin_str = base::StringPrintf("http://%zu.com", i);
-    InitSiteCharacteristicProto(&proto_temp,
-                                static_cast<::google::protobuf::int64>(i));
-    EXPECT_TRUE(proto_temp.IsInitialized());
-    url::Origin origin = url::Origin::Create(GURL(origin_str));
-    db_->WriteSiteCharacteristicsIntoDB(origin, proto_temp);
-    site_origins.emplace_back(origin);
-  }
-
-  WaitForAsyncOperationsToComplete();
+  std::vector<url::Origin> site_origins = AddDummyEntriesToDB();
 
   // Remove half the origins from the database.
   std::vector<url::Origin> site_origins_to_remove(
-      site_origins.begin(), site_origins.begin() + kEntryCount / 2);
+      site_origins.begin(), site_origins.begin() + site_origins.size() / 2);
   db_->RemoveSiteCharacteristicsFromDB(site_origins_to_remove);
 
   WaitForAsyncOperationsToComplete();
@@ -128,7 +138,7 @@ TEST_F(LevelDBSiteCharacteristicsDatabaseTest, RemoveEntries) {
   for (const auto& iter : site_origins_to_remove)
     EXPECT_FALSE(ReadFromDB(iter, &proto_temp));
 
-  for (auto iter = site_origins.begin() + kEntryCount / 2;
+  for (auto iter = site_origins.begin() + site_origins.size() / 2;
        iter != site_origins.end(); ++iter) {
     EXPECT_TRUE(ReadFromDB(*iter, &proto_temp));
   }
@@ -143,4 +153,29 @@ TEST_F(LevelDBSiteCharacteristicsDatabaseTest, RemoveEntries) {
     EXPECT_FALSE(ReadFromDB(iter, &proto_temp));
 }
 
+TEST_F(LevelDBSiteCharacteristicsDatabaseTest, DatabaseRecoveryTest) {
+  std::vector<url::Origin> site_origins = AddDummyEntriesToDB();
+
+  db_.reset();
+
+  EXPECT_TRUE(leveldb_chrome::CorruptClosedDBForTesting(temp_dir_.GetPath()));
+
+  base::HistogramTester histogram_tester;
+  histogram_tester.ExpectTotalCount(
+      "TabManager.Discarding.LocalDatabase.DatabaseInit", 0);
+  // Open the corrupt DB and ensure that the appropriate histograms gets
+  // updated.
+  OpenDB();
+  EXPECT_TRUE(db_->DatabaseIsInitializedForTesting());
+  histogram_tester.ExpectUniqueSample(
+      "TabManager.Discarding.LocalDatabase.DatabaseInit",
+      1 /* kInitStatusCorruption */, 1);
+  histogram_tester.ExpectUniqueSample(
+      "TabManager.Discarding.LocalDatabase.DatabaseInitAfterRepair",
+      0 /* kInitStatusOk */, 1);
+
+  // TODO(sebmarchand): try to induce an I/O error by deleting one of the
+  // manifest files.
+}
+
 }  // namespace resource_coordinator

tools/metrics/histograms/enums.xml

@@ -26722,6 +26722,13 @@ Called by update_gpu_driver_bug_workaround_entries.py.-->
   <int value="1" label="SinkNeverStarted"/>
 </enum>
 
+<enum name="LocalSiteCharacteristicsDBInitStatus">
+  <int value="0" label="Success"/>
+  <int value="1" label="Corruption"/>
+  <int value="2" label="IO Error"/>
+  <int value="3" label="Unknown Error"/>
+</enum>
+
 <enum name="LocalStorageOpenError">
   <int value="0" label="Directory open failed"/>
   <int value="1" label="Database open failed"/>

tools/metrics/histograms/histograms.xml

@@ -99777,6 +99777,41 @@ uploading your change for review.
   </summary>
 </histogram>
 
+<histogram name="TabManager.Discarding.LocalDatabase.DatabaseInit"
+    enum="LocalSiteCharacteristicsDBInitStatus">
+  <owner>sebmarchand@chromium.org</owner>
+  <summary>
+    The result of opening the Local Site Characteristics database.
+  </summary>
+</histogram>
+
+<histogram name="TabManager.Discarding.LocalDatabase.DatabaseInitAfterDelete"
+    enum="LocalSiteCharacteristicsDBInitStatus">
+  <owner>sebmarchand@chromium.org</owner>
+  <summary>
+    The result of opening the Local Site Characteristics database after deleting
+    it after a failed repair attempt.
+  </summary>
+</histogram>
+
+<histogram name="TabManager.Discarding.LocalDatabase.DatabaseInitAfterRepair"
+    enum="LocalSiteCharacteristicsDBInitStatus">
+  <owner>sebmarchand@chromium.org</owner>
+  <summary>
+    The result of opening the Local Site Characteristics database after a repair
+    attempt.
+  </summary>
+</histogram>
+
+<histogram name="TabManager.Discarding.LocalDatabase.DatabaseRepair"
+    enum="BooleanSuccess">
+  <owner>sebmarchand@chromium.org</owner>
+  <summary>
+    The result of trying to repair the Local Site Characteristics database after
+    a failed open.
+  </summary>
+</histogram>
+
 <histogram name="TabManager.Discarding.LogMemoryTime" units="ms">
   <owner>cywang@chromium.org</owner>
   <owner>georgesak@chromium.org</owner>