Introduce SafeBrowsingDatabase::ThreadSafeStateManager.

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 2db37e202634ca6b36486947f81c46872ab102c7..f81d6f24eb0c4e11ca1c0dbed54304b8f1a606ee 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -30,6 +30,8 @@
 #endif
 
 using content::BrowserThread;
+using safe_browsing::PrefixSet;
+using safe_browsing::PrefixSetBuilder;
 
 namespace {
 
@@ -432,9 +434,6 @@ 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();
@@ -462,6 +461,133 @@ void SafeBrowsingDatabase::RecordFailure(FailureType 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,
@@ -505,13 +631,9 @@ SafeBrowsingDatabaseNew::SafeBrowsingDatabaseNew(
 }
 
 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());
 
@@ -525,34 +647,57 @@ void SafeBrowsingDatabaseNew::Init(const base::FilePath& filename_base) {
   // 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();
+
+    txn->clear_prefix_gethash_cache();
 
-  if (unwanted_software_store_.get()) {
-    unwanted_software_store_->Init(
-        UnwantedSoftwareDBFilename(filename_base_),
+    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(
@@ -569,12 +714,12 @@ void SafeBrowsingDatabaseNew::Init(const base::FilePath& filename_base) {
 
     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()) {
@@ -585,12 +730,12 @@ void SafeBrowsingDatabaseNew::Init(const base::FilePath& filename_base) {
 
     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()) {
@@ -600,27 +745,6 @@ void SafeBrowsingDatabaseNew::Init(const base::FilePath& filename_base) {
                    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_),
@@ -637,8 +761,6 @@ void SafeBrowsingDatabaseNew::Init(const base::FilePath& filename_base) {
 }
 
 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()|?
@@ -647,16 +769,16 @@ bool SafeBrowsingDatabaseNew::ResetDatabase() {
 
   // 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;
 }
 
@@ -664,35 +786,23 @@ 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();
@@ -702,47 +812,42 @@ bool SafeBrowsingDatabaseNew::PrefixSetContainsUrl(
   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_);
-
-  // |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;
-
-  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);
+  {
+    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.
+    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(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);
+      }
     }
   }
 
@@ -757,8 +862,6 @@ bool SafeBrowsingDatabaseNew::PrefixSetContainsUrlHashes(
 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;
@@ -772,26 +875,20 @@ bool SafeBrowsingDatabaseNew::ContainsDownloadUrl(
 }
 
 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;
 
@@ -803,10 +900,6 @@ 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;
@@ -816,22 +909,21 @@ bool SafeBrowsingDatabaseNew::ContainsSideEffectFreeWhitelistUrl(
     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;
@@ -840,10 +932,10 @@ bool SafeBrowsingDatabaseNew::ContainsMalwareIP(const std::string& ip_address) {
   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');
@@ -865,28 +957,21 @@ 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);
+  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;
     }
@@ -899,7 +984,6 @@ 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
@@ -930,7 +1014,6 @@ 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
@@ -965,8 +1048,6 @@ void SafeBrowsingDatabaseNew::InsertSubChunk(
 void SafeBrowsingDatabaseNew::InsertChunks(
     const std::string& list_name,
     const std::vector<SBChunkData*>& chunks) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   if (corruption_detected_ || chunks.empty())
     return;
 
@@ -1000,8 +1081,6 @@ void SafeBrowsingDatabaseNew::InsertChunks(
 
 void SafeBrowsingDatabaseNew::DeleteChunks(
     const std::vector<SBChunkDelete>& chunk_deletes) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   if (corruption_detected_ || chunk_deletes.empty())
     return;
 
@@ -1031,30 +1110,26 @@ 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.
@@ -1110,10 +1185,10 @@ bool SafeBrowsingDatabaseNew::UpdateStarted(
   }
 
   {
-    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(),
@@ -1153,8 +1228,6 @@ bool SafeBrowsingDatabaseNew::UpdateStarted(
 }
 
 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.
@@ -1236,17 +1309,17 @@ void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
 
   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(
@@ -1260,7 +1333,7 @@ void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
   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);
@@ -1272,7 +1345,7 @@ void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
   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);
@@ -1282,19 +1355,17 @@ void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
 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;
   }
 
@@ -1302,18 +1373,16 @@ void SafeBrowsingDatabaseNew::UpdateWhitelistStore(
   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))
@@ -1329,13 +1398,11 @@ int64 SafeBrowsingDatabaseNew::UpdateHashPrefixStore(
 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;
@@ -1359,14 +1426,14 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
   // 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) {
@@ -1380,15 +1447,21 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
 
   // 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)) {
@@ -1416,11 +1489,9 @@ void SafeBrowsingDatabaseNew::UpdatePrefixSetUrlStore(
 }
 
 void SafeBrowsingDatabaseNew::UpdateIpBlacklistStore() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   // Note: prefixes will not be empty.  The current data store implementation
   // stores all full-length hashes as both full and prefix hashes.
-  safe_browsing::PrefixSetBuilder builder;
+  PrefixSetBuilder builder;
   std::vector<SBAddFullHash> full_hashes;
   if (!ip_blacklist_store_->FinishUpdate(&builder, &full_hashes)) {
     RecordFailure(FAILURE_IP_BLACKLIST_UPDATE_FINISH);
@@ -1436,8 +1507,6 @@ 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()) {
@@ -1449,8 +1518,6 @@ void SafeBrowsingDatabaseNew::HandleCorruptDatabase() {
 }
 
 void SafeBrowsingDatabaseNew::OnHandleCorruptDatabase() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-
   RecordFailure(FAILURE_DATABASE_CORRUPT_HANDLER);
   corruption_detected_ = true;  // Stop updating the database.
   ResetDatabase();
@@ -1464,19 +1531,13 @@ void SafeBrowsingDatabaseNew::OnHandleCorruptDatabase() {
 
 // 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;
@@ -1488,15 +1549,15 @@ void SafeBrowsingDatabaseNew::LoadPrefixSet(
   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())
+  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);
 }
 
 bool SafeBrowsingDatabaseNew::Delete() {
-  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!filename_base_.empty());
 
   // TODO(shess): This is a mess.  SafeBrowsingFileStore::Delete() closes the
@@ -1575,12 +1636,9 @@ bool SafeBrowsingDatabaseNew::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;
 
@@ -1602,20 +1660,18 @@ void SafeBrowsingDatabaseNew::WritePrefixSet(
 #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;
   }
 
@@ -1631,9 +1687,10 @@ void SafeBrowsingDatabaseNew::LoadWhitelist(
   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);
   }
@@ -1641,8 +1698,6 @@ void SafeBrowsingDatabaseNew::LoadWhitelist(
 
 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();
@@ -1676,26 +1731,24 @@ void SafeBrowsingDatabaseNew::LoadIpBlacklist(
     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();
 }