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 28cb1b9051e6615cc79d41105e1ebe93deb1e496..a5de866781858abc3bc049c2e7a463f3dd214ea3 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());
@@ -645,7 +647,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
@@ -672,6 +674,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/03 22:57:08]

The comments in the header say it's safe to call these methods on any thread. In
general I'm not so sure about the "these are thread-safe but dcheck for how they
are currently used" changes in this file.

[gab, 2014/12/04 19:57:27]

Ah, right, in fact I just updated all of those comments to say this (there was a
discrepancy between some saying some calls were IO thread only and some others
saying it was any threads when really all calls that properly lock are
any-thread friendly -- though doing so in practice would increase contention...
if we consider contention an issue then we should enforce IO thread only checks
via the API).

I still like the explicit checks for now as it helps document how things behave.
I do not want to put that in the API however as this is not how this class is
meant to behave.

In fact I was thinking of removing those checks once we have an explicit design
in-place which prevents developer mistakes (and thus alleviates the need to
remind developers of which methods may run on which threads -- as-is with this
CL every single method is documented with one DCHECK or another which adds to
readability in the mean time IMO, as proven by you noticing a missing lock
below!).

And even more so if we one-day think of componentizing safe_browsing (would make
sense as none of these DB's are really chrome/ specific) at which point the IO
thread would become a foreign concept.

+
   return PrefixSetContainsUrl(
       url, &browse_prefix_set_, prefix_hits, cache_hits);
 }
@@ -680,6 +686,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);
 }
@@ -689,6 +699,10 @@ bool SafeBrowsingDatabaseNew::PrefixSetContainsUrl(
     scoped_ptr<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();
@@ -715,11 +729,13 @@ bool SafeBrowsingDatabaseNew::PrefixSetContainsUrlHashes(
     scoped_ptr<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
@@ -751,7 +767,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())
@@ -766,9 +782,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);
@@ -783,7 +800,7 @@ 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;
 
@@ -795,6 +812,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;
@@ -804,7 +825,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
@@ -817,6 +837,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;
@@ -825,7 +849,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();
@@ -851,6 +874,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);
@@ -859,6 +886,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;
@@ -877,7 +908,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
@@ -908,7 +939,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
@@ -943,7 +974,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;
@@ -978,7 +1009,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;
@@ -1009,9 +1040,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.
@@ -1029,7 +1063,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.
@@ -1128,7 +1162,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).
@@ -1250,6 +1284,8 @@ void SafeBrowsingDatabaseNew::UpdateWhitelistStore(
     const base::FilePath& store_filename,
     SafeBrowsingStore* store,
     SBWhitelist* whitelist) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   if (!store)
     return;
 
@@ -1274,6 +1310,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;
@@ -1295,6 +1333,8 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
     scoped_ptr<safe_browsing::PrefixSet>* prefix_set,
     FailureType finish_failure_type,
     FailureType write_failure_type) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   // Measure the amount of IO during the filter build.
   base::IoCounters io_before, io_after;
   base::ProcessHandle handle = base::GetCurrentProcessHandle();
@@ -1372,6 +1412,8 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
 }
 
 void SafeBrowsingDatabaseNew::UpdateSideEffectFreeWhitelistStore() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   safe_browsing::PrefixSetBuilder builder;
   std::vector<SBAddFullHash> add_full_hashes_result;
 
@@ -1419,6 +1461,8 @@ void SafeBrowsingDatabaseNew::UpdateSideEffectFreeWhitelistStore() {
 }
 
 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;
@@ -1437,6 +1481,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()) {
@@ -1448,6 +1494,8 @@ void SafeBrowsingDatabaseNew::HandleCorruptDatabase() {
 }
 
 void SafeBrowsingDatabaseNew::OnHandleCorruptDatabase() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   RecordFailure(FAILURE_DATABASE_CORRUPT_HANDLER);
   corruption_detected_ = true;  // Stop updating the database.
   ResetDatabase();
@@ -1465,10 +1513,11 @@ void SafeBrowsingDatabaseNew::LoadPrefixSet(
     const base::FilePath& db_filename,
     scoped_ptr<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);
@@ -1492,7 +1541,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
@@ -1575,7 +1624,7 @@ void SafeBrowsingDatabaseNew::WritePrefixSet(
     const base::FilePath& db_filename,
     safe_browsing::PrefixSet* prefix_set,
     FailureType write_failure_type) {
-  DCHECK_EQ(creation_loop_, base::MessageLoop::current());
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!prefix_set)
     return;
@@ -1599,6 +1648,7 @@ void SafeBrowsingDatabaseNew::WritePrefixSet(
 }
 
 void SafeBrowsingDatabaseNew::WhitelistEverything(SBWhitelist* whitelist) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   base::AutoLock locked(lookup_lock_);
   whitelist->second = true;
   whitelist->first.clear();
@@ -1607,7 +1657,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;
@@ -1635,7 +1686,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();
@@ -1674,6 +1726,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);
@@ -1681,5 +1737,9 @@ 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);

[mattm, 2014/12/03 22:57:08]

Actually this function does not look safe. It should be holding lookup_lock_
before accessing csd_whitelist_. oops. Would you mind adding that while you're
here?

[gab, 2014/12/04 19:57:27]

Ah ah, indeed, this is exactly why I want to alter the design further in a
follow-up to enforce the concurrency requirements rather than rely on
developer/reviewer not making hard-to-spot mistakes.

+
   return csd_whitelist_.second;
 }