Initialize and reset V4LocalDBManager. Instantiate V4Stores.

components/safe_browsing_db/v4_local_database_manager.cc

Index: components/safe_browsing_db/v4_local_database_manager.cc
diff --git a/components/safe_browsing_db/v4_local_database_manager.cc b/components/safe_browsing_db/v4_local_database_manager.cc
index 376d26edac729bd59e0727bb0a9588be447cd9e2..4571c6381ae6cb52ad4f87b6bb2f2e3b36de7975 100644
--- a/components/safe_browsing_db/v4_local_database_manager.cc
+++ b/components/safe_browsing_db/v4_local_database_manager.cc
@@ -13,7 +13,12 @@ using content::BrowserThread;
 
 namespace safe_browsing {
 
-V4LocalDatabaseManager::V4LocalDatabaseManager() : enabled_(false) {}
+V4LocalDatabaseManager::V4LocalDatabaseManager(const base::FilePath& base_path)
+    : base_path_(base_path),
+      enabled_(false),
+      closing_database_(false) {
+      DCHECK(!base_path_.empty());
+    }

[Scott Hess - ex-Googler, 2016/05/09 19:40:39]

DCHECK() and closing brace should be undented 4 spaces.

[vakh (use Gerrit instead), 2016/05/11 20:01:27]

Done.

 
 V4LocalDatabaseManager::~V4LocalDatabaseManager() {
   DCHECK(!enabled_);
@@ -141,13 +146,31 @@ void V4LocalDatabaseManager::StartOnIOThread(
     const V4ProtocolConfig& config) {
   SafeBrowsingDatabaseManager::StartOnIOThread(request_context_getter, config);
 
-#if defined(OS_WIN) || defined (OS_LINUX) || defined (OS_MACOSX)
+  // Only get a new task runner if there isn't one already. If the service has
+  // previously been started and stopped, a task runner could already exist.
+  if (!task_runner_) {
+    base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool();
+    task_runner_ = pool->GetSequencedTaskRunnerWithShutdownBehavior(
+        pool->GetSequenceToken(), base::SequencedWorkerPool::SKIP_ON_SHUTDOWN);
+  }
+
+  SetupUpdateProtocolManager(request_context_getter, config);
+
+  SetupDatabase();
+
+  enabled_ = true;
+}
+
+void V4LocalDatabaseManager::SetupUpdateProtocolManager(
+    net::URLRequestContextGetter* request_context_getter,
+    const V4ProtocolConfig& config) {
+#if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_MACOSX)
   // TODO(vakh): Remove this if/endif block when the V4Database is implemented.
   // Filed as http://crbug.com/608075
   UpdateListIdentifier update_list_identifier;
 #if defined(OS_WIN)
   update_list_identifier.platform_type = WINDOWS_PLATFORM;
-#elif defined (OS_LINUX)
+#elif defined(OS_LINUX)
   update_list_identifier.platform_type = LINUX_PLATFORM;
 #else
   update_list_identifier.platform_type = OSX_PLATFORM;
@@ -159,25 +182,86 @@ void V4LocalDatabaseManager::StartOnIOThread(
 
   V4UpdateCallback callback = base::Bind(
       &V4LocalDatabaseManager::UpdateRequestCompleted, base::Unretained(this));
+
   v4_update_protocol_manager_ = V4UpdateProtocolManager::Create(
       request_context_getter, config, current_list_states_, callback);
+}
 
-  enabled_ = true;
+bool V4LocalDatabaseManager::DatabaseAvailable() const {
+  base::AutoLock lock(database_lock_);
+  return !closing_database_ && v4_database_.get();
+}
+
+void V4LocalDatabaseManager::SetupDatabase() {
+  DCHECK(task_runner_);
+  DCHECK(!base_path_.empty());
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  if (DatabaseAvailable())
+    return;
+
+  // TODO(vakh): list_info_map should probably be a hard-coded map.
+  ListInfoMap list_info_map;
+
+  // Acquiring the lock here guarantees correct ordering between the writes to
+  // the database object across threads.
+  base::AutoLock lock(database_lock_);
+  v4_database_.reset(
+      V4Database::Create(task_runner_, base_path_, list_info_map));
 }
 
 void V4LocalDatabaseManager::StopOnIOThread(bool shutdown) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  enabled_ = false;
+
   // Delete the V4UpdateProtocolManager.
   // This cancels any in-flight update request.
   if (v4_update_protocol_manager_.get()) {
     v4_update_protocol_manager_.reset();
   }
 
-  enabled_ = false;
+  // Close the database.  Cases to avoid:
+  //  * If |closing_database_| is true, continuing will queue up a second
+  //    request, |closing_database_| will be reset after handling the first
+  //    request, and if any functions on the db thread recreate the database, we
+  //    could start using it on the IO thread and then have the second request
+  //    handler delete it out from under us.
+  //  * If |v4_database_| isn't set, then either no creation request is in
+  //    flight, in which case we don't need to do anything, or one is in flight,
+  //    in which case the database will be recreated before our deletion request
+  //    is handled, and could be used on the IO thread in that time period,
+  //    leading to the same problem as above.
+  // Checking DatabaseAvailable() avoids both of these.
+  if (DatabaseAvailable()) {
+    closing_database_ = true;

[Scott Hess - ex-Googler, 2016/05/09 19:40:39]

Before I dig in and ponder things like "OMG, can't we just kill this code,
rather than propagating it?", I want to ask whether we're in a situation where
this code is the only plausible way to make progress, or if it's just happening
because it's being copied and pasted from elsewhere?

I think this basic approach has lived in this code (obviously not this
_specific_ code) for many many years.  Periodically there will be a change
resulting a a contentious review where we worked our race conditions and papered
over them, but I've often not found myself entirely convinced that the approach
was a good one.  What's putting me back in that mind is the amount of comments
explaining things, plus pieces which seem racy (here closing_database_ is being
set without a lock, SetupDatabase() has two different locking scopes, I can
probably find more).  So if this is only of this form "just because that's how
it was before", then maybe now would be a good time to think on approaches which
would be more obviously correct without a bunch of explanations and assertions.

But if there's a higher-level reason why other approaches would be unacceptable,
then stop me before I spend time thinking about it :-).

[Scott Hess - ex-Googler, 2016/05/09 20:28:08]

Too late!  Already pondered!

The original source for this code references database_ for two reasons.  There
is a set of thread-safe methods which can be called where database_ uses
internal locking.  Then there are the other methods which need to be async on
another thread.  The entire handle-setting part does _not_ really need all of
this cross-thread stuff.

Due to the consistent task_runner_, this class can assume that once it posts
"Close the database" to the background thread, it could simply stop using the
thread-safe handle.  Then this code could have setup like:

 - Post a setup task to the background thread.
 - Background thread does setup and posts a completion task to originating
thread.
 - Originating thread's task gets thread-safe handle as a parameter and stores
it with no locking.

teardown could be like:

 - Post a teardown task to the background thread.
 - Immediately forget thread-safe handle, no locking needed.
 - If setup is called here, a new background setup task will post after the
posted teardown task.

The foreground code can still track "Was the setup task posted already?" to
prevent creation of multiple database_ handles.

[vakh (use Gerrit instead), 2016/05/09 20:32:25]

Scott, you're right that some of this code, such as locking and
closing_database_, are pieces from existing code because it seems
to make sense why it would be done.

That being said, the re-write does aim to remove things that are unneeded today
or are inapplicable with the new protocol so I am going to keep this simple and
remove the locks for now. I can always add them later if needed.

On 2016/05/09 19:40:39, Scott Hess wrote:

[vakh (use Gerrit instead), 2016/05/09 20:34:29]

Sounds good. I'll look into this.
My understanding of threads in Chromium isn't fantastic so this is super useful.

+
+    // Scheduling it as a task instead of doing this right away since this may
+    // be a long operation (closing stores, writing to disk, etc.)
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&V4LocalDatabaseManager::OnCloseDatabase, this));
+  }
+
   SafeBrowsingDatabaseManager::StopOnIOThread(shutdown);
 }
 
+void V4LocalDatabaseManager::OnCloseDatabase() {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+  DCHECK(closing_database_);
+
+  // Acquiring the lock here guarantees correct ordering between the resetting
+  // of |v4_database_| and of |closing_database_|, which ensures there won't
+  // be a window during which the IO thread falsely believes the database is
+  // available.
+  base::AutoLock lock(database_lock_);
+
+  // Because |closing_database_| is true, nothing on the IO thread will be
+  // accessing the database, so it's safe to reset.
+  v4_database_.reset();
+
+  closing_database_ = false;
+}
+
 void V4LocalDatabaseManager::UpdateRequestCompleted(
     const std::vector<ListUpdateResponse>& responses) {
   // TODO(vakh): Updates downloaded. Store them on disk and record new state.