[SafeBrowsing] Remove thread_utils.h from V4Database
thread_utils.h is a utility that allows //components/safe_browsing/core
to use //content's and //ios/web's threading primitives via different
implementations that are conditionally included in the GN target. We
are going to eliminate this utility as part of eliminating
//components/safe_browsing/core's incorrect dependences on
//components/safe_browsing/{content, ios}.
This CL removes usage of thread_utils.h in
//components/safe_browsing/core's V4Database. This class executes
on the DB and IO thread and uses thread_utils.h to verify that certain
methods are called on the IO thread. We replace those DCHECKs with
usage of a ThreadChecker. However, it is slightly tricky to do so as
the object is created on the DB thread and never explicitly told which
thread is the IO thread. To handle this, we add a
V4Database::InitializeOnIOThread() method that the client is responsible
for invoking on the IO thread.
Bug: 1216341
Change-Id: I439d90eb8a0bdaf74f918471b0976b639c2cec6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2966500
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#893746}
diff --git a/components/safe_browsing/core/db/v4_database.cc b/components/safe_browsing/core/db/v4_database.cc
index 55d96696..35020a3 100644
--- a/components/safe_browsing/core/db/v4_database.cc
+++ b/components/safe_browsing/core/db/v4_database.cc
@@ -15,7 +15,6 @@
#include "base/metrics/histogram_macros.h"
#include "base/task_runner_util.h"
#include "base/threading/thread_task_runner_handle.h"
-#include "components/safe_browsing/core/common/thread_utils.h"
#include "components/safe_browsing/core/proto/webui.pb.h"
using base::TimeTicks;
@@ -132,13 +131,25 @@
db_task_runner_(db_task_runner),
pending_store_updates_(0) {
DCHECK(db_task_runner->RunsTasksInCurrentSequence());
+ // This method executes on the DB task runner, whereas |io_thread_checker_| is
+ // meant to verify methods that should execute on the IO thread. Detach that
+ // thread checker here; it will be bound to the IO thread in
+ // InitializeOnIOThread().
+ DETACH_FROM_THREAD(io_thread_checker_);
+}
+
+void V4Database::InitializeOnIOThread() {
+ // This invocation serves to bind |io_thread_checker_| to the IO thread after
+ // its having been detached from the DB task runner in this object's
+ // constructor.
+ DCHECK_CALLED_ON_VALID_THREAD(io_thread_checker_);
}
// static
void V4Database::Destroy(std::unique_ptr<V4Database> v4_database) {
- DCHECK(CurrentlyOnThread(ThreadID::IO));
V4Database* v4_database_raw = v4_database.release();
if (v4_database_raw) {
+ DCHECK_CALLED_ON_VALID_THREAD(v4_database_raw->io_thread_checker_);
v4_database_raw->weak_factory_on_io_.InvalidateWeakPtrs();
v4_database_raw->db_task_runner_->DeleteSoon(FROM_HERE, v4_database_raw);
}
@@ -151,7 +162,7 @@
void V4Database::ApplyUpdate(
std::unique_ptr<ParsedServerResponse> parsed_server_response,
DatabaseUpdatedCallback db_updated_callback) {
- DCHECK(CurrentlyOnThread(ThreadID::IO));
+ DCHECK_CALLED_ON_VALID_THREAD(io_thread_checker_);
DCHECK(!pending_store_updates_);
DCHECK(db_updated_callback_.is_null());
@@ -192,7 +203,7 @@
void V4Database::UpdatedStoreReady(ListIdentifier identifier,
std::unique_ptr<V4Store> new_store) {
- DCHECK(CurrentlyOnThread(ThreadID::IO));
+ DCHECK_CALLED_ON_VALID_THREAD(io_thread_checker_);
DCHECK(pending_store_updates_);
if (new_store) {
(*store_map_)[identifier].swap(new_store);
@@ -218,7 +229,7 @@
bool V4Database::AreAnyStoresAvailable(
const StoresToCheck& stores_to_check) const {
- DCHECK(CurrentlyOnThread(ThreadID::IO));
+ DCHECK_CALLED_ON_VALID_THREAD(io_thread_checker_);
for (const ListIdentifier& identifier : stores_to_check) {
if (IsStoreAvailable(identifier))
return true;
@@ -228,7 +239,7 @@
bool V4Database::AreAllStoresAvailable(
const StoresToCheck& stores_to_check) const {
- DCHECK(CurrentlyOnThread(ThreadID::IO));
+ DCHECK_CALLED_ON_VALID_THREAD(io_thread_checker_);
for (const ListIdentifier& identifier : stores_to_check) {
if (!IsStoreAvailable(identifier))
return false;
@@ -240,7 +251,7 @@
const FullHash& full_hash,
const StoresToCheck& stores_to_check,
StoreAndHashPrefixes* matched_store_and_hash_prefixes) {
- DCHECK(CurrentlyOnThread(ThreadID::IO));
+ DCHECK_CALLED_ON_VALID_THREAD(io_thread_checker_);
matched_store_and_hash_prefixes->clear();
for (const ListIdentifier& identifier : stores_to_check) {
if (!IsStoreAvailable(identifier))
@@ -257,7 +268,7 @@
void V4Database::ResetStores(
const std::vector<ListIdentifier>& stores_to_reset) {
- DCHECK(CurrentlyOnThread(ThreadID::IO));
+ DCHECK_CALLED_ON_VALID_THREAD(io_thread_checker_);
for (const ListIdentifier& identifier : stores_to_reset) {
store_map_->at(identifier)->Reset();
}
@@ -265,7 +276,7 @@
void V4Database::VerifyChecksum(
DatabaseReadyForUpdatesCallback db_ready_for_updates_callback) {
- DCHECK(CurrentlyOnThread(ThreadID::IO));
+ DCHECK_CALLED_ON_VALID_THREAD(io_thread_checker_);
// 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
@@ -287,7 +298,7 @@
void V4Database::OnChecksumVerified(
DatabaseReadyForUpdatesCallback db_ready_for_updates_callback,
const std::vector<ListIdentifier>& stores_to_reset) {
- DCHECK(CurrentlyOnThread(ThreadID::IO));
+ DCHECK_CALLED_ON_VALID_THREAD(io_thread_checker_);
std::move(db_ready_for_updates_callback).Run(stores_to_reset);
}
diff --git a/components/safe_browsing/core/db/v4_database.h b/components/safe_browsing/core/db/v4_database.h
index cb920eb..109e6b6 100644
--- a/components/safe_browsing/core/db/v4_database.h
+++ b/components/safe_browsing/core/db/v4_database.h
@@ -16,6 +16,7 @@
#include "base/memory/weak_ptr.h"
#include "base/sequenced_task_runner.h"
#include "base/single_thread_task_runner.h"
+#include "base/threading/thread_checker.h"
#include "components/safe_browsing/core/db/v4_protocol_manager_util.h"
#include "components/safe_browsing/core/db/v4_store.h"
#include "components/safe_browsing/core/proto/webui.pb.h"
@@ -106,12 +107,17 @@
// provided |db_task_runner| containing stores in |store_file_name_map|. When
// the database creation is complete, it runs the NewDatabaseReadyCallback on
// the same thread as it was called.
+ // NOTE: Within |new_db_callback| the client should invoke
+ // V4Database::InitializeOnIOThread() on the IO thread.
static void Create(
const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
const base::FilePath& base_path,
const ListInfos& list_infos,
NewDatabaseReadyCallback new_db_callback);
+ // Initialize state that lives on the IO thread.
+ void InitializeOnIOThread();
+
// Destroys the provided v4_database on its task_runner since this may be a
// long operation.
static void Destroy(std::unique_ptr<V4Database> v4_database);
@@ -229,6 +235,9 @@
bool IsStoreAvailable(const ListIdentifier& identifier) const;
+ // Used to verify that certain methods are called on the IO thread.
+ THREAD_CHECKER(io_thread_checker_);
+
const scoped_refptr<base::SequencedTaskRunner> db_task_runner_;
DatabaseUpdatedCallback db_updated_callback_;
diff --git a/components/safe_browsing/core/db/v4_local_database_manager.cc b/components/safe_browsing/core/db/v4_local_database_manager.cc
index 5a41c92..0ed38ea 100644
--- a/components/safe_browsing/core/db/v4_local_database_manager.cc
+++ b/components/safe_browsing/core/db/v4_local_database_manager.cc
@@ -676,6 +676,8 @@
std::unique_ptr<V4Database> v4_database) {
DCHECK(io_task_runner()->RunsTasksInCurrentSequence());
+ v4_database->InitializeOnIOThread();
+
// The following check is needed because it is possible that by the time the
// database is ready, StopOnIOThread has been called.
if (enabled_) {
