Commit b2f71d56 authored by gab's avatar gab Committed by Commit bot

Introduce SafeBrowsingDatabase::ThreadSafeStateManager.

This enforces thread-safety by design via transactions for any
implementation code accessing shared SafeBrowsingDatabase members.

Previously this was enforced by assuming everybody would make proper
usage of |lookup_lock_| but this was deemed insufficient as programming
mistakes proved easy to make twice.

One of the subtleties of this transactional model (which made the previous
ad-hoc locking even harder to prove (and keep) correct) is that only the
main database thread is allowed to modify this state, allowing for
unlocked reads on the main thread which are important to avoid
contention when flushing to disk.

BUG=440517

Review URL: https://codereview.chromium.org/794273002

Cr-Commit-Position: refs/heads/master@{#309611}
parent 908f1466
...@@ -830,7 +830,8 @@ void SafeBrowsingDatabaseTest::PopulateDatabaseForCacheTest() { ...@@ -830,7 +830,8 @@ void SafeBrowsingDatabaseTest::PopulateDatabaseForCacheTest() {
database_->UpdateFinished(true); database_->UpdateFinished(true);
// Cache should be cleared after updating. // Cache should be cleared after updating.
EXPECT_TRUE(database_->prefix_gethash_cache_.empty()); EXPECT_TRUE(
database_->GetUnsynchronizedPrefixGetHashCacheForTesting()->empty());
SBFullHashResult full_hash; SBFullHashResult full_hash;
full_hash.list_id = safe_browsing_util::MALWARE; full_hash.list_id = safe_browsing_util::MALWARE;
...@@ -854,7 +855,8 @@ TEST_F(SafeBrowsingDatabaseTest, HashCaching) { ...@@ -854,7 +855,8 @@ TEST_F(SafeBrowsingDatabaseTest, HashCaching) {
PopulateDatabaseForCacheTest(); PopulateDatabaseForCacheTest();
// We should have both full hashes in the cache. // We should have both full hashes in the cache.
EXPECT_EQ(2U, database_->prefix_gethash_cache_.size()); EXPECT_EQ(2U,
database_->GetUnsynchronizedPrefixGetHashCacheForTesting()->size());
// Test the cache lookup for the first prefix. // Test the cache lookup for the first prefix.
std::vector<SBPrefix> prefix_hits; std::vector<SBPrefix> prefix_hits;
...@@ -910,7 +912,8 @@ TEST_F(SafeBrowsingDatabaseTest, HashCaching) { ...@@ -910,7 +912,8 @@ TEST_F(SafeBrowsingDatabaseTest, HashCaching) {
database_->UpdateFinished(true); database_->UpdateFinished(true);
EXPECT_FALSE(database_->ContainsBrowseUrl( EXPECT_FALSE(database_->ContainsBrowseUrl(
GURL("http://www.evil.com/malware.html"), &prefix_hits, &cache_hits)); GURL("http://www.evil.com/malware.html"), &prefix_hits, &cache_hits));
EXPECT_TRUE(database_->prefix_gethash_cache_.empty()); EXPECT_TRUE(
database_->GetUnsynchronizedPrefixGetHashCacheForTesting()->empty());
prefix_hits.clear(); prefix_hits.clear();
cache_hits.clear(); cache_hits.clear();
...@@ -919,13 +922,13 @@ TEST_F(SafeBrowsingDatabaseTest, HashCaching) { ...@@ -919,13 +922,13 @@ TEST_F(SafeBrowsingDatabaseTest, HashCaching) {
// cache insert uses Time::Now(). First, store some entries. // cache insert uses Time::Now(). First, store some entries.
PopulateDatabaseForCacheTest(); PopulateDatabaseForCacheTest();
std::map<SBPrefix, SBCachedFullHashResult>* hash_cache = SafeBrowsingDatabaseNew::PrefixGetHashCache* hash_cache =
&database_->prefix_gethash_cache_; database_->GetUnsynchronizedPrefixGetHashCacheForTesting();
EXPECT_EQ(2U, hash_cache->size()); EXPECT_EQ(2U, hash_cache->size());
// Now adjust one of the entries times to be in the past. // Now adjust one of the entries times to be in the past.
const SBPrefix key = SBPrefixForString("www.evil.com/malware.html"); const SBPrefix key = SBPrefixForString("www.evil.com/malware.html");
std::map<SBPrefix, SBCachedFullHashResult>::iterator iter = SafeBrowsingDatabaseNew::PrefixGetHashCache::iterator iter =
hash_cache->find(key); hash_cache->find(key);
ASSERT_TRUE(iter != hash_cache->end()); ASSERT_TRUE(iter != hash_cache->end());
iter->second.expire_after = Time::Now() - TimeDelta::FromMinutes(1); iter->second.expire_after = Time::Now() - TimeDelta::FromMinutes(1);
...@@ -1885,7 +1888,8 @@ TEST_F(SafeBrowsingDatabaseTest, BrowseFullHashMatching) { ...@@ -1885,7 +1888,8 @@ TEST_F(SafeBrowsingDatabaseTest, BrowseFullHashMatching) {
database_->UpdateFinished(true); database_->UpdateFinished(true);
// Cache should be cleared after updating. // Cache should be cleared after updating.
EXPECT_TRUE(database_->prefix_gethash_cache_.empty()); EXPECT_TRUE(
database_->GetUnsynchronizedPrefixGetHashCacheForTesting()->empty());
{ {
// Now the database doesn't contain kFullHash1_1. // Now the database doesn't contain kFullHash1_1.
...@@ -1922,7 +1926,8 @@ TEST_F(SafeBrowsingDatabaseTest, BrowseFullHashMatching) { ...@@ -1922,7 +1926,8 @@ TEST_F(SafeBrowsingDatabaseTest, BrowseFullHashMatching) {
database_->UpdateFinished(true); database_->UpdateFinished(true);
// Cache should be cleared after updating. // Cache should be cleared after updating.
EXPECT_TRUE(database_->prefix_gethash_cache_.empty()); EXPECT_TRUE(
database_->GetUnsynchronizedPrefixGetHashCacheForTesting()->empty());
{ {
// None are present. // None are present.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment