[Merge 116] Fix MmapHashPrefixMap when Clear is called on db task runner

This was causing crbug.com/1459039 since in that case posting a task
with Unretained is unsafe.

(cherry picked from commit 0538574)

This also includes the follow up fix in crrev.com/c/4652938
(cherry picked from commit e2df683)

As well as the follow up fix in crrev.com/c/4647449
(cherry picked from commit 64af24a)

Bug: 1409674
Change-Id: I851265528e0bf6996f3f994d3866b982ad163bc6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4655068
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1163817}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4656807
Auto-Submit: Clark DuVall <cduvall@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/branch-heads/5845@{#243}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}

components/safe_browsing/core/browser/db/hash_prefix_map.cc

@@ -8,6 +8,7 @@
 #include "base/files/file_util.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/raw_ref.h"
+#include "base/run_loop.h"
 #include "base/strings/strcat.h"
 #include "components/safe_browsing/core/browser/db/prefix_iterator.h"
 #include "components/safe_browsing/core/common/features.h"
@@ -341,12 +342,36 @@ class MmapHashPrefixMap::BufferedFileWriter {
   bool has_error_;
 };
 
-MmapHashPrefixMap::MmapHashPrefixMap(const base::FilePath& store_path,
-                                     size_t buffer_size)
-    : store_path_(store_path), buffer_size_(buffer_size) {}
-MmapHashPrefixMap::~MmapHashPrefixMap() = default;
+MmapHashPrefixMap::MmapHashPrefixMap(
+    const base::FilePath& store_path,
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    size_t buffer_size)
+    : store_path_(store_path),
+      task_runner_(task_runner
+                       ? std::move(task_runner)
+                       : base::SequencedTaskRunner::GetCurrentDefault()),
+      buffer_size_(buffer_size) {}
+
+MmapHashPrefixMap::~MmapHashPrefixMap() {
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
+}
 
 void MmapHashPrefixMap::Clear() {
+  if (kMmapSafeBrowsingDatabaseAsync.Get() &&
+      !task_runner_->RunsTasksInCurrentSequence()) {
+    // Clear the map on the db task runner, since the memory mapped files should
+    // be destroyed on the same thread they were created. base::Unretained is
+    // safe since the map is destroyed on the db task runner.
+    task_runner_->PostTask(FROM_HERE,
+                           base::BindOnce(&MmapHashPrefixMap::ClearOnTaskRunner,
+                                          base::Unretained(this)));
+  } else {
+    map_.clear();
+  }
+}
+
+void MmapHashPrefixMap::ClearOnTaskRunner() {
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
   map_.clear();
 }
 
@@ -508,6 +533,13 @@ const std::string& MmapHashPrefixMap::GetExtensionForTesting(PrefixSize size) {
   return GetFileInfo(size).GetExtensionForTesting();  // IN-TEST
 }
 
+void MmapHashPrefixMap::ClearAndWaitForTesting() {
+  Clear();
+  base::RunLoop run_loop;
+  task_runner_->PostTask(FROM_HERE, run_loop.QuitClosure());
+  run_loop.Run();
+}
+
 MmapHashPrefixMap::FileInfo& MmapHashPrefixMap::GetFileInfo(PrefixSize size) {
   auto [it, inserted] = map_.try_emplace(size, store_path_, size);
   return it->second;

components/safe_browsing/core/browser/db/hash_prefix_map.h

@@ -168,8 +168,10 @@ class InMemoryHashPrefixMap : public HashPrefixMap {
 // prefix size. These will be mapped into memory on initialization.
 class MmapHashPrefixMap : public HashPrefixMap {
  public:
-  explicit MmapHashPrefixMap(const base::FilePath& store_path,
-                             size_t buffer_size = 1024 * 512);
+  explicit MmapHashPrefixMap(
+      const base::FilePath& store_path,
+      scoped_refptr<base::SequencedTaskRunner> task_runner = nullptr,
+      size_t buffer_size = 1024 * 512);
   ~MmapHashPrefixMap() override;
 
   // HashPrefixMap implementation:
@@ -193,6 +195,7 @@ class MmapHashPrefixMap : public HashPrefixMap {
                                 const std::string& extension);
 
   const std::string& GetExtensionForTesting(PrefixSize size);
+  void ClearAndWaitForTesting();
 
  private:
   class BufferedFileWriter;
@@ -221,8 +224,10 @@ class MmapHashPrefixMap : public HashPrefixMap {
   };
 
   FileInfo& GetFileInfo(PrefixSize size);
+  void ClearOnTaskRunner();
 
   base::FilePath store_path_;
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
   std::unordered_map<PrefixSize, FileInfo> map_;
   size_t buffer_size_;
 };

components/safe_browsing/core/browser/db/hash_prefix_map_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/ranges/algorithm.h"
 #include "base/strings/string_util.h"
 #include "base/test/scoped_feature_list.h"
+#include "base/test/task_environment.h"
 #include "components/safe_browsing/core/common/features.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/platform_test.h"
@@ -48,6 +49,7 @@ class HashPrefixMapTest : public PlatformTest {
 
   base::Time time_ = base::Time::Now();
   base::ScopedTempDir temp_dir_;
+  base::test::SingleThreadTaskEnvironment task_env_;
 };
 
 TEST_F(HashPrefixMapTest, WriteFile) {
@@ -106,7 +108,9 @@ TEST_F(HashPrefixMapTest, WriteMultipleFiles) {
 }
 
 TEST_F(HashPrefixMapTest, BuffersWrites) {
-  MmapHashPrefixMap map(GetBasePath(), /*buffer_size=*/4);
+  MmapHashPrefixMap map(GetBasePath(),
+                        base::SequencedTaskRunner::GetCurrentDefault(),
+                        /*buffer_size=*/4);
 
   map.Append(4, "fooo");
   EXPECT_EQ(GetContents(map.GetExtensionForTesting(4)), "");
@@ -226,13 +230,15 @@ TEST_F(HashPrefixMapTest, WriteAndReadFile) {
 }
 
 TEST_F(HashPrefixMapTest, ClearingMapBeforeWriteDeletesFile) {
-  MmapHashPrefixMap map(GetBasePath(), /*buffer_size=*/1);
+  MmapHashPrefixMap map(GetBasePath(),
+                        base::SequencedTaskRunner::GetCurrentDefault(),
+                        /*buffer_size=*/1);
   map.Append(4, "foo");
 
   std::string extension = map.GetExtensionForTesting(4);
   EXPECT_EQ(GetContents(extension), "foo");
 
-  map.Clear();
+  map.ClearAndWaitForTesting();
   EXPECT_FALSE(base::PathExists(GetPath(extension)));
 }
 
@@ -325,7 +331,7 @@ TEST_F(HashPrefixMapTest, UsesFileOffsets) {
   V4StoreFileFormat file_format;
   EXPECT_TRUE(map.WriteToDisk(&file_format));
   EXPECT_EQ(map.IsValid(), APPLY_UPDATE_SUCCESS);
-  map.Clear();
+  map.ClearAndWaitForTesting();
 
   EXPECT_EQ(file_format.hash_files().size(), 1);
   const auto& hash_file = file_format.hash_files(0);
@@ -442,6 +448,7 @@ class HashPrefixMapTypedTest : public ::testing::Test {
 
   absl::optional<base::ScopedTempDir> temp_dir_;
   std::unique_ptr<HashPrefixMap> hash_prefix_map_;
+  base::test::SingleThreadTaskEnvironment task_env_;
 };
 
 template <>

components/safe_browsing/core/browser/db/v4_store.cc

@@ -181,9 +181,11 @@ const base::FilePath TemporaryFileForFilename(const base::FilePath& filename) {
 }
 
 std::unique_ptr<HashPrefixMap> CreateHashPrefixMap(
-    const base::FilePath& store_path) {
+    const base::FilePath& store_path,
+    scoped_refptr<base::SequencedTaskRunner> task_runner) {
   if (base::FeatureList::IsEnabled(kMmapSafeBrowsingDatabase))
-    return std::make_unique<MmapHashPrefixMap>(store_path);
+    return std::make_unique<MmapHashPrefixMap>(store_path,
+                                               std::move(task_runner));
   return std::make_unique<InMemoryHashPrefixMap>();
 }
 
@@ -349,8 +351,8 @@ std::ostream& operator<<(std::ostream& os, const V4Store& store) {
 std::unique_ptr<V4Store> V4StoreFactory::CreateV4Store(
     const scoped_refptr<base::SequencedTaskRunner>& task_runner,
     const base::FilePath& store_path) {
-  auto new_store = std::make_unique<V4Store>(task_runner, store_path,
-                                             CreateHashPrefixMap(store_path));
+  auto new_store = std::make_unique<V4Store>(
+      task_runner, store_path, CreateHashPrefixMap(store_path, task_runner));
   new_store->Initialize();
   return new_store;
 }
@@ -533,7 +535,8 @@ void V4Store::ApplyUpdate(
     UpdatedStoreReadyCallback callback) {
   base::ElapsedThreadTimer thread_timer;
   auto new_store = std::make_unique<V4Store>(
-      task_runner_, store_path_, CreateHashPrefixMap(store_path_), file_size_);
+      task_runner_, store_path_, CreateHashPrefixMap(store_path_, task_runner_),
+      file_size_);
   ApplyUpdateResult apply_update_result;
   std::string metric;
   if (response->response_type() == ListUpdateResponse::PARTIAL_UPDATE) {

components/safe_browsing/core/browser/db/v4_store_unittest.cc

@@ -1034,14 +1034,18 @@ TEST_F(V4StoreTest, MigrateToInMemory) {
   EXPECT_EQ(write_store.GetMatchingHashPrefix(kFullHash), kHash);
 
   // Make sure a mmap store can read correctly.
-  V4Store mmap_store(task_runner_, store_path_,
-                     std::make_unique<MmapHashPrefixMap>(store_path_));
+  auto mmap_map2 = std::make_unique<MmapHashPrefixMap>(store_path_);
+  auto* mmap_map_raw2 = mmap_map2.get();
+  V4Store mmap_store(task_runner_, store_path_, std::move(mmap_map2));
   EXPECT_EQ(READ_SUCCESS, mmap_store.ReadFromDisk());
   EXPECT_EQ("test_client_state", mmap_store.state());
   EXPECT_EQ(mmap_store.hash_prefix_map_->view()[5], kHash);
   EXPECT_TRUE(base::PathExists(hashes_path));
   EXPECT_EQ(mmap_store.GetMatchingHashPrefix(kFullHash), kHash);
 
+  mmap_map_raw->ClearAndWaitForTesting();
+  mmap_map_raw2->ClearAndWaitForTesting();
+
   write_store.Reset();
   mmap_store.Reset();
 
@@ -1137,6 +1141,8 @@ TEST_F(V4StoreTest, FileSizeIncludesHashFiles) {
 
   int64_t original_file_size = write_store.file_size();
 
+  static_cast<MmapHashPrefixMap*>(write_store.hash_prefix_map_.get())
+      ->ClearAndWaitForTesting();
   write_store.Reset();
   write_store.hash_prefix_map_->Append(4, "abcd");
   write_store.hash_prefix_map_->Append(4, "efgh");