[PM] Fix some sitedata leveldb sequence violations

Bug: 1159407
Change-Id: Ibbc21edbc342c5850a3f20679a41a18722850156
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2595452Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840205}

components/performance_manager/persistence/site_data/leveldb_site_data_store.cc

@@ -10,12 +10,14 @@
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/callback_forward.h"
 #include "base/files/file_util.h"
 #include "base/hash/md5.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/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/task/thread_pool.h"
 #include "base/task_runner_util.h"
@@ -159,14 +161,12 @@ class LevelDBSiteDataStore::AsyncHelper {
   DatabaseSizeResult GetDatabaseSize();
 
   bool DBIsInitialized() {
-    // TODO(https://crbug.com/1159407):
-    // DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     return db_ != nullptr;
   }
 
   leveldb::DB* GetDBForTesting() {
-    // TODO(https://crbug.com/1159407):
-    // DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     DCHECK(DBIsInitialized());
     return db_.get();
   }
@@ -197,8 +197,7 @@ class LevelDBSiteDataStore::AsyncHelper {
   // The on disk location of the database.
   const base::FilePath db_path_ GUARDED_BY_CONTEXT(sequence_checker_);
   // The connection to the LevelDB database.
-  // TODO(https://crbug.com/1159407): GUARDED_BY_CONTEXT(sequence_checker_)
-  std::unique_ptr<leveldb::DB> db_;
+  std::unique_ptr<leveldb::DB> db_ GUARDED_BY_CONTEXT(sequence_checker_);
   // The options to be used for all database read operations.
   leveldb::ReadOptions read_options_ GUARDED_BY_CONTEXT(sequence_checker_);
   // The options to be used for all database write operations.
@@ -517,16 +516,29 @@ void LevelDBSiteDataStore::SetInitializationCallbackForTesting(
                                 std::move(callback)));
 }
 
-bool LevelDBSiteDataStore::DatabaseIsInitializedForTesting() {
-  // TODO(https://crbug.com/1159407):
-  // DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return async_helper_->DBIsInitialized();
+void LevelDBSiteDataStore::DatabaseIsInitializedForTesting(
+    base::OnceCallback<void(bool)> reply_cb) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  blocking_task_runner_->PostTaskAndReplyWithResult(
+      FROM_HERE,
+      base::BindOnce(&LevelDBSiteDataStore::AsyncHelper::DBIsInitialized,
+                     base::Unretained(async_helper_.get())),
+      std::move(reply_cb));
 }
 
-leveldb::DB* LevelDBSiteDataStore::GetDBForTesting() {
-  // TODO(https://crbug.com/1159407):
-  // DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  return async_helper_->GetDBForTesting();
+void LevelDBSiteDataStore::RunTaskWithRawDBForTesting(
+    base::OnceCallback<void(leveldb::DB*)> task,
+    base::OnceClosure after_task_run_closure) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  auto task_on_blocking_task_runner = base::BindOnce(
+      [](LevelDBSiteDataStore::AsyncHelper* helper,
+         base::OnceCallback<void(leveldb::DB*)> task) {
+        std::move(task).Run(helper->GetDBForTesting());  // IN-TEST
+      },
+      base::Unretained(async_helper_.get()), std::move(task));
+  blocking_task_runner_->PostTaskAndReply(
+      FROM_HERE, std::move(task_on_blocking_task_runner),
+      std::move(after_task_run_closure));
 }
 
 // static

components/performance_manager/persistence/site_data/leveldb_site_data_store.h

@@ -6,6 +6,7 @@
 #define COMPONENTS_PERFORMANCE_MANAGER_PERSISTENCE_SITE_DATA_LEVELDB_SITE_DATA_STORE_H_
 
 #include "base/auto_reset.h"
+#include "base/callback_forward.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/sequence_checker.h"
@@ -44,14 +45,11 @@ class LevelDBSiteDataStore : public SiteDataStore {
   void GetStoreSize(GetStoreSizeCallback callback) override;
   void SetInitializationCallbackForTesting(base::OnceClosure callback) override;
 
-  bool DatabaseIsInitializedForTesting();
+  void DatabaseIsInitializedForTesting(base::OnceCallback<void(bool)> reply_cb);
 
-  // Returns a raw pointer to the database for testing purposes. Note that as
-  // the DB operations are made on a separate sequence it's recommended to call
-  // TaskEnvironment::RunUntilIdle before calling this function to ensure
-  // that the database has been fully initialized. The LevelDB implementation is
-  // thread safe.
-  leveldb::DB* GetDBForTesting();
+  // Run a task against the raw DB implementation and wait for it to complete.
+  void RunTaskWithRawDBForTesting(base::OnceCallback<void(leveldb::DB*)> task,
+                                  base::OnceClosure after_task_run_closure);
 
   // Make the new instances of this class use an in memory database rather than
   // creating it on disk.
@@ -70,8 +68,8 @@ class LevelDBSiteDataStore : public SiteDataStore {
   // to run on |blocking_task_runner_|, it is guaranteed that the AsyncHelper
   // held by this object will only be destructed once all the tasks that have
   // been posted to it have completed.
-  // TODO(https://crbug.com/1159407): GUARDED_BY_CONTEXT(sequence_checker_)
-  std::unique_ptr<AsyncHelper, base::OnTaskRunnerDeleter> async_helper_;
+  std::unique_ptr<AsyncHelper, base::OnTaskRunnerDeleter> async_helper_
+      GUARDED_BY_CONTEXT(sequence_checker_);
 
   SEQUENCE_CHECKER(sequence_checker_);
 

components/performance_manager/persistence/site_data/leveldb_site_data_store_unittest.cc

@@ -7,8 +7,10 @@
 #include <limits>
 
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "base/test/bind.h"
@@ -95,6 +97,21 @@ class LevelDBSiteDataStoreTest : public ::testing::Test {
     db_path_ = path;
   }
 
+  bool DbIsInitialized() {
+    base::RunLoop run_loop;
+    bool ret = false;
+    auto cb = base::BindOnce(
+        [](bool* ret, base::OnceClosure quit_closure, bool db_is_initialized) {
+          *ret = db_is_initialized;
+          std::move(quit_closure).Run();
+        },
+        base::Unretained(&ret), run_loop.QuitClosure());
+    db_->DatabaseIsInitializedForTesting(std::move(cb));
+    run_loop.Run();
+
+    return ret;
+  }
+
   const base::FilePath& GetTempPath() { return temp_dir_.GetPath(); }
   const base::FilePath& GetDBPath() { return db_path_; }
 
@@ -228,7 +245,7 @@ TEST_F(LevelDBSiteDataStoreTest, DatabaseRecoveryTest) {
   // Open the corrupt DB and ensure that the appropriate histograms gets
   // updated.
   OpenDB();
-  EXPECT_TRUE(db_->DatabaseIsInitializedForTesting());
+  EXPECT_TRUE(DbIsInitialized());
   histogram_tester.ExpectUniqueSample(
       "ResourceCoordinator.LocalDB.DatabaseInit", 1 /* kInitStatusCorruption */,
       1);
@@ -247,7 +264,7 @@ TEST_F(LevelDBSiteDataStoreTest, DatabaseOpeningFailure) {
   ScopedReadOnlyDirectory read_only_dir(GetTempPath());
 
   OpenDB(read_only_dir.GetReadOnlyPath());
-  EXPECT_FALSE(db_->DatabaseIsInitializedForTesting());
+  EXPECT_FALSE(DbIsInitialized());
 
   SiteDataProto proto_temp;
   EXPECT_FALSE(
@@ -263,14 +280,19 @@ TEST_F(LevelDBSiteDataStoreTest, DatabaseOpeningFailure) {
 }
 
 TEST_F(LevelDBSiteDataStoreTest, DBGetsClearedOnVersionUpgrade) {
-  leveldb::DB* raw_db = db_->GetDBForTesting();
-  EXPECT_TRUE(raw_db);
-
   // Remove the entry containing the DB version number, this will cause the DB
   // to be cleared the next time it gets opened.
-  leveldb::Status s = raw_db->Delete(leveldb::WriteOptions(),
-                                     LevelDBSiteDataStore::kDbMetadataKey);
-  EXPECT_TRUE(s.ok());
+  {
+    base::RunLoop run_loop;
+    db_->RunTaskWithRawDBForTesting(base::BindOnce([](leveldb::DB* raw_db) {
+                                      leveldb::Status s = raw_db->Delete(
+                                          leveldb::WriteOptions(),
+                                          LevelDBSiteDataStore::kDbMetadataKey);
+                                      EXPECT_TRUE(s.ok());
+                                    }),
+                                    run_loop.QuitClosure());
+    run_loop.Run();
+  }
 
   // Add some dummy data to the data store to ensure the data store gets cleared
   // when upgrading it to the new version.
@@ -284,14 +306,23 @@ TEST_F(LevelDBSiteDataStoreTest, DBGetsClearedOnVersionUpgrade) {
 
   // Reopen the data store and ensure that it has been cleared.
   OpenDB();
-  raw_db = db_->GetDBForTesting();
-  std::string db_metadata;
-  s = raw_db->Get(leveldb::ReadOptions(), LevelDBSiteDataStore::kDbMetadataKey,
-                  &db_metadata);
-  EXPECT_TRUE(s.ok());
-  size_t version = std::numeric_limits<size_t>::max();
-  EXPECT_TRUE(base::StringToSizeT(db_metadata, &version));
-  EXPECT_EQ(LevelDBSiteDataStore::kDbVersion, version);
+
+  {
+    base::RunLoop run_loop;
+    db_->RunTaskWithRawDBForTesting(
+        base::BindOnce([](leveldb::DB* raw_db) {
+          std::string db_metadata;
+          leveldb::Status s =
+              raw_db->Get(leveldb::ReadOptions(),
+                          LevelDBSiteDataStore::kDbMetadataKey, &db_metadata);
+          EXPECT_TRUE(s.ok());
+          size_t version = std::numeric_limits<size_t>::max();
+          EXPECT_TRUE(base::StringToSizeT(db_metadata, &version));
+          EXPECT_EQ(LevelDBSiteDataStore::kDbVersion, version);
+        }),
+        run_loop.QuitClosure());
+    run_loop.Run();
+  }
 
   SiteDataProto proto_temp;
   EXPECT_FALSE(ReadFromDB(kDummyOrigin, &proto_temp));