More explicit thread checking in SafeBrowsingDatabase.

chrome/browser/safe_browsing/safe_browsing_database.cc

Index: chrome/browser/safe_browsing/safe_browsing_database.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.cc b/chrome/browser/safe_browsing/safe_browsing_database.cc
index 853219493a3f66e906d26be27d4fae7aa53a3152..2db37e202634ca6b36486947f81c46872ab102c7 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -432,6 +432,9 @@ base::FilePath SafeBrowsingDatabase::UnwantedSoftwareDBFilename(
 }
 
 SafeBrowsingStore* SafeBrowsingDatabaseNew::GetStore(const int list_id) {
+  // Stores are not thread safe.
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   if (list_id == safe_browsing_util::PHISH ||
       list_id == safe_browsing_util::MALWARE) {
     return browse_store_.get();
@@ -487,8 +490,7 @@ SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew(
     SafeBrowsingStore* side_effect_free_whitelist_store,
     SafeBrowsingStore* ip_blacklist_store,
     SafeBrowsingStore* unwanted_software_store)
-    : creation_loop_(base::MessageLoop::current()),
-      browse_store_(browse_store),
+    : browse_store_(browse_store),
       download_store_(download_store),
       csd_whitelist_store_(csd_whitelist_store),
       download_whitelist_store_(download_whitelist_store),
@@ -504,11 +506,11 @@ SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew(
 
 SafeBrowsingDatabaseNew::~SafeBrowsingDatabaseNew() {
   // The DCHECK is disabled due to crbug.com/338486 .
-  // DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  // DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 void SafeBrowsingDatabaseNew::Init(const base::FilePath& filename_base) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   // This should not be run multiple times.
   DCHECK(filename_base_.empty());
@@ -635,7 +637,7 @@ void SafeBrowsingDatabaseNew::Init(const base::FilePath& filename_base) {
 }
 
 bool SafeBrowsingDatabaseNew::ResetDatabase() {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   // Delete files on disk.
   // TODO(shess): Hard to see where one might want to delete without a
@@ -662,6 +664,10 @@ bool SafeBrowsingDatabaseNew::ContainsBrowseUrl(
     const GURL& url,
     std::vector<SBPrefix>* prefix_hits,
     std::vector<SBFullHashResult>* cache_hits) {
+  // This method is theoretically thread-safe but document that it is currently
+  // only expected to be called on the IO thread.
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);

[mattm, 2014/12/12 23:50:44]

Think I still don't see the benefit of these documentation checks, especially if
they're just going to be removed in the next CL.

[gab, 2014/12/15 22:05:37]

Ok, agreed, they mostly helped me understanding how all of this was hooked
together and I thought it would be useful to a future reader (i.e., have every
method annotated), but the ThreadSafeStateManager will make this
self-documenting anyways.

Removed.

+
   return PrefixSetContainsUrl(
       url, &browse_prefix_set_, prefix_hits, cache_hits);
 }
@@ -670,6 +676,10 @@ bool SafeBrowsingDatabaseNew::ContainsUnwantedSoftwareUrl(
     const GURL& url,
     std::vector<SBPrefix>* prefix_hits,
     std::vector<SBFullHashResult>* cache_hits) {
+  // This method is theoretically thread-safe but document that it is currently
+  // only expected to be called on the IO thread.
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   return PrefixSetContainsUrl(
       url, &unwanted_software_prefix_set_, prefix_hits, cache_hits);
 }
@@ -679,6 +689,10 @@ bool SafeBrowsingDatabaseNew::PrefixSetContainsUrl(
     scoped_ptr<const safe_browsing::PrefixSet>* prefix_set_getter,
     std::vector<SBPrefix>* prefix_hits,
     std::vector<SBFullHashResult>* cache_hits) {
+  // This method is theoretically thread-safe but document that it is currently
+  // only expected to be called on the IO thread.
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   // Clear the results first.
   prefix_hits->clear();
   cache_hits->clear();
@@ -705,11 +719,13 @@ bool SafeBrowsingDatabaseNew::PrefixSetContainsUrlHashes(
     scoped_ptr<const safe_browsing::PrefixSet>* prefix_set_getter,
     std::vector<SBPrefix>* prefix_hits,
     std::vector<SBFullHashResult>* cache_hits) {
+  // This method is theoretically thread-safe but document that it is currently
+  // only expected to be called on the IO thread.
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   // Used to determine cache expiration.
   const base::Time now = base::Time::Now();
 
-  // This function is called on the I/O thread, prevent changes to
-  // filter and caches.
   base::AutoLock locked(lookup_lock_);
 
   // |prefix_set| is empty until it is either read from disk, or the first
@@ -741,7 +757,7 @@ bool SafeBrowsingDatabaseNew::PrefixSetContainsUrlHashes(
 bool SafeBrowsingDatabaseNew::ContainsDownloadUrl(
     const std::vector<GURL>& urls,
     std::vector<SBPrefix>* prefix_hits) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   // Ignore this check when download checking is not enabled.
   if (!download_store_.get())
@@ -756,9 +772,10 @@ bool SafeBrowsingDatabaseNew::ContainsDownloadUrl(
 }
 
 bool SafeBrowsingDatabaseNew::ContainsCsdWhitelistedUrl(const GURL& url) {
-  // This method is theoretically thread-safe but we expect all calls to
-  // originate from the IO thread.
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  // This method is theoretically thread-safe but document that it is currently
+  // only expected to be called on the IO thread.
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   std::vector<SBFullHash> full_hashes;
   UrlToFullHashes(url, true, &full_hashes);
   return ContainsWhitelistedHashes(csd_whitelist_, full_hashes);
@@ -773,7 +790,8 @@ bool SafeBrowsingDatabaseNew::ContainsDownloadWhitelistedUrl(const GURL& url) {
 bool SafeBrowsingDatabaseNew::ContainsExtensionPrefixes(
     const std::vector<SBPrefix>& prefixes,
     std::vector<SBPrefix>* prefix_hits) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   if (!extension_blacklist_store_)
     return false;
 
@@ -785,6 +803,10 @@ bool SafeBrowsingDatabaseNew::ContainsExtensionPrefixes(
 
 bool SafeBrowsingDatabaseNew::ContainsSideEffectFreeWhitelistUrl(
     const GURL& url) {
+  // This method is theoretically thread-safe but document that it is currently
+  // only expected to be called on the UI thread.
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
   std::string host;
   std::string path;
   std::string query;
@@ -794,7 +816,6 @@ bool SafeBrowsingDatabaseNew::ContainsSideEffectFreeWhitelistUrl(
     url_to_check +=  "?" + query;
   SBFullHash full_hash = SBFullHashForString(url_to_check);
 
-  // This function can be called on any thread, so lock against any changes
   base::AutoLock locked(lookup_lock_);
 
   // |side_effect_free_whitelist_prefix_set_| is empty until it is either read
@@ -807,6 +828,10 @@ bool SafeBrowsingDatabaseNew::ContainsSideEffectFreeWhitelistUrl(
 }
 
 bool SafeBrowsingDatabaseNew::ContainsMalwareIP(const std::string& ip_address) {
+  // This method is theoretically thread-safe but document that it is currently
+  // only expected to be called on the IO thread.
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   net::IPAddressNumber ip_number;
   if (!net::ParseIPLiteralToNumber(ip_address, &ip_number))
     return false;
@@ -815,7 +840,6 @@ bool SafeBrowsingDatabaseNew::ContainsMalwareIP(const std::string& ip_address) {
   if (ip_number.size() != net::kIPv6AddressSize)
     return false;  // better safe than sorry.
 
-  // This function can be called from any thread.
   base::AutoLock locked(lookup_lock_);
   for (IPBlacklist::const_iterator it = ip_blacklist_.begin();
        it != ip_blacklist_.end();
@@ -841,6 +865,10 @@ bool SafeBrowsingDatabaseNew::ContainsMalwareIP(const std::string& ip_address) {
 
 bool SafeBrowsingDatabaseNew::ContainsDownloadWhitelistedString(
     const std::string& str) {
+  // This method is theoretically thread-safe but document that it is currently
+  // only expected to be called on the IO thread.
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   std::vector<SBFullHash> hashes;
   hashes.push_back(SBFullHashForString(str));
   return ContainsWhitelistedHashes(download_whitelist_, hashes);
@@ -849,6 +877,10 @@ bool SafeBrowsingDatabaseNew::ContainsDownloadWhitelistedString(
 bool SafeBrowsingDatabaseNew::ContainsWhitelistedHashes(
     const SBWhitelist& whitelist,
     const std::vector<SBFullHash>& hashes) {
+  // This method is theoretically thread-safe but document that it is currently
+  // only expected to be called on the IO thread.
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   base::AutoLock l(lookup_lock_);
   if (whitelist.second)
     return true;
@@ -867,7 +899,7 @@ void SafeBrowsingDatabaseNew::InsertAddChunk(
     SafeBrowsingStore* store,
     const safe_browsing_util::ListType list_id,
     const SBChunkData& chunk_data) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(store);
 
   // The server can give us a chunk that we already have because
@@ -898,7 +930,7 @@ void SafeBrowsingDatabaseNew::InsertSubChunk(
     SafeBrowsingStore* store,
     const safe_browsing_util::ListType list_id,
     const SBChunkData& chunk_data) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(store);
 
   // The server can give us a chunk that we already have because
@@ -933,7 +965,7 @@ void SafeBrowsingDatabaseNew::InsertSubChunk(
 void SafeBrowsingDatabaseNew::InsertChunks(
     const std::string& list_name,
     const std::vector<SBChunkData*>& chunks) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   if (corruption_detected_ || chunks.empty())
     return;
@@ -968,7 +1000,7 @@ void SafeBrowsingDatabaseNew::InsertChunks(
 
 void SafeBrowsingDatabaseNew::DeleteChunks(
     const std::vector<SBChunkDelete>& chunk_deletes) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   if (corruption_detected_ || chunk_deletes.empty())
     return;
@@ -999,9 +1031,12 @@ void SafeBrowsingDatabaseNew::CacheHashResults(
     const std::vector<SBPrefix>& prefixes,
     const std::vector<SBFullHashResult>& full_hits,
     const base::TimeDelta& cache_lifetime) {
+  // This method is theoretically thread-safe but document that it is currently
+  // only expected to be called on the IO thread.
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   const base::Time expire_after = base::Time::Now() + cache_lifetime;
 
-  // This is called on the I/O thread, lock against updates.
   base::AutoLock locked(lookup_lock_);
 
   // Create or reset all cached results for these prefixes.
@@ -1019,7 +1054,7 @@ void SafeBrowsingDatabaseNew::CacheHashResults(
 
 bool SafeBrowsingDatabaseNew::UpdateStarted(
     std::vector<SBListChunkRanges>* lists) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(lists);
 
   // If |BeginUpdate()| fails, reset the database.
@@ -1118,7 +1153,7 @@ bool SafeBrowsingDatabaseNew::UpdateStarted(
 }
 
 void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   // The update may have failed due to corrupt storage (for instance,
   // an excessive number of invalid add_chunks and sub_chunks).
@@ -1248,6 +1283,8 @@ void SafeBrowsingDatabaseNew::UpdateWhitelistStore(
     const base::FilePath& store_filename,
     SafeBrowsingStore* store,
     SBWhitelist* whitelist) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   if (!store)
     return;
 
@@ -1272,6 +1309,8 @@ int64 SafeBrowsingDatabaseNew::UpdateHashPrefixStore(
     const base::FilePath& store_filename,
     SafeBrowsingStore* store,
     FailureType failure_type) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   // These results are not used after this call. Simply ignore the
   // returned value after FinishUpdate(...).
   safe_browsing::PrefixSetBuilder builder;
@@ -1294,6 +1333,7 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
     FailureType finish_failure_type,
     FailureType write_failure_type,
     bool store_full_hashes_in_prefix_set) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(url_store);
   DCHECK(prefix_set);
 
@@ -1376,6 +1416,8 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
 }
 
 void SafeBrowsingDatabaseNew::UpdateIpBlacklistStore() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   // Note: prefixes will not be empty.  The current data store implementation
   // stores all full-length hashes as both full and prefix hashes.
   safe_browsing::PrefixSetBuilder builder;
@@ -1394,6 +1436,8 @@ void SafeBrowsingDatabaseNew::UpdateIpBlacklistStore() {
 }
 
 void SafeBrowsingDatabaseNew::HandleCorruptDatabase() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   // Reset the database after the current task has unwound (but only
   // reset once within the scope of a given task).
   if (!reset_factory_.HasWeakPtrs()) {
@@ -1405,6 +1449,8 @@ void SafeBrowsingDatabaseNew::HandleCorruptDatabase() {
 }
 
 void SafeBrowsingDatabaseNew::OnHandleCorruptDatabase() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   RecordFailure(FAILURE_DATABASE_CORRUPT_HANDLER);
   corruption_detected_ = true;  // Stop updating the database.
   ResetDatabase();
@@ -1422,10 +1468,11 @@ void SafeBrowsingDatabaseNew::LoadPrefixSet(
     const base::FilePath& db_filename,
     scoped_ptr<const safe_browsing::PrefixSet>* prefix_set,
     FailureType read_failure_type) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   if (!prefix_set)
     return;
 
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
   DCHECK(!filename_base_.empty());
 
   const base::FilePath prefix_set_filename = PrefixSetForFilename(db_filename);
@@ -1449,7 +1496,7 @@ void SafeBrowsingDatabaseNew::LoadPrefixSet(
 }
 
 bool SafeBrowsingDatabaseNew::Delete() {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!filename_base_.empty());
 
   // TODO(shess): This is a mess.  SafeBrowsingFileStore::Delete() closes the
@@ -1532,7 +1579,7 @@ void SafeBrowsingDatabaseNew::WritePrefixSet(
     const base::FilePath& db_filename,
     const safe_browsing::PrefixSet* prefix_set,
     FailureType write_failure_type) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!prefix_set)
     return;
@@ -1556,6 +1603,7 @@ void SafeBrowsingDatabaseNew::WritePrefixSet(
 }
 
 void SafeBrowsingDatabaseNew::WhitelistEverything(SBWhitelist* whitelist) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   base::AutoLock locked(lookup_lock_);
   whitelist->second = true;
   whitelist->first.clear();
@@ -1564,7 +1612,8 @@ void SafeBrowsingDatabaseNew::WhitelistEverything(SBWhitelist* whitelist) {
 void SafeBrowsingDatabaseNew::LoadWhitelist(
     const std::vector<SBAddFullHash>& full_hashes,
     SBWhitelist* whitelist) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   if (full_hashes.size() > kMaxWhitelistSize) {
     WhitelistEverything(whitelist);
     return;
@@ -1592,7 +1641,8 @@ void SafeBrowsingDatabaseNew::LoadWhitelist(
 
 void SafeBrowsingDatabaseNew::LoadIpBlacklist(
     const std::vector<SBAddFullHash>& full_hashes) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   IPBlacklist new_blacklist;
   for (std::vector<SBAddFullHash>::const_iterator it = full_hashes.begin();
        it != full_hashes.end();
@@ -1631,6 +1681,10 @@ void SafeBrowsingDatabaseNew::LoadIpBlacklist(
 }
 
 bool SafeBrowsingDatabaseNew::IsMalwareIPMatchKillSwitchOn() {
+  // This method is theoretically thread-safe but document that it is currently
+  // only expected to be called on the IO thread.
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   SBFullHash malware_kill_switch = SBFullHashForString(kMalwareIPKillSwitchUrl);
   std::vector<SBFullHash> full_hashes;
   full_hashes.push_back(malware_kill_switch);
@@ -1638,5 +1692,10 @@ bool SafeBrowsingDatabaseNew::IsMalwareIPMatchKillSwitchOn() {
 }
 
 bool SafeBrowsingDatabaseNew::IsCsdWhitelistKillSwitchOn() {
+  // This method is theoretically thread-safe but document that it is currently
+  // only expected to be called on the IO thread.
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  base::AutoLock locked(lookup_lock_);
   return csd_whitelist_.second;
 }