Reland "Fix thread safety issues with safe browsing DB."

This is a reland of 0ac8d89e
Original change's description:
> Fix thread safety issues with safe browsing DB.
> 
> Fix thread safety issues with safe browsing DB.
> The Safe Browsing DB has some sequence consistency issues, which
> is caught by the stricter WeakPtr semantics detailed on CL 2908073007.
> 
> * Delete SafeBrowsingDatabaseManager on IO thread.
> * Remove illegal dereferencing of IO-thread WeakPtr from DB thread in
>   V4Database::VerifyChecksumOnTaskRunner
> * Remove non-threadsafe accesses of |io_thread_| resident members from
>   the DB thread in V4LocalDatabaseManager.
> * Add IO-thread runloops to unit tests to handle database teardown.
> 
> 
> R=nparker@chromium.org
> CC=wez@chromium.org
> 
> Bug: 729716
> Change-Id: I1bc620d42aca2f1cc99e482b7776a628d783d390
> Reviewed-on: https://chromium-review.googlesource.com/523983
> Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
> Reviewed-by: Charlie Harrison <csharrison@chromium.org>
> Reviewed-by: Greg Thompson <grt@chromium.org>
> Reviewed-by: Varun Khaneja <vakh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#481735}

TBR=csharrison@chromium.org, grt@chromium.org, vakh@chromium.org

Bug: 729716, 742596
Change-Id: I4d67aa201c54be7a1e75a7375ac9861a29f1d87f
Reviewed-on: https://chromium-review.googlesource.com/571684Reviewed-by: Kevin Marshall <kmarshall@chromium.org>
Commit-Queue: Kevin Marshall <kmarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486946}

chrome/browser/safe_browsing/incident_reporting/resource_request_detector_unittest.cc

@@ -81,8 +81,15 @@ class ResourceRequestDetectorTest : public testing::Test {
             new StrictMock<safe_browsing::MockIncidentReceiver>()),
         mock_database_manager_(new StrictMock<MockSafeBrowsingDatabaseManager>),
         fake_resource_request_detector_(
-            mock_database_manager_,
-            base::WrapUnique(mock_incident_receiver_)) {}
+            base::MakeUnique<FakeResourceRequestDetector>(
+                mock_database_manager_,
+                base::WrapUnique(mock_incident_receiver_))) {}
+
+  void TearDown() override {
+    fake_resource_request_detector_.reset();
+    mock_database_manager_ = nullptr;
+    base::RunLoop().RunUntilIdle();
+  }
 
   std::unique_ptr<net::URLRequest> GetTestURLRequest(
       const std::string& url,
@@ -135,7 +142,7 @@ class ResourceRequestDetectorTest : public testing::Test {
 
     ResourceRequestInfo info =
         ResourceRequestDetector::GetRequestInfo(request.get());
-    fake_resource_request_detector_.ProcessResourceRequest(&info);
+    fake_resource_request_detector_->ProcessResourceRequest(&info);
     base::RunLoop().RunUntilIdle();
   }
 
@@ -152,7 +159,7 @@ class ResourceRequestDetectorTest : public testing::Test {
 
     ResourceRequestInfo info =
         ResourceRequestDetector::GetRequestInfo(request.get());
-    fake_resource_request_detector_.ProcessResourceRequest(&info);
+    fake_resource_request_detector_->ProcessResourceRequest(&info);
     base::RunLoop().RunUntilIdle();
 
     ASSERT_TRUE(incident);
@@ -168,7 +175,7 @@ class ResourceRequestDetectorTest : public testing::Test {
 
   StrictMock<safe_browsing::MockIncidentReceiver>* mock_incident_receiver_;
   scoped_refptr<MockSafeBrowsingDatabaseManager> mock_database_manager_;
-  FakeResourceRequestDetector fake_resource_request_detector_;
+  std::unique_ptr<FakeResourceRequestDetector> fake_resource_request_detector_;
 
  private:
   // UrlRequest requires a message loop. This provides one.

components/safe_browsing_db/BUILD.gn

@@ -500,6 +500,7 @@ source_set("unit_tests_mobile") {
     ":v4_test_util",
     "//base",
     "//components/variations",
+    "//content/test:test_support",
     "//testing/gtest",
     "//url",
   ]
@@ -531,6 +532,8 @@ source_set("whitelist_checker_client_unittest") {
     ":whitelist_checker_client",
     "//base:base",
     "//base/test:test_support",
+    "//content/public/browser",
+    "//content/test:test_support",
     "//testing/gmock:gmock",
     "//testing/gtest:gtest",
   ]

components/safe_browsing_db/database_manager.cc

@@ -15,7 +15,11 @@ using content::BrowserThread;
 
 namespace safe_browsing {
 
-SafeBrowsingDatabaseManager::SafeBrowsingDatabaseManager() : enabled_(false) {}
+SafeBrowsingDatabaseManager::SafeBrowsingDatabaseManager()
+    : base::RefCountedDeleteOnSequence<SafeBrowsingDatabaseManager>(
+          content::BrowserThread::GetTaskRunnerForThread(
+              content::BrowserThread::IO)),
+      enabled_(false) {}
 
 SafeBrowsingDatabaseManager::~SafeBrowsingDatabaseManager() {
   DCHECK(!v4_get_hash_protocol_manager_);

components/safe_browsing_db/database_manager.h

@@ -15,7 +15,7 @@
 
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
-#include "base/memory/ref_counted.h"
+#include "base/memory/ref_counted_delete_on_sequence.h"
 #include "components/safe_browsing_db/hit_report.h"
 #include "components/safe_browsing_db/util.h"
 #include "components/safe_browsing_db/v4_protocol_manager_util.h"
@@ -41,7 +41,7 @@ class V4GetHashProtocolManager;
 
 // Base class to either the locally-managed or a remotely-managed database.
 class SafeBrowsingDatabaseManager
-    : public base::RefCountedThreadSafe<SafeBrowsingDatabaseManager> {
+    : public base::RefCountedDeleteOnSequence<SafeBrowsingDatabaseManager> {
  public:
   // Callers requesting a result should derive from this class.
   // The destructor should call db_manager->CancelCheck(client) if a
@@ -264,7 +264,8 @@ class SafeBrowsingDatabaseManager
 
   virtual ~SafeBrowsingDatabaseManager();
 
-  friend class base::RefCountedThreadSafe<SafeBrowsingDatabaseManager>;
+  friend class base::RefCountedDeleteOnSequence<SafeBrowsingDatabaseManager>;
+  friend class base::DeleteHelper<SafeBrowsingDatabaseManager>;
 
   FRIEND_TEST_ALL_PREFIXES(SafeBrowsingDatabaseManagerTest,
                            CheckApiBlacklistUrlPrefixes);

components/safe_browsing_db/database_manager_unittest.cc

@@ -17,6 +17,7 @@
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/safe_browsing_db/test_database_manager.h"
 #include "components/safe_browsing_db/v4_protocol_manager_util.h"
@@ -68,8 +69,9 @@ class SafeBrowsingDatabaseManagerTest : public testing::Test {
   }
 
   void TearDown() override {
-    base::RunLoop().RunUntilIdle();
     db_manager_->StopOnIOThread(false);
+    db_manager_ = nullptr;
+    base::RunLoop().RunUntilIdle();
   }
 
   std::string GetStockV4GetHashResponse() {

components/safe_browsing_db/remote_database_manager_unittest.cc

@@ -8,10 +8,12 @@
 
 #include "base/logging.h"
 #include "base/metrics/field_trial.h"
+#include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/time/time.h"
 #include "components/safe_browsing_db/safe_browsing_api_handler.h"
 #include "components/variations/variations_associated_data.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace safe_browsing {
@@ -37,6 +39,11 @@ class RemoteDatabaseManagerTest : public testing::Test {
     db_ = new RemoteSafeBrowsingDatabaseManager();
   }
 
+  void TearDown() override {
+    db_ = nullptr;
+    base::RunLoop().RunUntilIdle();
+  }
+
   // Setup the two field trial params.  These are read in db_'s ctor.
   void SetFieldTrialParams(const std::string types_to_check_val) {
     // Destroy the existing FieldTrialList before creating a new one to avoid
@@ -59,6 +66,7 @@ class RemoteDatabaseManagerTest : public testing::Test {
                                                      group_name, params));
   }
 
+  content::TestBrowserThreadBundle thread_bundle_;
   std::unique_ptr<base::FieldTrialList> field_trials_;
   TestSafeBrowsingApiHandler api_handler_;
   scoped_refptr<RemoteSafeBrowsingDatabaseManager> db_;

components/safe_browsing_db/v4_database.cc

@@ -9,6 +9,7 @@
 #include "base/lazy_instance.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/task_runner_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/safe_browsing_db/v4_database.h"
 #include "content/public/browser/browser_thread.h"
@@ -30,6 +31,19 @@ base::LazyInstance<std::unique_ptr<V4DatabaseFactory>>::Leaky g_db_factory =
 base::LazyInstance<std::unique_ptr<V4StoreFactory>>::Leaky g_store_factory =
     LAZY_INSTANCE_INITIALIZER;
 
+// Verifies the checksums on a collection of stores.
+// Returns the IDs of stores whose checksums failed to verify.
+std::vector<ListIdentifier> VerifyChecksums(
+    std::vector<std::pair<ListIdentifier, V4Store*>> stores) {
+  std::vector<ListIdentifier> stores_to_reset;
+  for (const auto& store_map_iter : stores) {
+    if (!store_map_iter.second->VerifyChecksum()) {
+      stores_to_reset.push_back(store_map_iter.first);
+    }
+  }
+  return stores_to_reset;
+}
+
 }  // namespace
 
 std::unique_ptr<V4Database> V4DatabaseFactory::Create(
@@ -254,28 +268,19 @@ void V4Database::ResetStores(
 void V4Database::VerifyChecksum(
     DatabaseReadyForUpdatesCallback db_ready_for_updates_callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  // TODO(vakh): Consider using PostTaskAndReply instead.
-  const scoped_refptr<base::SingleThreadTaskRunner> callback_task_runner =
-      base::ThreadTaskRunnerHandle::Get();
-  db_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&V4Database::VerifyChecksumOnTaskRunner,
-                 weak_factory_on_io_.GetWeakPtr(), callback_task_runner,
-                 db_ready_for_updates_callback));
-}
 
-void V4Database::VerifyChecksumOnTaskRunner(
-    const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner,
-    DatabaseReadyForUpdatesCallback db_ready_for_updates_callback) {
-  std::vector<ListIdentifier> stores_to_reset;
-  for (const auto& store_map_iter : *store_map_) {
-    if (!store_map_iter.second->VerifyChecksum()) {
-      stores_to_reset.push_back(store_map_iter.first);
-    }
+  // Make a threadsafe copy of store_map_ w/raw pointers that we can hand to
+  // the DB thread. The V4Stores ptrs are guaranteed to be valid because their
+  // deletion would be sequenced on the DB thread, after this posted task is
+  // serviced.
+  std::vector<std::pair<ListIdentifier, V4Store*>> stores;
+  for (const auto& next_store : *store_map_) {
+    stores.push_back(std::make_pair(next_store.first, next_store.second.get()));
   }
 
-  callback_task_runner->PostTask(
-      FROM_HERE, base::Bind(db_ready_for_updates_callback, stores_to_reset));
+  base::PostTaskAndReplyWithResult(db_task_runner_.get(), FROM_HERE,
+                                   base::Bind(&VerifyChecksums, stores),
+                                   db_ready_for_updates_callback);
 }
 
 bool V4Database::IsStoreAvailable(const ListIdentifier& identifier) const {

components/safe_browsing_db/v4_database.h

@@ -167,7 +167,11 @@ class V4Database {
   V4Database(const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
              std::unique_ptr<StoreMap> store_map);
 
-  // Map of ListIdentifier to the V4Store.
+  // The collection of V4Stores, keyed by ListIdentifier.
+  // The map itself lives on the V4Database's parent thread, but its V4Store
+  // objects live on the db_task_runner_thread.
+  // TODO(vakh): Consider writing a container object which encapsulates or
+  // harmonizes thread affinity for the associative container and the data.
   const std::unique_ptr<StoreMap> store_map_;
 
  private:

components/safe_browsing_db/v4_local_database_manager.h

@@ -334,7 +334,6 @@ class V4LocalDatabaseManager : public SafeBrowsingDatabaseManager {
 
   base::WeakPtrFactory<V4LocalDatabaseManager> weak_factory_;
 
-  friend class base::RefCountedThreadSafe<V4LocalDatabaseManager>;
   DISALLOW_COPY_AND_ASSIGN(V4LocalDatabaseManager);
 };  // class V4LocalDatabaseManager
 

components/safe_browsing_db/whitelist_checker_client_unittest.cc

@@ -10,6 +10,8 @@
 #include "base/test/test_mock_time_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "components/safe_browsing_db/test_database_manager.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_browser_thread.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -39,17 +41,22 @@ class MockSafeBrowsingDatabaseManager : public TestSafeBrowsingDatabaseManager {
 
 class WhitelistCheckerClientTest : public testing::Test {
  public:
-  WhitelistCheckerClientTest() : target_url_("http://foo.bar"){};
+  WhitelistCheckerClientTest() : target_url_("http://foo.bar") {}
 
   void SetUp() override {
     database_manager_ = new MockSafeBrowsingDatabaseManager;
     task_runner_ = new base::TestMockTimeTaskRunner(base::Time::Now(),
                                                     base::TimeTicks::Now());
     message_loop_.reset(new base::MessageLoop);
+    io_thread_ = base::MakeUnique<content::TestBrowserThread>(
+        content::BrowserThread::IO, base::MessageLoop::current());
     message_loop_->SetTaskRunner(task_runner_);
   }
 
   void TearDown() override {
+    database_manager_ = nullptr;
+    base::RunLoop().RunUntilIdle();
+
     // Verify no callback is remaining.
     // TODO(nparker): We should somehow EXPECT that no entry is remaining,
     // rather than just invoking it.
@@ -60,6 +67,9 @@ class WhitelistCheckerClientTest : public testing::Test {
   GURL target_url_;
   scoped_refptr<MockSafeBrowsingDatabaseManager> database_manager_;
 
+  // Needed for |database_manager_| teardown tasks.
+  std::unique_ptr<content::TestBrowserThread> io_thread_;
+
   std::unique_ptr<base::MessageLoop> message_loop_;
   scoped_refptr<base::TestMockTimeTaskRunner> task_runner_;
 };

components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc

@@ -193,7 +193,17 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest
 
   void TearDown() override {
     client_.reset();
+
+    // RunUntilIdle() must be called multiple times to flush any outstanding
+    // cross-thread interactions.
+    // TODO(csharrison): Clean up test teardown logic.
+    RunUntilIdle();
+    RunUntilIdle();
+
+    // RunUntilIdle() called once more, to delete the database on the IO thread.
+    fake_safe_browsing_database_ = nullptr;
     RunUntilIdle();
+
     content::RenderViewHostTestHarness::TearDown();
   }
 
@@ -316,8 +326,8 @@ class SubresourceFilterSafeBrowsingActivationThrottleTest
   void UsePassThroughThrottle() { fake_safe_browsing_database_ = nullptr; }
 
   void RunUntilIdle() {
-    test_io_task_runner_->RunUntilIdle();
     base::RunLoop().RunUntilIdle();
+    test_io_task_runner_->RunUntilIdle();
   }
 
   content::NavigationSimulator* navigation_simulator() {