More explicit thread checking in SafeBrowsingDatabase.

chrome/browser/safe_browsing/safe_browsing_database.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/safe_browsing/safe_browsing_database.h"
 
 #include <algorithm>
 #include <iterator>
 
 #include "base/bind.h"
@@ skipping 414 matching lines @@
   return base::FilePath(db_filename.value() + kIPBlacklistDBFile);
 }
 
 // static
 base::FilePath SafeBrowsingDatabase::UnwantedSoftwareDBFilename(
     const base::FilePath& db_filename) {
   return base::FilePath(db_filename.value() + kUnwantedSoftwareDBFile);
 }
 
 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();
   } else if (list_id == safe_browsing_util::BINURL) {
     return download_store_.get();
   } else if (list_id == safe_browsing_util::CSDWHITELIST) {
     return csd_whitelist_store_.get();
   } else if (list_id == safe_browsing_util::DOWNLOADWHITELIST) {
     return download_whitelist_store_.get();
   } else if (list_id == safe_browsing_util::EXTENSIONBLACKLIST) {
@@ skipping 35 matching lines @@
 
 SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew(
     SafeBrowsingStore* browse_store,
     SafeBrowsingStore* download_store,
     SafeBrowsingStore* csd_whitelist_store,
     SafeBrowsingStore* download_whitelist_store,
     SafeBrowsingStore* extension_blacklist_store,
     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),
       extension_blacklist_store_(extension_blacklist_store),
       side_effect_free_whitelist_store_(side_effect_free_whitelist_store),
       ip_blacklist_store_(ip_blacklist_store),
       unwanted_software_store_(unwanted_software_store),
       corruption_detected_(false),
       change_detected_(false),
       reset_factory_(this) {
   DCHECK(browse_store_.get());
 }
 
 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());
 
   filename_base_ = filename_base;
 
   // TODO(shess): The various stores are really only necessary while doing
   // updates (see |UpdateFinished()|) or when querying a store directly (see
   // |ContainsDownloadUrl()|).
   // The store variables are also tested to see if a list is enabled.  Perhaps
@@ skipping 116 matching lines @@
     std::vector<SBAddFullHash> full_hashes;
     if (ip_blacklist_store_->GetAddFullHashes(&full_hashes)) {
       LoadIpBlacklist(full_hashes);
     } else {
       LoadIpBlacklist(std::vector<SBAddFullHash>());  // Clear the list.
     }
   }
 }
 
 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
   // reset.  Perhaps inline |Delete()|?
   if (!Delete())
     return false;
 
   // Reset objects in memory.
   {
     base::AutoLock locked(lookup_lock_);
     prefix_gethash_cache_.clear();
     browse_prefix_set_.reset();
     side_effect_free_whitelist_prefix_set_.reset();
     ip_blacklist_.clear();
     unwanted_software_prefix_set_.reset();
   }
   // Wants to acquire the lock itself.
   WhitelistEverything(&csd_whitelist_);
   WhitelistEverything(&download_whitelist_);
   return true;
 }
 
 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);
 }
 
 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);
 }
 
 bool SafeBrowsingDatabaseNew::PrefixSetContainsUrl(
     const GURL& url,
     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();
 
   std::vector<SBFullHash> full_hashes;
   UrlToFullHashes(url, false, &full_hashes);
   if (full_hashes.empty())
     return false;
 
   return PrefixSetContainsUrlHashes(
       full_hashes, prefix_set_getter, prefix_hits, cache_hits);
 }
 
 bool SafeBrowsingDatabaseNew::ContainsBrowseUrlHashesForTesting(
     const std::vector<SBFullHash>& full_hashes,
     std::vector<SBPrefix>* prefix_hits,
     std::vector<SBFullHashResult>* cache_hits) {
   return PrefixSetContainsUrlHashes(
       full_hashes, &browse_prefix_set_, prefix_hits, cache_hits);
 }
 
 bool SafeBrowsingDatabaseNew::PrefixSetContainsUrlHashes(
     const std::vector<SBFullHash>& full_hashes,
     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
   // update populates it.  Bail out without a hit if not yet available.
   // |prefix_set_getter| can only be accessed while holding |lookup_lock_| hence
   // why it is passed as a parameter rather than passing the |prefix_set|
   // directly.
   safe_browsing::PrefixSet* prefix_set = prefix_set_getter->get();
   if (!prefix_set)
     return false;
@@ skipping 11 matching lines @@
   std::sort(prefix_hits->begin(), prefix_hits->end());
   prefix_hits->erase(std::unique(prefix_hits->begin(), prefix_hits->end()),
                      prefix_hits->end());
 
   return !prefix_hits->empty() || !cache_hits->empty();
 }
 
 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())
     return false;
 
   std::vector<SBPrefix> prefixes;
   GetDownloadUrlPrefixes(urls, &prefixes);
   return MatchAddPrefixes(download_store_.get(),
                           safe_browsing_util::BINURL % 2,
                           prefixes,
                           prefix_hits);
 }
 
 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);
 }
 
 bool SafeBrowsingDatabaseNew::ContainsDownloadWhitelistedUrl(const GURL& url) {
   std::vector<SBFullHash> full_hashes;
   UrlToFullHashes(url, true, &full_hashes);
   return ContainsWhitelistedHashes(download_whitelist_, full_hashes);
 }
 
 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;
 
   return MatchAddPrefixes(extension_blacklist_store_.get(),
                           safe_browsing_util::EXTENSIONBLACKLIST % 2,
                           prefixes,
                           prefix_hits);
 }
 
 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;
   safe_browsing_util::CanonicalizeUrl(url, &host, &path, &query);
   std::string url_to_check = host + path;
   if (!query.empty())
     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
   // from disk, or the first update populates it.  Bail out without a hit if
   // not yet available.
   if (!side_effect_free_whitelist_prefix_set_.get())
     return false;
 
   return side_effect_free_whitelist_prefix_set_->Exists(full_hash);
 }
 
 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;
   if (ip_number.size() == net::kIPv4AddressSize)
     ip_number = net::ConvertIPv4NumberToIPv6Number(ip_number);
   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();
        ++it) {
     const std::string& mask = it->first;
     DCHECK_EQ(mask.size(), ip_number.size());
     std::string subnet(net::kIPv6AddressSize, '\0');
     for (size_t i = 0; i < net::kIPv6AddressSize; ++i) {
       subnet[i] = ip_number[i] & mask[i];
     }
     const std::string hash = base::SHA1HashString(subnet);
     DVLOG(2) << "Lookup Malware IP: "
              << " ip:" << ip_address
              << " mask:" << base::HexEncode(mask.data(), mask.size())
              << " subnet:" << base::HexEncode(subnet.data(), subnet.size())
              << " hash:" << base::HexEncode(hash.data(), hash.size());
     if (it->second.count(hash) > 0) {
       return true;
     }
   }
   return false;
 }
 
 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);
 }
 
 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;
   for (std::vector<SBFullHash>::const_iterator it = hashes.begin();
        it != hashes.end(); ++it) {
     if (std::binary_search(whitelist.first.begin(), whitelist.first.end(),
                            *it, SBFullHashLess)) {
       return true;
     }
   }
   return false;
 }
 
 // Helper to insert add-chunk entries.
 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
   // it's part of a range.  Don't add it again.
   const int chunk_id = chunk_data.ChunkNumber();
   const int encoded_chunk_id = EncodeChunkId(chunk_id, list_id);
   if (store->CheckAddChunk(encoded_chunk_id))
     return;
 
   store->SetAddChunk(encoded_chunk_id);
@@ skipping 10 matching lines @@
       store->WriteAddHash(encoded_chunk_id, chunk_data.FullHashAt(i));
     }
   }
 }
 
 // Helper to insert sub-chunk entries.
 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
   // it's part of a range.  Don't add it again.
   const int chunk_id = chunk_data.ChunkNumber();
   const int encoded_chunk_id = EncodeChunkId(chunk_id, list_id);
   if (store->CheckSubChunk(encoded_chunk_id))
     return;
 
   store->SetSubChunk(encoded_chunk_id);
@@ skipping 14 matching lines @@
       const int encoded_add_chunk_id = EncodeChunkId(add_chunk_id, list_id);
       store->WriteSubHash(encoded_chunk_id, encoded_add_chunk_id,
                           chunk_data.FullHashAt(i));
     }
   }
 }
 
 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;
 
   const base::TimeTicks before = base::TimeTicks::Now();
 
   // TODO(shess): The caller should just pass list_id.
   const safe_browsing_util::ListType list_id =
       safe_browsing_util::GetListId(list_name);
 
@@ skipping 14 matching lines @@
       NOTREACHED();
     }
   }
   store->FinishChunk();
 
   UMA_HISTOGRAM_TIMES("SB2.ChunkInsert", base::TimeTicks::Now() - before);
 }
 
 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;
 
   const std::string& list_name = chunk_deletes.front().list_name;
   const safe_browsing_util::ListType list_id =
       safe_browsing_util::GetListId(list_name);
 
   SafeBrowsingStore* store = GetStore(list_id);
   if (!store) return;
@@ skipping 10 matching lines @@
       else
         store->DeleteAddChunk(encoded_chunk_id);
     }
   }
 }
 
 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.
   for (size_t i = 0; i < prefixes.size(); ++i) {
     prefix_gethash_cache_[prefixes[i]] = SBCachedFullHashResult(expire_after);
   }
 
   // Insert any fullhash hits. Note that there may be one, multiple, or no
   // fullhashes for any given entry in |prefixes|.
   for (size_t i = 0; i < full_hits.size(); ++i) {
     const SBPrefix prefix = full_hits[i].hash.prefix;
     prefix_gethash_cache_[prefix].full_hashes.push_back(full_hits[i]);
   }
 }
 
 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.
   if (!browse_store_->BeginUpdate()) {
     RecordFailure(FAILURE_BROWSE_DATABASE_UPDATE_BEGIN);
     HandleCorruptDatabase();
     return false;
   }
 
   if (download_store_.get() && !download_store_->BeginUpdate()) {
@@ skipping 78 matching lines @@
   UpdateChunkRangesForList(unwanted_software_store_.get(),
                            safe_browsing_util::kUnwantedUrlList,
                            lists);
 
   corruption_detected_ = false;
   change_detected_ = false;
   return true;
 }
 
 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).
   // Double-check that the databases are valid.
   // TODO(shess): Providing a checksum for the add_chunk and sub_chunk
   // sections would allow throwing a corruption error in
   // UpdateStarted().
   if (!update_succeeded) {
     if (!browse_store_->CheckValidity())
       DLOG(ERROR) << "Safe-browsing browse database corrupt.";
@@ skipping 101 matching lines @@
                             &unwanted_software_prefix_set_,
                             FAILURE_UNWANTED_SOFTWARE_DATABASE_UPDATE_FINISH,
                             FAILURE_UNWANTED_SOFTWARE_PREFIX_SET_WRITE);
   }
 }
 
 void SafeBrowsingDatabaseNew::UpdateWhitelistStore(
     const base::FilePath& store_filename,
     SafeBrowsingStore* store,
     SBWhitelist* whitelist) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   if (!store)
     return;
 
   // Note: |builder| will not be empty.  The current data store implementation
   // stores all full-length hashes as both full and prefix hashes.
   safe_browsing::PrefixSetBuilder builder;
   std::vector<SBAddFullHash> full_hashes;
   if (!store->FinishUpdate(&builder, &full_hashes)) {
     RecordFailure(FAILURE_WHITELIST_DATABASE_UPDATE_FINISH);
     WhitelistEverything(whitelist);
     return;
   }
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(store_filename);
 #endif
 
   LoadWhitelist(full_hashes, whitelist);
 }
 
 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;
   std::vector<SBAddFullHash> add_full_hashes_result;
 
   if (!store->FinishUpdate(&builder, &add_full_hashes_result))
     RecordFailure(failure_type);
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(store_filename);
 #endif
 
   return GetFileSizeOrZero(store_filename);
 }
 
 void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
     const base::FilePath& db_filename,
     SafeBrowsingStore* url_store,
     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();
   scoped_ptr<base::ProcessMetrics> metric(
 #if !defined(OS_MACOSX)
       base::ProcessMetrics::CreateProcessMetrics(handle)
 #else
       // Getting stats only for the current process is enough, so NULL is fine.
       base::ProcessMetrics::CreateProcessMetrics(handle, NULL)
 #endif
@@ skipping 57 matching lines @@
   const int64 file_size = GetFileSizeOrZero(db_filename);
   UMA_HISTOGRAM_COUNTS("SB2.BrowseDatabaseKilobytes",
                        static_cast<int>(file_size / 1024));
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(db_filename);
 #endif
 }
 
 void SafeBrowsingDatabaseNew::UpdateSideEffectFreeWhitelistStore() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   safe_browsing::PrefixSetBuilder builder;
   std::vector<SBAddFullHash> add_full_hashes_result;
 
   if (!side_effect_free_whitelist_store_->FinishUpdate(
           &builder, &add_full_hashes_result)) {
     RecordFailure(FAILURE_SIDE_EFFECT_FREE_WHITELIST_UPDATE_FINISH);
     return;
   }
   scoped_ptr<safe_browsing::PrefixSet>
       prefix_set(builder.GetPrefixSetNoHashes());
@@ skipping 27 matching lines @@
                        static_cast<int>(file_size / 1024));
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(side_effect_free_whitelist_filename);
   base::mac::SetFileBackupExclusion(
       side_effect_free_whitelist_prefix_set_filename);
 #endif
 }
 
 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;
   std::vector<SBAddFullHash> full_hashes;
   if (!ip_blacklist_store_->FinishUpdate(&builder, &full_hashes)) {
     RecordFailure(FAILURE_IP_BLACKLIST_UPDATE_FINISH);
     LoadIpBlacklist(std::vector<SBAddFullHash>());  // Clear the list.
     return;
   }
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(IpBlacklistDBFilename(filename_base_));
 #endif
 
   LoadIpBlacklist(full_hashes);
 }
 
 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()) {
     RecordFailure(FAILURE_DATABASE_CORRUPT);
     base::MessageLoop::current()->PostTask(FROM_HERE,
         base::Bind(&SafeBrowsingDatabaseNew::OnHandleCorruptDatabase,
                    reset_factory_.GetWeakPtr()));
   }
 }
 
 void SafeBrowsingDatabaseNew::OnHandleCorruptDatabase() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
   RecordFailure(FAILURE_DATABASE_CORRUPT_HANDLER);
   corruption_detected_ = true;  // Stop updating the database.
   ResetDatabase();
 
   // NOTE(shess): ResetDatabase() should remove the corruption, so this should
   // only happen once.  If you are here because you are hitting this after a
   // restart, then I would be very interested in working with you to figure out
   // what is happening, since it may affect real users.
   DLOG(FATAL) << "SafeBrowsing database was corrupt and reset";
 }
 
 // TODO(shess): I'm not clear why this code doesn't have any
 // real error-handling.
 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);
 
   // Only use the prefix set if database is present and non-empty.
   if (!GetFileSizeOrZero(db_filename))
     return;
 
   // Cleanup any stale bloom filter (no longer used).
   // TODO(shess): Track existence to drive removal of this code?
   const base::FilePath bloom_filter_filename =
       BloomFilterForFilename(db_filename);
   base::DeleteFile(bloom_filter_filename, false);
 
   const base::TimeTicks before = base::TimeTicks::Now();
   *prefix_set = safe_browsing::PrefixSet::LoadFile(prefix_set_filename);
   UMA_HISTOGRAM_TIMES("SB2.PrefixSetLoad", base::TimeTicks::Now() - before);
 
   if (!prefix_set->get())
     RecordFailure(read_failure_type);
 }
 
 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
   // store before calling DeleteStore().  DeleteStore() deletes transient files
   // in addition to the main file.  Probably all of these should be converted to
   // a helper which calls Delete() if the store exists, else DeleteStore() on
   // the generated filename.
 
   // TODO(shess): Determine if the histograms are useful in any way.  I cannot
   // recall any action taken as a result of their values, in which case it might
@@ skipping 62 matching lines @@
   if (!r11)
     RecordFailure(FAILURE_UNWANTED_SOFTWARE_PREFIX_SET_DELETE);
 
   return r1 && r2 && r3 && r4 && r5 && r6 && r7 && r8 && r9 && r10 && r11;
 }
 
 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;
 
   const base::FilePath prefix_set_filename = PrefixSetForFilename(db_filename);
 
   const base::TimeTicks before = base::TimeTicks::Now();
   const bool write_ok = prefix_set->WriteFile(prefix_set_filename);
   UMA_HISTOGRAM_TIMES("SB2.PrefixSetWrite", base::TimeTicks::Now() - before);
 
   const int64 file_size = GetFileSizeOrZero(prefix_set_filename);
   UMA_HISTOGRAM_COUNTS("SB2.PrefixSetKilobytes",
                        static_cast<int>(file_size / 1024));
 
   if (!write_ok)
     RecordFailure(write_failure_type);
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(prefix_set_filename);
 #endif
 }
 
 void SafeBrowsingDatabaseNew::WhitelistEverything(SBWhitelist* whitelist) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   base::AutoLock locked(lookup_lock_);
   whitelist->second = true;
   whitelist->first.clear();
 }
 
 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;
   }
 
   std::vector<SBFullHash> new_whitelist;
   new_whitelist.reserve(full_hashes.size());
   for (std::vector<SBAddFullHash>::const_iterator it = full_hashes.begin();
        it != full_hashes.end(); ++it) {
     new_whitelist.push_back(it->full_hash);
   }
   std::sort(new_whitelist.begin(), new_whitelist.end(), SBFullHashLess);
 
   SBFullHash kill_switch = SBFullHashForString(kWhitelistKillSwitchUrl);
   if (std::binary_search(new_whitelist.begin(), new_whitelist.end(),
                          kill_switch, SBFullHashLess)) {
     // The kill switch is whitelisted hence we whitelist all URLs.
     WhitelistEverything(whitelist);
   } else {
     base::AutoLock locked(lookup_lock_);
     whitelist->second = false;
     whitelist->first.swap(new_whitelist);
   }
 }
 
 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();
        ++it) {
     const char* full_hash = it->full_hash.full_hash;
     DCHECK_EQ(crypto::kSHA256Length, arraysize(it->full_hash.full_hash));
     // The format of the IP blacklist is:
     // SHA-1(IPv6 prefix) + uint8(prefix size) + 11 unused bytes.
     std::string hashed_ip_prefix(full_hash, base::kSHA1Length);
     size_t prefix_size = static_cast<uint8>(full_hash[base::kSHA1Length]);
@@ skipping 18 matching lines @@
              << " hashed_ip:" << base::HexEncode(hashed_ip_prefix.data(),
                                                  hashed_ip_prefix.size());
     new_blacklist[mask].insert(hashed_ip_prefix);
   }
 
   base::AutoLock locked(lookup_lock_);
   ip_blacklist_.swap(new_blacklist);
 }
 
 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);
   return ContainsWhitelistedHashes(csd_whitelist_, full_hashes);
 }
 
 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;
 }