Introduce SafeBrowsingDatabase::ThreadSafeStateManager.

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 12 matching lines @@
 #include "content/public/browser/browser_thread.h"
 #include "crypto/sha2.h"
 #include "net/base/net_util.h"
 #include "url/gurl.h"
 
 #if defined(OS_MACOSX)
 #include "base/mac/mac_util.h"
 #endif
 
 using content::BrowserThread;
+using safe_browsing::PrefixSet;
+using safe_browsing::PrefixSetBuilder;
 
 namespace {
 
 // Filename suffix for the bloom filter.
 const base::FilePath::CharType kBloomFilterFile[] =
     FILE_PATH_LITERAL(" Filter 2");
 // Filename suffix for the prefix set.
 const base::FilePath::CharType kPrefixSetFile[] =
     FILE_PATH_LITERAL(" Prefix Set");
 // Filename suffix for download store.
@@ skipping 382 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) {
     return extension_blacklist_store_.get();
   } else if (list_id == safe_browsing_util::SIDEEFFECTFREEWHITELIST) {
     return side_effect_free_whitelist_store_.get();
   } else if (list_id == safe_browsing_util::IPBLACKLIST) {
     return ip_blacklist_store_.get();
   } else if (list_id == safe_browsing_util::UNWANTEDURL) {
     return unwanted_software_store_.get();
   }
   return NULL;
 }
 
 // static
 void SafeBrowsingDatabase::RecordFailure(FailureType failure_type) {
   UMA_HISTOGRAM_ENUMERATION("SB2.DatabaseFailure", failure_type,
                             FAILURE_DATABASE_MAX);
 }
 
+class SafeBrowsingDatabaseNew::ThreadSafeStateManager::ReadTransaction {
+ public:
+  enum class AutoLockRequirement {
+    LOCK,
+    // SBWhitelist's, IPBlacklist's, and PrefixSet's (not caches) are only
+    // ever written to on the main thread (as enforced by
+    // ThreadSafeStateManager) and can therefore be read on the main thread
+    // without first acquiring |lock_|.
+    DONT_LOCK_ON_MAIN_THREAD
+  };
+
+  ReadTransaction(ThreadSafeStateManager* outer,

[mattm, 2014/12/12 23:20:30]

Maybe ReadTransaction can take a const pointer to ThreadSafeStateManager to make
it even more explicit? (would need to make the lock mutable)

[gab, 2014/12/15 23:02:30]

It could but |prefix_gethash_cache_| would also have to be mutable, WDYT?

[mattm, 2014/12/23 02:09:45]

Hm, good point. It kinda seems like prefix_gethash_cache_ should actually go on
WriteTransaction, though then you get the issue of wanting things from
ReadTransaction and WriteTransaction in the same transaction.

The one in PrefixSetContainsUrlHashes doesn't seem too egregious since you could
argue that expiring old entries makes sense as sort of an internal detail that
would fit with mutable. The use of ReadTransaction in CacheHashResults feels odd
though. 

I guess this isn't a huge deal either way so you could put that as maybe
something to improve later.

[gab, 2014/12/23 21:39:54]

Agree that it's kind-of weird to write to the cache from a ReadTransaction, but
the reason it is this way right now is that WriteTransactions are special in
that they are only ever allowed on the main thread (where as the cache can be
written to from any thread).

At first this feels like it's calling for a special type of transactions for the
cache, but this would be problematic as transactions can't be nested (the lock
is not re-entrant) and we also can't use a different lock for these transactions
or we'd have to enforce locking order (as without locking order, we'd introduce
a potential for deadlocks).

(the above argument also applies to why we can't move cache writes to
WriteTransactions in the current design)

I think I still like making everything const and the cache mutable though as it
at least strongly highlights this somewhat weird design decision.
(Done.)


We could also add a special BeginCacheOnlyWriteTransaction() to alleviate weird
usage of a ReadTransaction in CacheHashResults(), but since the cache already
has to be mutable for expiring in PrefixSetContainsUrlHashes() this would, in
practice, only add logic/code without providing additional functionality.


Overall perhaps it's just the use of "Read"/"Write" nomenclature that is
confusing? i.e. it's more "AnyThread"/"MainThread", but I'm not sure that's
clearer to the average reader... Can you think of a better idea for clear
nomenclature?


Another idea (not in this CL but long term) would be to make the cache
non-thread-safe (i.e. strictly used on the IO thread) as the only reason the
main thread needs access is to clear it (which could potentially be done through
a posted task) -- that would impose a thread restriction on cache users however
(whereas currently it can be accessed from "any thread" though in practice it's
already only accessed from the IO thread).

[mattm, 2014/12/24 00:29:53]

Not coming up with anything.. I guess this is okay.
Hm, interesting. Although, the code already needs to hold the lock at the point
the cache is used in PrefixSetContainsUrlHashes, so CacheHashResults is the only
place that would really benefit. I think the current revision is fine.

+                  AutoLockRequirement auto_lock_requirement)
+      : outer_(outer) {
+    if (auto_lock_requirement == AutoLockRequirement::LOCK)
+      transaction_lock_.reset(new base::AutoLock(outer_->lock_));
+    else
+      DCHECK(outer_->thread_checker_.CalledOnValidThread());
+  }
+
+  const SBWhitelist* GetSBWhitelist(SBWhitelistId id) {
+    switch (id) {
+      case SBWhitelistId::CSD:
+        return &outer_->csd_whitelist_;
+      case SBWhitelistId::DOWNLOAD:
+        return &outer_->download_whitelist_;
+    }
+    NOTREACHED();
+    return nullptr;
+  }
+
+  const IPBlacklist* ip_blacklist() { return &outer_->ip_blacklist_; }
+
+  const PrefixSet* GetPrefixSet(PrefixSetId id) {
+    switch (id) {
+      case PrefixSetId::BROWSE:
+        return outer_->browse_prefix_set_.get();
+      case PrefixSetId::SIDE_EFFECT_FREE_WHITELIST:
+        return outer_->side_effect_free_whitelist_prefix_set_.get();
+      case PrefixSetId::UNWANTED_SOFTWARE:
+        return outer_->unwanted_software_prefix_set_.get();
+    }
+    NOTREACHED();
+    return nullptr;
+  }
+
+  PrefixGetHashCache* prefix_gethash_cache() {
+    // The cache is special: it is read/write on all threads. Access to it
+    // therefore requires a LOCK'ed transaction (i.e. it can't benefit from
+    // DONT_LOCK_ON_MAIN_THREAD).
+    DCHECK(transaction_lock_);
+    return &outer_->prefix_gethash_cache_;
+  }
+
+ private:
+  ThreadSafeStateManager* outer_;
+  scoped_ptr<base::AutoLock> transaction_lock_;
+
+  DISALLOW_COPY_AND_ASSIGN(ReadTransaction);
+};
+
+class SafeBrowsingDatabaseNew::ThreadSafeStateManager::WriteTransaction {
+ public:
+  explicit WriteTransaction(ThreadSafeStateManager* outer)
+      : outer_(outer), transaction_lock_(outer_->lock_) {
+    DCHECK(outer_->thread_checker_.CalledOnValidThread());
+  }
+
+  SBWhitelist* GetWritableSBWhitelist(SBWhitelistId id) {
+    switch (id) {
+      case SBWhitelistId::CSD:
+        return &outer_->csd_whitelist_;
+      case SBWhitelistId::DOWNLOAD:
+        return &outer_->download_whitelist_;
+    }
+    NOTREACHED();
+    return nullptr;
+  }
+
+  IPBlacklist* writable_ip_blacklist() { return &outer_->ip_blacklist_; }
+
+  void SwapPrefixSet(PrefixSetId id,
+                     scoped_ptr<const PrefixSet> new_prefix_set) {
+    switch (id) {
+      case PrefixSetId::BROWSE:
+        outer_->browse_prefix_set_.swap(new_prefix_set);
+        break;
+      case PrefixSetId::SIDE_EFFECT_FREE_WHITELIST:
+        outer_->side_effect_free_whitelist_prefix_set_.swap(new_prefix_set);
+        break;
+      case PrefixSetId::UNWANTED_SOFTWARE:
+        outer_->unwanted_software_prefix_set_.swap(new_prefix_set);
+        break;
+    }
+  }
+
+  void clear_prefix_gethash_cache() { outer_->prefix_gethash_cache_.clear(); }
+
+ private:
+  ThreadSafeStateManager* outer_;
+  base::AutoLock transaction_lock_;
+
+  DISALLOW_COPY_AND_ASSIGN(WriteTransaction);
+};
+
+SafeBrowsingDatabaseNew::ThreadSafeStateManager::ThreadSafeStateManager() {
+}
+SafeBrowsingDatabaseNew::ThreadSafeStateManager::~ThreadSafeStateManager() {
+}
+
+scoped_ptr<SafeBrowsingDatabaseNew::ReadTransaction>
+SafeBrowsingDatabaseNew::ThreadSafeStateManager::BeginReadTransaction() {
+  return make_scoped_ptr(
+      new ReadTransaction(this, ReadTransaction::AutoLockRequirement::LOCK));
+}
+
+scoped_ptr<SafeBrowsingDatabaseNew::ReadTransaction> SafeBrowsingDatabaseNew::
+    ThreadSafeStateManager::BeginReadTransactionNoLockOnMainThread() {
+  return make_scoped_ptr(new ReadTransaction(
+      this, ReadTransaction::AutoLockRequirement::DONT_LOCK_ON_MAIN_THREAD));
+}
+
+scoped_ptr<SafeBrowsingDatabaseNew::WriteTransaction>
+SafeBrowsingDatabaseNew::ThreadSafeStateManager::BeginWriteTransaction() {
+  return make_scoped_ptr(new WriteTransaction(this));
+}
+
 SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew()
     : SafeBrowsingDatabaseNew(new SafeBrowsingStoreFile,
                               NULL,
                               NULL,
                               NULL,
                               NULL,
                               NULL,
                               NULL,
                               NULL) {
   DCHECK(browse_store_.get());
@@ skipping 23 matching lines @@
       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(thread_checker_.CalledOnValidThread());

[gab, 2014/12/11 19:39:20]

This DCHECK is still covered by making SafeBrowsingStore NonThreadSafe in
https://codereview.chromium.org/744183002/ (although it currently has to be
disabled for the same reason as this one).

 }
 
 void SafeBrowsingDatabaseNew::Init(const base::FilePath& filename_base) {
-  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
   // the stores could be refactored into an update object so that they are only
   // live in memory while being actively used.  The sense of enabled probably
   // belongs in protocol_manager or database_manager.
 
-  browse_store_->Init(
-      BrowseDBFilename(filename_base_),
-      base::Bind(&SafeBrowsingDatabaseNew::HandleCorruptDatabase,
-                 base::Unretained(this)));
+  {
+    // NOTE: A transaction here is overkill as there are no pointers to this
+    // class on other threads until this function returns, but it's also
+    // harmless as that also means there is no possibility of contention on the
+    // lock.
+    scoped_ptr<WriteTransaction> txn = state_manager_.BeginWriteTransaction();
 
-  if (unwanted_software_store_.get()) {
-    unwanted_software_store_->Init(
-        UnwantedSoftwareDBFilename(filename_base_),
+    txn->clear_prefix_gethash_cache();
+
+    browse_store_->Init(
+        BrowseDBFilename(filename_base_),
         base::Bind(&SafeBrowsingDatabaseNew::HandleCorruptDatabase,
                    base::Unretained(this)));
-  }
 
-  {
-    // NOTE: There is no need to grab the lock in this function, since
-    // until it returns, there are no pointers to this class on other
-    // threads.  Then again, that means there is no possibility of
-    // contention on the lock...
-    base::AutoLock locked(lookup_lock_);
-    prefix_gethash_cache_.clear();
-    LoadPrefixSet(BrowseDBFilename(filename_base_),
-                  &browse_prefix_set_,
-                  FAILURE_BROWSE_PREFIX_SET_READ);
     if (unwanted_software_store_.get()) {
-      LoadPrefixSet(UnwantedSoftwareDBFilename(filename_base_),
-                    &unwanted_software_prefix_set_,
+      unwanted_software_store_->Init(
+          UnwantedSoftwareDBFilename(filename_base_),
+          base::Bind(&SafeBrowsingDatabaseNew::HandleCorruptDatabase,
+                     base::Unretained(this)));
+    }
+    LoadPrefixSet(BrowseDBFilename(filename_base_), txn.get(),
+                  PrefixSetId::BROWSE, FAILURE_BROWSE_PREFIX_SET_READ);
+    if (unwanted_software_store_.get()) {
+      LoadPrefixSet(UnwantedSoftwareDBFilename(filename_base_), txn.get(),
+                    PrefixSetId::UNWANTED_SOFTWARE,
                     FAILURE_UNWANTED_SOFTWARE_PREFIX_SET_READ);
     }
+
+    if (side_effect_free_whitelist_store_.get()) {
+      const base::FilePath side_effect_free_whitelist_filename =
+          SideEffectFreeWhitelistDBFilename(filename_base_);
+      side_effect_free_whitelist_store_->Init(
+          side_effect_free_whitelist_filename,
+          base::Bind(&SafeBrowsingDatabaseNew::HandleCorruptDatabase,
+                     base::Unretained(this)));
+
+      LoadPrefixSet(side_effect_free_whitelist_filename, txn.get(),
+                    PrefixSetId::SIDE_EFFECT_FREE_WHITELIST,
+                    FAILURE_SIDE_EFFECT_FREE_WHITELIST_PREFIX_SET_READ);
+    } else {
+      // Delete any files of the side-effect free sidelist that may be around
+      // from when it was previously enabled.
+      SafeBrowsingStoreFile::DeleteStore(
+          SideEffectFreeWhitelistDBFilename(filename_base_));
+      base::DeleteFile(PrefixSetForFilename(
+                           SideEffectFreeWhitelistDBFilename(filename_base_)),
+                       false);
+    }
   }
+  // Note: End the transaction early because LoadWhiteList() and
+  // WhitelistEverything() manage their own transactions.
 
   if (download_store_.get()) {
     download_store_->Init(
         DownloadDBFilename(filename_base_),
         base::Bind(&SafeBrowsingDatabaseNew::HandleCorruptDatabase,
                    base::Unretained(this)));
   }
 
   if (csd_whitelist_store_.get()) {
     csd_whitelist_store_->Init(
         CsdWhitelistDBFilename(filename_base_),
         base::Bind(&SafeBrowsingDatabaseNew::HandleCorruptDatabase,
                    base::Unretained(this)));
 
     std::vector<SBAddFullHash> full_hashes;
     if (csd_whitelist_store_->GetAddFullHashes(&full_hashes)) {
-      LoadWhitelist(full_hashes, &csd_whitelist_);
+      LoadWhitelist(full_hashes, SBWhitelistId::CSD);
     } else {
-      WhitelistEverything(&csd_whitelist_);
+      WhitelistEverything(SBWhitelistId::CSD);
     }
   } else {
-    WhitelistEverything(&csd_whitelist_);  // Just to be safe.
+    WhitelistEverything(SBWhitelistId::CSD);  // Just to be safe.
   }
 
   if (download_whitelist_store_.get()) {
     download_whitelist_store_->Init(
         DownloadWhitelistDBFilename(filename_base_),
         base::Bind(&SafeBrowsingDatabaseNew::HandleCorruptDatabase,
                    base::Unretained(this)));
 
     std::vector<SBAddFullHash> full_hashes;
     if (download_whitelist_store_->GetAddFullHashes(&full_hashes)) {
-      LoadWhitelist(full_hashes, &download_whitelist_);
+      LoadWhitelist(full_hashes, SBWhitelistId::DOWNLOAD);
     } else {
-      WhitelistEverything(&download_whitelist_);
+      WhitelistEverything(SBWhitelistId::DOWNLOAD);
     }
   } else {
-    WhitelistEverything(&download_whitelist_);  // Just to be safe.
+    WhitelistEverything(SBWhitelistId::DOWNLOAD);  // Just to be safe.
   }
 
   if (extension_blacklist_store_.get()) {
     extension_blacklist_store_->Init(
         ExtensionBlacklistDBFilename(filename_base_),
         base::Bind(&SafeBrowsingDatabaseNew::HandleCorruptDatabase,
                    base::Unretained(this)));
   }
 
-  if (side_effect_free_whitelist_store_.get()) {
-    const base::FilePath side_effect_free_whitelist_filename =
-        SideEffectFreeWhitelistDBFilename(filename_base_);
-    side_effect_free_whitelist_store_->Init(
-        side_effect_free_whitelist_filename,
-        base::Bind(&SafeBrowsingDatabaseNew::HandleCorruptDatabase,
-                   base::Unretained(this)));
-
-    LoadPrefixSet(side_effect_free_whitelist_filename,
-                  &side_effect_free_whitelist_prefix_set_,
-                  FAILURE_SIDE_EFFECT_FREE_WHITELIST_PREFIX_SET_READ);
-  } else {
-    // Delete any files of the side-effect free sidelist that may be around
-    // from when it was previously enabled.
-    SafeBrowsingStoreFile::DeleteStore(
-        SideEffectFreeWhitelistDBFilename(filename_base_));
-    base::DeleteFile(
-        PrefixSetForFilename(SideEffectFreeWhitelistDBFilename(filename_base_)),
-        false);
-  }
-
   if (ip_blacklist_store_.get()) {
     ip_blacklist_store_->Init(
         IpBlacklistDBFilename(filename_base_),
         base::Bind(&SafeBrowsingDatabaseNew::HandleCorruptDatabase,
                    base::Unretained(this)));
 
     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(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();
+    scoped_ptr<WriteTransaction> txn = state_manager_.BeginWriteTransaction();
+    txn->clear_prefix_gethash_cache();
+    txn->SwapPrefixSet(PrefixSetId::BROWSE, nullptr);
+    txn->SwapPrefixSet(PrefixSetId::SIDE_EFFECT_FREE_WHITELIST, nullptr);
+    txn->SwapPrefixSet(PrefixSetId::UNWANTED_SOFTWARE, nullptr);
+    txn->writable_ip_blacklist()->clear();
   }
   // Wants to acquire the lock itself.

[mattm, 2014/12/12 23:20:31]

Why not make WhitelistEverything a method on WriteTransation?

[gab, 2014/12/15 23:02:30]

I debated that in lengths while writing this and here was my initial conclusion
(actually what I'd precisely debated was having WhitelistEverything take a
WriteTransaction* -- in which case your suggestion of making it a method of
WriteTransaction is potentially even nicer -- but the reasoning remains the
same):


Doing so means that any caller of WhitelistEverything needs a transaction. This
makes the specific call-site highlighted by this comment much cleaner
off-course, but it makes some other call-sites very ugly unless we also make
LoadWhitelist() take a WriteTransaction* (rather than obtain one for itself).

We however can't make LoadWhitelist() take a transaction as it does some
computation we'd rather do prior to holding the lock (i.e. it only needs the
lock for the final in-place swap).

One option I considered was having some sort of
WriteTransaction::TemporaryRelease() method which would return a
scoped_ptr<base::AutoUnlock> to allow unlocking in the middle of a transaction
to perform some work, but I felt this was overly complicated and could be
misused and instead settled for the current implementation of
WhitelistEverything()/LoadWhitelist() -- which, FWIW, locks just as much as the
previous implementation did so it's at least not worse and we could make
trickier improvements in smaller follow-up changes if desired.


Slight tangent: I made LoadPrefixSet() take a WriteTransaction* but it's
different as it's only called during Init() so it doesn't matter whereas
LoadWhitelist() is called on update as well (I stressed this new requirement in
the updated documentation for LoadPrefixSet() in this CL).

[mattm, 2014/12/23 02:09:45]

Well, you could one-line it like
state_manager_.BeginWriteTransaction()->WhitelistEverything(whitelist_id);
which is only sort-of ugly. But I get your point.
 

[gab, 2014/12/23 21:39:54]

Actually, I kind of like that and used it for a few other things which allows a
little more inlining in WriteTransaction (i.e. no longer need to expose raw
non-const pointers of ThreadSafeStateManager's members -- apart from the cache).

-  WhitelistEverything(&csd_whitelist_);
-  WhitelistEverything(&download_whitelist_);
+  WhitelistEverything(SBWhitelistId::CSD);
+  WhitelistEverything(SBWhitelistId::DOWNLOAD);
   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);
-
-  return PrefixSetContainsUrl(
-      url, &browse_prefix_set_, prefix_hits, cache_hits);
+  return PrefixSetContainsUrl(url, PrefixSetId::BROWSE, 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);
+  return PrefixSetContainsUrl(url, PrefixSetId::UNWANTED_SOFTWARE, prefix_hits,
+                              cache_hits);
 }
 
 bool SafeBrowsingDatabaseNew::PrefixSetContainsUrl(
     const GURL& url,
-    scoped_ptr<const safe_browsing::PrefixSet>* prefix_set_getter,
+    PrefixSetId prefix_set_id,
     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);
+  return PrefixSetContainsUrlHashes(full_hashes, prefix_set_id, 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);
+  return PrefixSetContainsUrlHashes(full_hashes, PrefixSetId::BROWSE,
+                                    prefix_hits, cache_hits);
 }
 
 bool SafeBrowsingDatabaseNew::PrefixSetContainsUrlHashes(
     const std::vector<SBFullHash>& full_hashes,
-    scoped_ptr<const safe_browsing::PrefixSet>* prefix_set_getter,
+    PrefixSetId prefix_set_id,
     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();
 
-  base::AutoLock locked(lookup_lock_);
+  {
+    scoped_ptr<ReadTransaction> txn = state_manager_.BeginReadTransaction();
 
-  // |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.
-  const safe_browsing::PrefixSet* prefix_set = prefix_set_getter->get();
-  if (!prefix_set)
-    return false;
+    // |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.
+    const PrefixSet* prefix_set = txn->GetPrefixSet(prefix_set_id);
+    if (!prefix_set)
+      return false;
 
-  for (size_t i = 0; i < full_hashes.size(); ++i) {
-    if (!GetCachedFullHash(
-            &prefix_gethash_cache_, full_hashes[i], now, cache_hits)) {
-      // No valid cached result, check the database.
-      if (prefix_set->Exists(full_hashes[i]))
-        prefix_hits->push_back(full_hashes[i].prefix);
+    for (size_t i = 0; i < full_hashes.size(); ++i) {
+      if (!GetCachedFullHash(txn->prefix_gethash_cache(), full_hashes[i], now,
+                             cache_hits)) {
+        // No valid cached result, check the database.
+        if (prefix_set->Exists(full_hashes[i]))
+          prefix_hits->push_back(full_hashes[i].prefix);
+      }
     }
   }
 
   // Multiple full hashes could share prefix, remove duplicates.
   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(thread_checker_.CalledOnValidThread());

[mattm, 2014/12/12 23:20:31]

Why are all these removed?

[gab, 2014/12/15 23:02:30]

Because |thread_checker_| moved to ThreadSafeStateManager which now
automatically enforces this where required.

And for SafeBrowsingStore accesses we're also safe as it was made NonThreadSafe
in https://codereview.chromium.org/744183002/.

I just realized however that this doesn't cover the two bool members of this
class.

I'll add them back, plus they're nice for documentation purposes.

-
   // 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 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);
+  return ContainsWhitelistedHashes(SBWhitelistId::CSD, full_hashes);
 }
 
 bool SafeBrowsingDatabaseNew::ContainsDownloadWhitelistedUrl(const GURL& url) {
   std::vector<SBFullHash> full_hashes;
   UrlToFullHashes(url, true, &full_hashes);
-  return ContainsWhitelistedHashes(download_whitelist_, full_hashes);
+  return ContainsWhitelistedHashes(SBWhitelistId::DOWNLOAD, full_hashes);
 }
 
 bool SafeBrowsingDatabaseNew::ContainsExtensionPrefixes(
     const std::vector<SBPrefix>& prefixes,
     std::vector<SBPrefix>* prefix_hits) {
-  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);
 
-  base::AutoLock locked(lookup_lock_);
+  scoped_ptr<ReadTransaction> txn = state_manager_.BeginReadTransaction();
+
+  const PrefixSet* side_effect_free_whitelist_prefix_set =
+      txn->GetPrefixSet(PrefixSetId::SIDE_EFFECT_FREE_WHITELIST);
 
   // |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())
+  if (!side_effect_free_whitelist_prefix_set)
     return false;
 
-  return side_effect_free_whitelist_prefix_set_->Exists(full_hash);
+  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.
 
-  base::AutoLock locked(lookup_lock_);
-  for (IPBlacklist::const_iterator it = ip_blacklist_.begin();
-       it != ip_blacklist_.end();
-       ++it) {
+  scoped_ptr<ReadTransaction> txn = state_manager_.BeginReadTransaction();
+  const IPBlacklist* ip_blacklist = txn->ip_blacklist();
+  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);
+  return ContainsWhitelistedHashes(SBWhitelistId::DOWNLOAD, hashes);
 }
 
 bool SafeBrowsingDatabaseNew::ContainsWhitelistedHashes(
-    const SBWhitelist& whitelist,
+    SBWhitelistId whitelist_id,
     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)
+  scoped_ptr<ReadTransaction> txn = state_manager_.BeginReadTransaction();
+  const SBWhitelist* whitelist = txn->GetSBWhitelist(whitelist_id);
+  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(),
+    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(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(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(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);
 
   SafeBrowsingStore* store = GetStore(list_id);
@@ skipping 13 matching lines @@
       NOTREACHED();
     }
   }
   store->FinishChunk();
 
   UMA_HISTOGRAM_TIMES("SB2.ChunkInsert", base::TimeTicks::Now() - before);
 }
 
 void SafeBrowsingDatabaseNew::DeleteChunks(
     const std::vector<SBChunkDelete>& chunk_deletes) {
-  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;
 
   change_detected_ = true;
 
   for (size_t i = 0; i < chunk_deletes.size(); ++i) {
     std::vector<int> chunk_numbers;
     RangesToChunks(chunk_deletes[i].chunk_del, &chunk_numbers);
     for (size_t j = 0; j < chunk_numbers.size(); ++j) {
       const int encoded_chunk_id = EncodeChunkId(chunk_numbers[j], list_id);
       if (chunk_deletes[i].is_sub_del)
         store->DeleteSubChunk(encoded_chunk_id);
       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;
 
-  base::AutoLock locked(lookup_lock_);
+  scoped_ptr<ReadTransaction> txn = state_manager_.BeginReadTransaction();
+  PrefixGetHashCache* prefix_gethash_cache = txn->prefix_gethash_cache();
 
   // 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);
+    (*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]);
+    (*prefix_gethash_cache)[prefix].full_hashes.push_back(full_hits[i]);
   }
 }
 
 bool SafeBrowsingDatabaseNew::UpdateStarted(
     std::vector<SBListChunkRanges>* lists) {
-  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 35 matching lines @@
     return false;
   }
 
   if (unwanted_software_store_ && !unwanted_software_store_->BeginUpdate()) {
     RecordFailure(FAILURE_UNWANTED_SOFTWARE_DATABASE_UPDATE_BEGIN);
     HandleCorruptDatabase();
     return false;
   }
 
   {
-    base::AutoLock locked(lookup_lock_);
+    scoped_ptr<WriteTransaction> txn = state_manager_.BeginWriteTransaction();
     // Cached fullhash results must be cleared on every database update (whether
-    // successful or not.)
-    prefix_gethash_cache_.clear();
+    // successful or not).
+    txn->clear_prefix_gethash_cache();
   }
 
   UpdateChunkRangesForLists(browse_store_.get(),
                             safe_browsing_util::kMalwareList,
                             safe_browsing_util::kPhishingList,
                             lists);
 
   // NOTE(shess): |download_store_| used to contain kBinHashList, which has been
   // deprecated.  Code to delete the list from the store shows ~15k hits/day as
   // of Feb 2014, so it has been removed.  Everything _should_ be resilient to
@@ skipping 19 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(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 61 matching lines @@
     int64 size_bytes = UpdateHashPrefixStore(
         DownloadDBFilename(filename_base_),
         download_store_.get(),
         FAILURE_DOWNLOAD_DATABASE_UPDATE_FINISH);
     UMA_HISTOGRAM_COUNTS("SB2.DownloadDatabaseKilobytes",
                          static_cast<int>(size_bytes / 1024));
   }
 
   UpdatePrefixSetUrlStore(BrowseDBFilename(filename_base_),
                           browse_store_.get(),
-                          &browse_prefix_set_,
+                          PrefixSetId::BROWSE,
                           FAILURE_BROWSE_DATABASE_UPDATE_FINISH,
                           FAILURE_BROWSE_PREFIX_SET_WRITE,
                           true);
 
   UpdateWhitelistStore(CsdWhitelistDBFilename(filename_base_),
                        csd_whitelist_store_.get(),
-                       &csd_whitelist_);
+                       SBWhitelistId::CSD);
   UpdateWhitelistStore(DownloadWhitelistDBFilename(filename_base_),
                        download_whitelist_store_.get(),
-                       &download_whitelist_);
+                       SBWhitelistId::DOWNLOAD);
 
   if (extension_blacklist_store_) {
     int64 size_bytes = UpdateHashPrefixStore(
         ExtensionBlacklistDBFilename(filename_base_),
         extension_blacklist_store_.get(),
         FAILURE_EXTENSION_BLACKLIST_UPDATE_FINISH);
     UMA_HISTOGRAM_COUNTS("SB2.ExtensionBlacklistKilobytes",
                          static_cast<int>(size_bytes / 1024));
   }
 
   if (side_effect_free_whitelist_store_) {
     UpdatePrefixSetUrlStore(SideEffectFreeWhitelistDBFilename(filename_base_),
                             side_effect_free_whitelist_store_.get(),
-                            &side_effect_free_whitelist_prefix_set_,
+                            PrefixSetId::SIDE_EFFECT_FREE_WHITELIST,
                             FAILURE_SIDE_EFFECT_FREE_WHITELIST_UPDATE_FINISH,
                             FAILURE_SIDE_EFFECT_FREE_WHITELIST_PREFIX_SET_WRITE,
                             false);
   }
 
   if (ip_blacklist_store_)
     UpdateIpBlacklistStore();
 
   if (unwanted_software_store_) {
     UpdatePrefixSetUrlStore(UnwantedSoftwareDBFilename(filename_base_),
                             unwanted_software_store_.get(),
-                            &unwanted_software_prefix_set_,
+                            PrefixSetId::UNWANTED_SOFTWARE,
                             FAILURE_UNWANTED_SOFTWARE_DATABASE_UPDATE_FINISH,
                             FAILURE_UNWANTED_SOFTWARE_PREFIX_SET_WRITE,
                             true);
   }
 }
 
 void SafeBrowsingDatabaseNew::UpdateWhitelistStore(
     const base::FilePath& store_filename,
     SafeBrowsingStore* store,
-    SBWhitelist* whitelist) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
+    SBWhitelistId whitelist_id) {
   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;
+  PrefixSetBuilder builder;
   std::vector<SBAddFullHash> full_hashes;
   if (!store->FinishUpdate(&builder, &full_hashes)) {
     RecordFailure(FAILURE_WHITELIST_DATABASE_UPDATE_FINISH);
-    WhitelistEverything(whitelist);
+    WhitelistEverything(whitelist_id);
     return;
   }
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(store_filename);
 #endif
 
-  LoadWhitelist(full_hashes, whitelist);
+  LoadWhitelist(full_hashes, whitelist_id);
 }
 
 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;
+  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<const safe_browsing::PrefixSet>* prefix_set,
+    PrefixSetId prefix_set_id,
     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);
 
   // 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
   );
 
   // IoCounters are currently not supported on Mac, and may not be
   // available for Linux, so we check the result and only show IO
   // stats if they are available.
   const bool got_counters = metric->GetIOCounters(&io_before);
 
   const base::TimeTicks before = base::TimeTicks::Now();
 
   // TODO(shess): Perhaps refactor to let builder accumulate full hashes on the
   // fly?  Other clients use the SBAddFullHash vector, but AFAICT they only use
   // the SBFullHash portion.  It would need an accessor on PrefixSet.
-  safe_browsing::PrefixSetBuilder builder;
+  PrefixSetBuilder builder;
   std::vector<SBAddFullHash> add_full_hashes;
   if (!url_store->FinishUpdate(&builder, &add_full_hashes)) {
     RecordFailure(finish_failure_type);
     return;
   }
 
-  scoped_ptr<const safe_browsing::PrefixSet> new_prefix_set;
+  scoped_ptr<const PrefixSet> new_prefix_set;
   if (store_full_hashes_in_prefix_set) {
     std::vector<SBFullHash> full_hash_results;
     for (size_t i = 0; i < add_full_hashes.size(); ++i) {
       full_hash_results.push_back(add_full_hashes[i].full_hash);
     }
 
     new_prefix_set = builder.GetPrefixSet(full_hash_results);
   } else {
     new_prefix_set = builder.GetPrefixSetNoHashes();
   }
 
   // Swap in the newly built filter.
   {
-    base::AutoLock locked(lookup_lock_);
-    prefix_set->swap(new_prefix_set);
+    scoped_ptr<WriteTransaction> txn = state_manager_.BeginWriteTransaction();
+    txn->SwapPrefixSet(prefix_set_id, new_prefix_set.Pass());
   }
 
   UMA_HISTOGRAM_LONG_TIMES("SB2.BuildFilter", base::TimeTicks::Now() - before);
 
-  // Persist the prefix set to disk. Note: there is no need to lock since the
-  // only write to |*prefix_set| is on this thread (in the swap() above).
-  WritePrefixSet(db_filename, prefix_set->get(), write_failure_type);
+  {
+    // Persist the prefix set to disk. Do not grab the lock to avoid contention
+    // while writing to disk. This is safe as only this thread can ever modify
+    // |state_manager_|'s prefix sets anyways.
+    scoped_ptr<ReadTransaction> txn =
+        state_manager_.BeginReadTransactionNoLockOnMainThread();
+    WritePrefixSet(db_filename, txn->GetPrefixSet(prefix_set_id),
+                   write_failure_type);
+  }
 
   // Gather statistics.
   if (got_counters && metric->GetIOCounters(&io_after)) {
     UMA_HISTOGRAM_COUNTS("SB2.BuildReadKilobytes",
                          static_cast<int>(io_after.ReadTransferCount -
                                           io_before.ReadTransferCount) / 1024);
     UMA_HISTOGRAM_COUNTS("SB2.BuildWriteKilobytes",
                          static_cast<int>(io_after.WriteTransferCount -
                                           io_before.WriteTransferCount) / 1024);
     UMA_HISTOGRAM_COUNTS("SB2.BuildReadOperations",
                          static_cast<int>(io_after.ReadOperationCount -
                                           io_before.ReadOperationCount));
     UMA_HISTOGRAM_COUNTS("SB2.BuildWriteOperations",
                          static_cast<int>(io_after.WriteOperationCount -
                                           io_before.WriteOperationCount));
   }
 
   const int64 file_size = GetFileSizeOrZero(db_filename);
   UMA_HISTOGRAM_COUNTS("SB2.DatabaseKilobytes",
                        static_cast<int>(file_size / 1024));
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(db_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;
+  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<const safe_browsing::PrefixSet>* prefix_set,
-    FailureType read_failure_type) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
-  if (!prefix_set)
-    return;
-
+void SafeBrowsingDatabaseNew::LoadPrefixSet(const base::FilePath& db_filename,
+                                            WriteTransaction* txn,
+                                            PrefixSetId prefix_set_id,
+                                            FailureType read_failure_type) {
+  DCHECK(txn);
   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);
+  scoped_ptr<const PrefixSet> new_prefix_set =
+      PrefixSet::LoadFile(PrefixSetForFilename(db_filename));
+  if (!new_prefix_set.get())
+    RecordFailure(read_failure_type);
+  txn->SwapPrefixSet(prefix_set_id, new_prefix_set.Pass());
   UMA_HISTOGRAM_TIMES("SB2.PrefixSetLoad", base::TimeTicks::Now() - before);
-
-  if (!prefix_set->get())
-    RecordFailure(read_failure_type);
 }
 
 bool SafeBrowsingDatabaseNew::Delete() {
-  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 58 matching lines @@
     RecordFailure(FAILURE_IP_BLACKLIST_DELETE);
 
   const bool r11 =
       base::DeleteFile(UnwantedSoftwareDBFilename(filename_base_), false);
   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,
-    const safe_browsing::PrefixSet* prefix_set,
-    FailureType write_failure_type) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
+void SafeBrowsingDatabaseNew::WritePrefixSet(const base::FilePath& db_filename,
+                                             const PrefixSet* prefix_set,
+                                             FailureType write_failure_type) {
   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_);
+void SafeBrowsingDatabaseNew::WhitelistEverything(SBWhitelistId whitelist_id) {
+  scoped_ptr<WriteTransaction> txn = state_manager_.BeginWriteTransaction();
+  SBWhitelist* whitelist = txn->GetWritableSBWhitelist(whitelist_id);
   whitelist->second = true;
   whitelist->first.clear();
 }
 
 void SafeBrowsingDatabaseNew::LoadWhitelist(
     const std::vector<SBAddFullHash>& full_hashes,
-    SBWhitelist* whitelist) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
+    SBWhitelistId whitelist_id) {
   if (full_hashes.size() > kMaxWhitelistSize) {
-    WhitelistEverything(whitelist);
+    WhitelistEverything(whitelist_id);
     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);
+    WhitelistEverything(whitelist_id);
   } else {
-    base::AutoLock locked(lookup_lock_);
+    scoped_ptr<WriteTransaction> txn = state_manager_.BeginWriteTransaction();
+    SBWhitelist* whitelist = txn->GetWritableSBWhitelist(whitelist_id);
     whitelist->second = false;
     whitelist->first.swap(new_whitelist);
   }
 }
 
 void SafeBrowsingDatabaseNew::LoadIpBlacklist(
     const std::vector<SBAddFullHash>& full_hashes) {
-  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 13 matching lines @@
     }
     DVLOG(2) << "Inserting malicious IP: "
              << " raw:" << base::HexEncode(full_hash, crypto::kSHA256Length)
              << " mask:" << base::HexEncode(mask.data(), mask.size())
              << " prefix_size:" << prefix_size
              << " 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);
+  scoped_ptr<WriteTransaction> txn = state_manager_.BeginWriteTransaction();
+  txn->writable_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);
+  return ContainsWhitelistedHashes(SBWhitelistId::CSD, 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);
+  scoped_ptr<ReadTransaction> txn = state_manager_.BeginReadTransaction();
+  return txn->GetSBWhitelist(SBWhitelistId::CSD)->second;
+}
 
-  base::AutoLock locked(lookup_lock_);
-  return csd_whitelist_.second;
+SafeBrowsingDatabaseNew::PrefixGetHashCache*
+SafeBrowsingDatabaseNew::GetUnsynchronizedPrefixGetHashCacheForTesting() {
+  scoped_ptr<ReadTransaction> txn = state_manager_.BeginReadTransaction();
+  return txn->prefix_gethash_cache();
 }