From fd220e6008ba976de0de1a5dd5c10576c2cd8450 Mon Sep 17 00:00:00 2001
From: "pkasting@chromium.org"
<pkasting@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>
Date: Sat, 14 Nov 2009 01:02:37 +0000
Subject: More safe browsing cleanup work: * Remove |resetting_| from the safe
browsing service, it doesn't do anything useful. * Add appropriate locks in a
few places in the database that were missing them. * Prevent potential
infinite recursion in the database at one spot.
BUG=none
TEST=none
Review URL: http://codereview.chromium.org/391060
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@31979 0039d316-1c4b-4281-b951-d872f2087c98
---
.../safe_browsing/safe_browsing_database_bloom.cc | 40 ++++++++++++++--------
.../safe_browsing/safe_browsing_database_bloom.h | 10 +++---
.../browser/safe_browsing/safe_browsing_service.cc | 34 +++++++++---------
.../browser/safe_browsing/safe_browsing_service.h | 3 --
4 files changed, 48 insertions(+), 39 deletions(-)
(limited to 'chrome/browser/safe_browsing')
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc b/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
index 1f4d943..650f556 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
@@ -10,6 +10,7 @@
#include "base/message_loop.h"
#include "base/platform_thread.h"
#include "base/process_util.h"
+#include "base/scoped_bool.h"
#include "base/sha2.h"
#include "base/stats_counters.h"
#include "base/string_util.h"
@@ -38,9 +39,9 @@ static const FilePath::CharType kBloomFilterFileSuffix[] =
SafeBrowsingDatabaseBloom::SafeBrowsingDatabaseBloom()
: db_(NULL),
- init_(false),
ALLOW_THIS_IN_INITIALIZER_LIST(reset_factory_(this)),
- add_count_(0) {
+ add_count_(0),
+ performing_reset_(false) {
}
SafeBrowsingDatabaseBloom::~SafeBrowsingDatabaseBloom() {
@@ -49,16 +50,17 @@ SafeBrowsingDatabaseBloom::~SafeBrowsingDatabaseBloom() {
void SafeBrowsingDatabaseBloom::Init(const FilePath& filename,
Callback0::Type* chunk_inserted_callback) {
- DCHECK(!init_ && filename_.empty());
+ DCHECK(filename_.empty()); // Ensure we haven't been run before.
filename_ = FilePath(filename.value() + kBloomFilterFileSuffix);
bloom_filter_filename_ = BloomFilterFilename(filename_);
+ // 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.
hash_cache_.reset(new HashCache);
LoadBloomFilter();
- init_ = true;
chunk_inserted_callback_.reset(chunk_inserted_callback);
}
@@ -186,26 +188,33 @@ bool SafeBrowsingDatabaseBloom::CreateTables() {
return true;
}
-// The SafeBrowsing service assumes this operation is run synchronously on the
-// database thread. Any URLs that the service needs to check when this is
-// running are queued up and run once the reset is done.
bool SafeBrowsingDatabaseBloom::ResetDatabase() {
- hash_cache_->clear();
- ClearUpdateCaches();
+ // Open() can call us when trying to handle potential database corruption.
+ // Because we call Open() at the bottom of the function, we need to guard
+ // against recursion here.
+ if (performing_reset_)
+ return false; // Don't recurse.
+
+ ScopedBool preforming_reset_scope_(&performing_reset_);
+ // Delete files on disk.
bool rv = Close();
DCHECK(rv);
-
if (!file_util::Delete(filename_, false)) {
NOTREACHED();
return false;
}
-
DeleteBloomFilter();
- bloom_filter_ = new BloomFilter(BloomFilter::kBloomFilterMinSize *
- BloomFilter::kBloomFilterSizeRatio);
- // TODO(paulg): Fix potential infinite recursion between Open and Reset.
+ // Reset objects in memory.
+ {
+ AutoLock lock(lookup_lock_);
+ hash_cache_->clear();
+ ClearUpdateCaches();
+ bloom_filter_ = new BloomFilter(BloomFilter::kBloomFilterMinSize *
+ BloomFilter::kBloomFilterSizeRatio);
+ }
+
return Open();
}
@@ -225,6 +234,7 @@ bool SafeBrowsingDatabaseBloom::CheckCompatibleVersion() {
}
void SafeBrowsingDatabaseBloom::ClearUpdateCaches() {
+ AutoLock lock(lookup_lock_);
add_del_cache_.clear();
sub_del_cache_.clear();
add_chunk_cache_.clear();
@@ -1214,6 +1224,7 @@ void SafeBrowsingDatabaseBloom::BuildBloomFilter() {
add_count_ = GetAddPrefixCount();
if (add_count_ == 0) {
+ AutoLock lock(lookup_lock_);
bloom_filter_ = NULL;
return;
}
@@ -1318,6 +1329,7 @@ void SafeBrowsingDatabaseBloom::GetCachedFullHashes(
std::vector<SBFullHashResult>* full_hits,
Time last_update) {
DCHECK(prefix_hits && full_hits);
+ lookup_lock_.AssertAcquired();
Time max_age = Time::Now() - TimeDelta::FromMinutes(kMaxStalenessMinutes);
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_bloom.h b/chrome/browser/safe_browsing/safe_browsing_database_bloom.h
index c6a65b3..296e4e649 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_bloom.h
+++ b/chrome/browser/safe_browsing/safe_browsing_database_bloom.h
@@ -178,9 +178,6 @@ class SafeBrowsingDatabaseBloom : public SafeBrowsingDatabase {
// Cache of compiled statements for our database.
scoped_ptr<SqliteStatementCache> statement_cache_;
- // True iff the database has been opened successfully.
- bool init_;
-
// Called after an add/sub chunk is processed.
scoped_ptr<Callback0::Type> chunk_inserted_callback_;
@@ -202,9 +199,14 @@ class SafeBrowsingDatabaseBloom : public SafeBrowsingDatabase {
// Transaction for protecting database integrity during updates.
scoped_ptr<SQLTransaction> insert_transaction_;
- // Lock for protecting access to the bloom filter and hash cache.
+ // Lock for protecting access to variables that may be used on the IO thread.
+ // This includes |bloom_filter_|, |hash_cache_| and |prefix_miss_cache_|.
Lock lookup_lock_;
+ // True if we're in the middle of a reset. This is used to prevent possible
+ // infinite recursion.
+ bool performing_reset_;
+
// A store for GetHash results that have not yet been written to the database.
HashList pending_full_hashes_;
diff --git a/chrome/browser/safe_browsing/safe_browsing_service.cc b/chrome/browser/safe_browsing/safe_browsing_service.cc
index e59183b..cc01493 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_service.cc
@@ -39,7 +39,6 @@ SafeBrowsingService::SafeBrowsingService()
: database_(NULL),
protocol_manager_(NULL),
enabled_(false),
- resetting_(false),
update_in_progress_(false) {
}
@@ -66,7 +65,7 @@ bool SafeBrowsingService::CheckUrl(const GURL& url, Client* client) {
if (!enabled_)
return true;
- if (!database_ || resetting_) {
+ if (!database_) {
QueuedCheck check;
check.client = client;
check.url = url;
@@ -114,7 +113,7 @@ void SafeBrowsingService::CancelCheck(Client* client) {
}
// Scan the queued clients store. Clients may be here if they requested a URL
- // check before the database has finished loading or resetting.
+ // check before the database has finished loading.
for (std::deque<QueuedCheck>::iterator it(queued_checks_.begin());
it != queued_checks_.end(); ++it) {
if (it->client == client)
@@ -261,7 +260,6 @@ void SafeBrowsingService::RegisterPrefs(PrefService* prefs) {
void SafeBrowsingService::ResetDatabase() {
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
- resetting_ = true;
safe_browsing_thread_->message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
this, &SafeBrowsingService::OnResetDatabase));
}
@@ -317,7 +315,6 @@ void SafeBrowsingService::OnIOShutdown() {
return;
enabled_ = false;
- resetting_ = false;
// This cancels all in-flight GetHash requests.
delete protocol_manager_;
@@ -467,8 +464,7 @@ void SafeBrowsingService::DatabaseLoadComplete() {
return;
// If we have any queued requests, we can now check them.
- if (!resetting_)
- RunQueuedClients();
+ RunQueuedClients();
}
void SafeBrowsingService::HandleChunkForDatabase(
@@ -545,7 +541,6 @@ void SafeBrowsingService::Start() {
void SafeBrowsingService::OnResetDatabase() {
DCHECK(MessageLoop::current() == safe_browsing_thread_->message_loop());
GetDatabase()->ResetDatabase();
- resetting_ = false;
ChromeThread::PostTask(
ChromeThread::IO, FROM_HERE,
NewRunnableMethod(this, &SafeBrowsingService::DatabaseLoadComplete));
@@ -651,19 +646,22 @@ void SafeBrowsingService::ReportMalware(const GURL& malware_url,
const GURL& referrer_url) {
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
- if (!enabled_ || !database_)
+ if (!enabled_)
return;
- // Check if 'page_url' is already blacklisted (exists in our cache). Only
- // report if it's not there.
- std::string list;
- std::vector<SBPrefix> prefix_hits;
- std::vector<SBFullHashResult> full_hits;
- database_->ContainsUrl(page_url, &list, &prefix_hits, &full_hits,
- protocol_manager_->last_update());
+ if (database_) {
+ // Check if 'page_url' is already blacklisted (exists in our cache). Only
+ // report if it's not there.
+ std::string list;
+ std::vector<SBPrefix> prefix_hits;
+ std::vector<SBFullHashResult> full_hits;
+ database_->ContainsUrl(page_url, &list, &prefix_hits, &full_hits,
+ protocol_manager_->last_update());
+ if (!full_hits.empty())
+ return;
+ }
- if (full_hits.empty())
- protocol_manager_->ReportMalware(malware_url, page_url, referrer_url);
+ protocol_manager_->ReportMalware(malware_url, page_url, referrer_url);
}
void SafeBrowsingService::RunQueuedClients() {
diff --git a/chrome/browser/safe_browsing/safe_browsing_service.h b/chrome/browser/safe_browsing/safe_browsing_service.h
index a2d5adb..9f138c8 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.h
+++ b/chrome/browser/safe_browsing/safe_browsing_service.h
@@ -274,9 +274,6 @@ class SafeBrowsingService
// The SafeBrowsing thread that runs database operations.
scoped_ptr<base::Thread> safe_browsing_thread_;
- // Indicates if we are in the process of resetting the database.
- bool resetting_;
-
// Indicates if we're currently in an update cycle.
bool update_in_progress_;
--
cgit v1.1