Fixed data race between GetDatabase and CheckURL at startup.

Also did a bit of code cleanup.

BUG=11106
TEST=use tsan on ViewSourceTest.DoesBrowserRenderInViewSource

Review URL: http://codereview.chromium.org/273020

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@28743 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 23 insertions, 58 deletions

diff --git a/chrome/browser/safe_browsing/database_perftest.cc b/chrome/browser/safe_browsing/database_perftest.cc
index 90d3d98..e134b18 100644
--- a/chrome/browser/safe_browsing/database_perftest.cc
+++ b/chrome/browser/safe_browsing/database_perftest.cc
@@ -40,7 +40,7 @@ class Database {
     }
   }
 
-  bool Init(const FilePath& name, bool create) {
+  void Init(const FilePath& name, bool create) {
     // get an empty file for the test DB
     FilePath filename;
     PathService::Get(base::DIR_TEMP, &filename);
@@ -55,15 +55,14 @@ class Database {
     }
 
     const std::string sqlite_path = WideToUTF8(filename.ToWStringHack());
-    if (sqlite3_open(sqlite_path.c_str(), &db_) != SQLITE_OK)
-      return false;
+    ASSERT_EQ(sqlite3_open(sqlite_path.c_str(), &db_), SQLITE_OK);
 
     statement_cache_.set_db(db_);
 
     if (!create)
-      return true;
+      return;
 
-    return CreateTable();
+    ASSERT_TRUE(CreateTable());
   }
 
   virtual bool CreateTable() = 0;
@@ -253,7 +252,7 @@ class SafeBrowsing: public testing::Test {
     db_name_.append(db_->GetDBSuffix());
 
     FilePath path = FilePath::FromWStringHack(ASCIIToWide(db_name_));
-    ASSERT_TRUE(db_->Init(path, type == WRITE));
+    db_->Init(path, type == WRITE);
 
     if (type == WRITE) {
       WriteEntries(size);
@@ -398,7 +397,7 @@ class SafeBrowsingDatabaseTest {
 
     scoped_ptr<SafeBrowsingDatabase> database(SafeBrowsingDatabase::Create());
     database->SetSynchronous();
-    EXPECT_TRUE(database->Init(path_, NULL));
+    database->Init(path_, NULL);
 
     int chunk_id = 0;
     int total_host_keys = size;
@@ -431,7 +430,7 @@ class SafeBrowsingDatabaseTest {
 
     scoped_ptr<SafeBrowsingDatabase> database(SafeBrowsingDatabase::Create());
     database->SetSynchronous();
-    EXPECT_TRUE(database->Init(path_, NULL));
+    database->Init(path_, NULL);
 
     PerfTimer total_timer;
     int64 db_ms = 0;
@@ -479,7 +478,7 @@ class SafeBrowsingDatabaseTest {
 
     scoped_ptr<SafeBrowsingDatabase> database(SafeBrowsingDatabase::Create());
     database->SetSynchronous();
-    EXPECT_TRUE(database->Init(path_, NULL));
+    database->Init(path_, NULL);
 
     int64 total_ms = total_timer.Elapsed().InMilliseconds();
 
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.h b/chrome/browser/safe_browsing/safe_browsing_database.h
index a2575eb..dd0d49c4 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.h
+++ b/chrome/browser/safe_browsing/safe_browsing_database.h
@@ -36,7 +36,7 @@ class SafeBrowsingDatabase {
 
   // Initializes the database with the given filename.  The callback is
   // executed after finishing a chunk.
-  virtual bool Init(const FilePath& filename,
+  virtual void Init(const FilePath& filename,
                     Callback0::Type* chunk_inserted_callback) = 0;
 
   // Deletes the current database and creates a new one.
@@ -84,8 +84,9 @@ class SafeBrowsingDatabase {
   // Called when the user's machine has resumed from a lower power state.
   virtual void HandleResume() = 0;
 
-  virtual bool UpdateStarted() { return true; }
-  virtual void UpdateFinished(bool update_succeeded) {}
+  // Returns true if we have successfully started the update transaction.
+  virtual bool UpdateStarted() = 0;
+  virtual void UpdateFinished(bool update_succeeded) = 0;
 
   virtual FilePath filename() const { return filename_; }
 
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc b/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
index f2fbff2..53f46a4 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database_bloom.cc
@@ -54,7 +54,7 @@ SafeBrowsingDatabaseBloom::~SafeBrowsingDatabaseBloom() {
   Close();
 }
 
-bool SafeBrowsingDatabaseBloom::Init(const FilePath& filename,
+void SafeBrowsingDatabaseBloom::Init(const FilePath& filename,
                                      Callback0::Type* chunk_inserted_callback) {
   DCHECK(!init_ && filename_.empty());
 
@@ -67,8 +67,6 @@ bool SafeBrowsingDatabaseBloom::Init(const FilePath& filename,
 
   init_ = true;
   chunk_inserted_callback_.reset(chunk_inserted_callback);
-
-  return true;
 }
 
 bool SafeBrowsingDatabaseBloom::Open() {
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_bloom.h b/chrome/browser/safe_browsing/safe_browsing_database_bloom.h
index f983ce1..57e2b27 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_bloom.h
+++ b/chrome/browser/safe_browsing/safe_browsing_database_bloom.h
@@ -32,53 +32,27 @@ class SafeBrowsingDatabaseBloom : public SafeBrowsingDatabase {
   virtual ~SafeBrowsingDatabaseBloom();
 
   // SafeBrowsingDatabase interface:
-
-  // Initializes the database with the given filename.  The callback is
-  // executed after finishing a chunk.
-  virtual bool Init(const FilePath& filename,
+  virtual void Init(const FilePath& filename,
                     Callback0::Type* chunk_inserted_callback);
-
-  // Deletes the current database and creates a new one.
   virtual bool ResetDatabase();
-
-  // Returns false if the given url is not in the database.  If it returns
-  // true, then either "list" is the name of the matching list, or prefix_hits
-  // contains the matching hash prefixes.
   virtual bool ContainsUrl(const GURL& url,
                            std::string* matching_list,
                            std::vector<SBPrefix>* prefix_hits,
                            std::vector<SBFullHashResult>* full_hits,
                            base::Time last_update);
-
-  // Processes add/sub commands.  Database will free the chunks when it's done.
   virtual void InsertChunks(const std::string& list_name,
                             std::deque<SBChunk>* chunks);
-
-  // Processs adddel/subdel commands.  Database will free chunk_deletes when
-  // it's done.
   virtual void DeleteChunks(std::vector<SBChunkDelete>* chunk_deletes);
-
-  // Returns the lists and their add/sub chunks.
   virtual void GetListsInfo(std::vector<SBListChunkRanges>* lists);
-
-  // Does nothing in this implementation.  Operations in this class are
-  // always synchronous.
   virtual void SetSynchronous();
-
-  // Store the results of a GetHash response. In the case of empty results, we
-  // cache the prefixes until the next update so that we don't have to issue
-  // further GetHash requests we know will be empty.
   virtual void CacheHashResults(
       const std::vector<SBPrefix>& prefixes,
       const std::vector<SBFullHashResult>& full_hits);
-
-  // Called when the user's machine has resumed from a lower power state.
   virtual void HandleResume();
-
-  // Returns true if we have successfully started the update transaction.
   virtual bool UpdateStarted();
   virtual void UpdateFinished(bool update_succeeded);
 
+
   virtual bool NeedToCheckUrl(const GURL& url);
 
  private:
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
index 4542b5c..0e41133 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
@@ -82,7 +82,7 @@ namespace {
 
     SafeBrowsingDatabase* database = SafeBrowsingDatabase::Create();
     database->SetSynchronous();
-    EXPECT_TRUE(database->Init(filename, NULL));
+    database->Init(filename, NULL);
 
     return database;
   }
@@ -1076,7 +1076,7 @@ void PeformUpdate(const std::wstring& initial_db,
 
   SafeBrowsingDatabase* database = SafeBrowsingDatabase::Create();
   database->SetSynchronous();
-  EXPECT_TRUE(database->Init(path, NULL));
+  database->Init(path, NULL);
 
   Time before_time = Time::Now();
   base::ProcessHandle handle = base::Process::Current().handle();
diff --git a/chrome/browser/safe_browsing/safe_browsing_service.cc b/chrome/browser/safe_browsing/safe_browsing_service.cc
index 834d1cc..bc06479 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_service.cc
@@ -371,17 +371,11 @@ SafeBrowsingDatabase* SafeBrowsingService::GetDatabase() {
   SafeBrowsingDatabase* database = SafeBrowsingDatabase::Create();
   Callback0::Type* chunk_callback =
       NewCallback(this, &SafeBrowsingService::ChunkInserted);
-  bool init_success = database->Init(path, chunk_callback);
+  database->Init(path, chunk_callback);
+  database_ = database;
 
   io_loop_->PostTask(FROM_HERE, NewRunnableMethod(
-      this, &SafeBrowsingService::DatabaseLoadComplete, !init_success));
-
-  if (!init_success) {
-    NOTREACHED();
-    return NULL;
-  }
-
-  database_ = database;
+      this, &SafeBrowsingService::DatabaseLoadComplete));
 
   TimeDelta open_time = Time::Now() - before;
   SB_DLOG(INFO) << "SafeBrowsing database open took " <<
@@ -532,15 +526,14 @@ void SafeBrowsingService::OnChunkInserted() {
     protocol_manager_->OnChunkInserted();
 }
 
-void SafeBrowsingService::DatabaseLoadComplete(bool database_error) {
+void SafeBrowsingService::DatabaseLoadComplete() {
   DCHECK(MessageLoop::current() == io_loop_);
   if (!enabled_)
     return;
 
   database_loaded_ = true;
 
-  // TODO(paulg): More robust database initialization error handling.
-  if (protocol_manager_ && !database_error)
+  if (protocol_manager_)
     protocol_manager_->Initialize();
 
   // If we have any queued requests, we can now check them.
diff --git a/chrome/browser/safe_browsing/safe_browsing_service.h b/chrome/browser/safe_browsing/safe_browsing_service.h
index 76b7f8c..cc994ff 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.h
+++ b/chrome/browser/safe_browsing/safe_browsing_service.h
@@ -152,7 +152,7 @@ class SafeBrowsingService
   void ChunkInserted();
 
   // Notification from the database when it's done loading its bloom filter.
-  void DatabaseLoadComplete(bool database_error);
+  void DatabaseLoadComplete();
 
   // Preference handling.
   static void RegisterPrefs(PrefService* prefs);