Add IsCorrupt() to ProtoDatabase wrappers/clients.

Adds the ability to check for corruption via an IsCorrupt() function
that's pure virtual on the ProtoDatabase. IsCorrupt() is also introduced
in the ProtoLevelDBWrapper, and the corruption flag gets set on the Init
callback after checking status.

Unit tests changed slightly because the init callback needed to be put
into ProtoLevelDBWrapper instead of outside so it could modify
is_corrupt_, and the new use of the WeakPtr caused a race condition that
meant the test always failed to trigger the callback if we don't wait
for init to complete.

Bug: 870813
Change-Id: Ib00435dae8cdac490053fbfc6befb95094b1edec
Reviewed-on: https://chromium-review.googlesource.com/c/1355521
Commit-Queue: Troy Hildebrandt <thildebr@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612434}

components/leveldb_proto/proto_database.h

@@ -115,6 +115,8 @@ class ProtoDatabase {
   // Asynchronously destroys the database.
   virtual void Destroy(DestroyCallback callback) = 0;
 
+  virtual bool IsCorrupt() = 0;
+
  protected:
   ProtoDatabase() = default;
 };

components/leveldb_proto/proto_leveldb_wrapper.cc

@@ -10,11 +10,6 @@ namespace leveldb_proto {
 
 namespace {
 
-void RunInitCallback(typename ProtoLevelDBWrapper::InitCallback callback,
-                     const leveldb::Status* status) {
-  std::move(callback).Run(status->ok());
-}
-
 inline void InitFromTaskRunner(LevelDB* database,
                                const base::FilePath& database_dir,
                                const leveldb_env::Options& options,
@@ -66,19 +61,26 @@ inline void LoadKeysFromTaskRunner(LevelDB* database,
 
 ProtoLevelDBWrapper::ProtoLevelDBWrapper(
     const scoped_refptr<base::SequencedTaskRunner>& task_runner)
-    : task_runner_(task_runner) {
+    : task_runner_(task_runner), weak_ptr_factory_(this) {
   DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
 ProtoLevelDBWrapper::ProtoLevelDBWrapper(
     const scoped_refptr<base::SequencedTaskRunner>& task_runner,
     LevelDB* db)
-    : task_runner_(task_runner), db_(db) {
+    : task_runner_(task_runner), db_(db), weak_ptr_factory_(this) {
   DETACH_FROM_SEQUENCE(sequence_checker_);
 }
 
 ProtoLevelDBWrapper::~ProtoLevelDBWrapper() = default;
 
+void ProtoLevelDBWrapper::RunInitCallback(
+    typename ProtoLevelDBWrapper::InitCallback callback,
+    const leveldb::Status* status) {
+  is_corrupt_ = status->IsCorruption();
+  std::move(callback).Run(status->ok());
+}
+
 void ProtoLevelDBWrapper::InitWithDatabase(
     LevelDB* database,
     const base::FilePath& database_dir,
@@ -94,7 +96,8 @@ void ProtoLevelDBWrapper::InitWithDatabase(
       FROM_HERE,
       base::BindOnce(InitFromTaskRunner, base::Unretained(db_), database_dir,
                      options, destroy_on_corruption, status, metrics_id_),
-      base::BindOnce(RunInitCallback, std::move(callback),
+      base::BindOnce(&ProtoLevelDBWrapper::RunInitCallback,
+                     weak_ptr_factory_.GetWeakPtr(), std::move(callback),
                      base::Owned(status)));
 }
 
@@ -139,9 +142,16 @@ void ProtoLevelDBWrapper::SetMetricsId(const std::string& id) {
 }
 
 bool ProtoLevelDBWrapper::GetApproximateMemoryUse(uint64_t* approx_mem_use) {
+  if (db_ == nullptr)
+    return 0;
+
   return db_->GetApproximateMemoryUse(approx_mem_use);
 }
 
+bool ProtoLevelDBWrapper::IsCorrupt() {
+  return is_corrupt_;
+}
+
 const scoped_refptr<base::SequencedTaskRunner>&
 ProtoLevelDBWrapper::task_runner() {
   return task_runner_;

components/leveldb_proto/proto_leveldb_wrapper.h

@@ -133,6 +133,9 @@ class ProtoLevelDBWrapper {
 
   void Destroy(DestroyCallback callback);
 
+  void RunInitCallback(typename ProtoLevelDBWrapper::InitCallback callback,
+                       const leveldb::Status* status);
+
   // Allow callers to provide their own Database implementation.
   void InitWithDatabase(LevelDB* database,
                         const base::FilePath& database_dir,
@@ -142,6 +145,7 @@ class ProtoLevelDBWrapper {
 
   void SetMetricsId(const std::string& id);
 
+  bool IsCorrupt();
   bool GetApproximateMemoryUse(uint64_t* approx_mem_use);
 
   const scoped_refptr<base::SequencedTaskRunner>& task_runner();
@@ -149,6 +153,9 @@ class ProtoLevelDBWrapper {
  private:
   SEQUENCE_CHECKER(sequence_checker_);
 
+  // Set to true if the status from the previous Init call is corruption.
+  bool is_corrupt_ = false;
+
   // Used to run blocking tasks in-order, must be the TaskRunner that |db_|
   // relies on.
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
@@ -158,6 +165,8 @@ class ProtoLevelDBWrapper {
   // LevelDB calls, likely the database client name.
   std::string metrics_id_ = "Default";
 
+  base::WeakPtrFactory<ProtoLevelDBWrapper> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ProtoLevelDBWrapper);
 };
 

components/leveldb_proto/shared_proto_database.cc

@@ -84,6 +84,10 @@ void SharedProtoDatabase::OnDatabaseInit(
     bool success) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(on_task_runner_);
   init_state_ = success ? InitState::kSuccess : InitState::kFailure;
+
+  // TODO(thildebr): Check the db_wrapper_->IsCorrupt() and store corruption
+  // information to inform clients they may have lost data.
+
   callback_task_runner->PostTask(FROM_HERE,
                                  base::BindOnce(std::move(callback), success));
 }

components/leveldb_proto/shared_proto_database_client.h

@@ -41,14 +41,14 @@ class SharedProtoDatabaseClient : public ProtoDatabase<T> {
  public:
   virtual ~SharedProtoDatabaseClient();
 
-  virtual void Init(const std::string& client_name,
-                    typename ProtoDatabase<T>::InitCallback callback) override;
-
-  virtual void Init(const char* client_name,
-                    const base::FilePath& database_dir,
-                    const leveldb_env::Options& options,
-                    typename ProtoDatabase<T>::InitCallback callback) override;
-  virtual void InitWithDatabase(
+  void Init(const std::string& client_name,
+            typename ProtoDatabase<T>::InitCallback callback) override;
+
+  void Init(const char* client_name,
+            const base::FilePath& database_dir,
+            const leveldb_env::Options& options,
+            typename ProtoDatabase<T>::InitCallback callback) override;
+  void InitWithDatabase(
       LevelDB* database,
       const base::FilePath& database_dir,
       const leveldb_env::Options& options,
@@ -56,60 +56,58 @@ class SharedProtoDatabaseClient : public ProtoDatabase<T> {
 
   // Overrides for prepending namespace and type prefix to all operations on the
   // shared database.
-  virtual void UpdateEntries(
+  void UpdateEntries(
       std::unique_ptr<typename ProtoDatabase<T>::KeyEntryVector>
           entries_to_save,
       std::unique_ptr<std::vector<std::string>> keys_to_remove,
       typename ProtoDatabase<T>::UpdateCallback callback) override;
-  virtual void UpdateEntriesWithRemoveFilter(
+  void UpdateEntriesWithRemoveFilter(
       std::unique_ptr<typename ProtoDatabase<T>::KeyEntryVector>
           entries_to_save,
       const LevelDB::KeyFilter& delete_key_filter,
       typename ProtoDatabase<T>::UpdateCallback callback) override;
-  virtual void UpdateEntriesWithRemoveFilter(
+  void UpdateEntriesWithRemoveFilter(
       std::unique_ptr<typename ProtoDatabase<T>::KeyEntryVector>
           entries_to_save,
       const LevelDB::KeyFilter& delete_key_filter,
       const std::string& target_prefix,
       typename ProtoDatabase<T>::UpdateCallback callback) override;
 
-  virtual void LoadEntries(
-      typename ProtoDatabase<T>::LoadCallback callback) override;
-  virtual void LoadEntriesWithFilter(
+  void LoadEntries(typename ProtoDatabase<T>::LoadCallback callback) override;
+  void LoadEntriesWithFilter(
       const LevelDB::KeyFilter& filter,
       typename ProtoDatabase<T>::LoadCallback callback) override;
-  virtual void LoadEntriesWithFilter(
+  void LoadEntriesWithFilter(
       const LevelDB::KeyFilter& key_filter,
       const leveldb::ReadOptions& options,
       const std::string& target_prefix,
       typename ProtoDatabase<T>::LoadCallback callback) override;
 
-  virtual void LoadKeys(
-      typename ProtoDatabase<T>::LoadKeysCallback callback) override;
-  virtual void LoadKeys(
-      const std::string& target_prefix,
-      typename ProtoDatabase<T>::LoadKeysCallback callback) override;
+  void LoadKeys(typename ProtoDatabase<T>::LoadKeysCallback callback) override;
+  void LoadKeys(const std::string& target_prefix,
+                typename ProtoDatabase<T>::LoadKeysCallback callback) override;
 
-  virtual void LoadKeysAndEntries(
+  void LoadKeysAndEntries(
       typename ProtoDatabase<T>::LoadKeysAndEntriesCallback callback) override;
-  virtual void LoadKeysAndEntriesWithFilter(
+  void LoadKeysAndEntriesWithFilter(
       const LevelDB::KeyFilter& filter,
       typename ProtoDatabase<T>::LoadKeysAndEntriesCallback callback) override;
-  virtual void LoadKeysAndEntriesWithFilter(
+  void LoadKeysAndEntriesWithFilter(
       const LevelDB::KeyFilter& filter,
       const leveldb::ReadOptions& options,
       const std::string& target_prefix,
       typename ProtoDatabase<T>::LoadKeysAndEntriesCallback callback) override;
 
-  virtual void GetEntry(
-      const std::string& key,
-      typename ProtoDatabase<T>::GetCallback callback) override;
+  void GetEntry(const std::string& key,
+                typename ProtoDatabase<T>::GetCallback callback) override;
 
-  virtual void Destroy(
-      typename ProtoDatabase<T>::DestroyCallback callback) override;
+  void Destroy(typename ProtoDatabase<T>::DestroyCallback callback) override;
 
   typename ProtoLevelDBWrapper::InitCallback GetInitCallback() const;
 
+  bool IsCorrupt() override;
+  void SetIsCorrupt(bool is_corrupt);
+
  private:
   friend class SharedProtoDatabase;
   friend class SharedProtoDatabaseTest;
@@ -142,6 +140,10 @@ class SharedProtoDatabaseClient : public ProtoDatabase<T> {
 
   SEQUENCE_CHECKER(sequence_checker_);
 
+  // |is_corrupt_| should be set by the SharedProtoDatabase that creates this
+  // when a client is created that doesn't know about a previous shared
+  // database corruption.
+  bool is_corrupt_ = false;
   std::string prefix_;
 
   scoped_refptr<SharedProtoDatabase> parent_db_;
@@ -326,6 +328,16 @@ void SharedProtoDatabaseClient<T>::Destroy(
                      std::move(callback)));
 }
 
+template <typename T>
+void SharedProtoDatabaseClient<T>::SetIsCorrupt(bool is_corrupt) {
+  is_corrupt_ = is_corrupt;
+}
+
+template <typename T>
+bool SharedProtoDatabaseClient<T>::IsCorrupt() {
+  return is_corrupt_;
+}
+
 // static
 template <typename T>
 void SharedProtoDatabaseClient<T>::StripPrefixLoadKeysCallback(

components/leveldb_proto/unique_proto_database.h

@@ -96,6 +96,8 @@ class UniqueProtoDatabase : public ProtoDatabase<T> {
 
   bool GetApproximateMemoryUse(uint64_t* approx_mem_use);
 
+  bool IsCorrupt() override;
+
   // Sets the identifier used by the underlying LevelDB wrapper to record
   // metrics.
   void SetMetricsId(const std::string& id);
@@ -288,6 +290,11 @@ void UniqueProtoDatabase<T>::SetMetricsId(const std::string& id) {
   db_wrapper_->SetMetricsId(id);
 }
 
+template <typename T>
+bool UniqueProtoDatabase<T>::IsCorrupt() {
+  return db_wrapper_->IsCorrupt();
+}
+
 }  // namespace leveldb_proto
 
 #endif  // COMPONENTS_LEVELDB_PROTO_UNIQUE_PROTO_DATABASE_H_

components/leveldb_proto/unique_proto_database_unittest.cc

@@ -660,9 +660,16 @@ TEST(UniqueProtoDatabaseThreadingTest, TestDBDestruction) {
 
   MockDatabaseCaller caller;
   EXPECT_CALL(caller, InitCallback(_));
+  base::RunLoop init_loop;
   db->Init(kTestLevelDBClientName, temp_dir.GetPath(), CreateSimpleOptions(),
-           base::BindOnce(&MockDatabaseCaller::InitCallback,
-                          base::Unretained(&caller)));
+           base::BindOnce(
+               [](MockDatabaseCaller* caller, base::OnceClosure closure,
+                  bool success) {
+                 caller->InitCallback(success);
+                 std::move(closure).Run();
+               },
+               &caller, init_loop.QuitClosure()));
+  init_loop.Run();
 
   db.reset();
 
@@ -688,9 +695,16 @@ TEST(UniqueProtoDatabaseThreadingTest, TestDBDestroy) {
 
   MockDatabaseCaller caller;
   EXPECT_CALL(caller, InitCallback(_));
+  base::RunLoop init_loop;
   db->Init(kTestLevelDBClientName, temp_dir.GetPath(), CreateSimpleOptions(),
-           base::BindOnce(&MockDatabaseCaller::InitCallback,
-                          base::Unretained(&caller)));
+           base::BindOnce(
+               [](MockDatabaseCaller* caller, base::OnceClosure closure,
+                  bool success) {
+                 caller->InitCallback(success);
+                 std::move(closure).Run();
+               },
+               &caller, init_loop.QuitClosure()));
+  init_loop.Run();
 
   EXPECT_CALL(caller, DestroyCallback(_));
   db->Destroy(base::BindOnce(&MockDatabaseCaller::DestroyCallback,