Reland: 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..590e7f9ccb9442788f7fe7fbc35c60d2ce8474a8 100644
--- a/components/safe_browsing_db/v4_local_database_manager.cc
+++ b/components/safe_browsing_db/v4_local_database_manager.cc
@@ -6,6 +6,7 @@
 
 #include <vector>
 
+#include "base/callback.h"
 #include "components/safe_browsing_db/safebrowsing.pb.h"
 #include "content/public/browser/browser_thread.h"
 
@@ -13,7 +14,12 @@ using content::BrowserThread;
 
 namespace safe_browsing {
 
-V4LocalDatabaseManager::V4LocalDatabaseManager() : enabled_(false) {}
+V4LocalDatabaseManager::V4LocalDatabaseManager(const base::FilePath& base_path)
+    : base_path_(base_path), enabled_(false) {
+  DCHECK(!base_path_.empty());
+  DVLOG(1) << "V4LocalDatabaseManager::V4LocalDatabaseManager: "
+           << "base_path_: " << base_path_.AsUTF8Unsafe();
+}
 
 V4LocalDatabaseManager::~V4LocalDatabaseManager() {
   DCHECK(!enabled_);
@@ -123,8 +129,9 @@ bool V4LocalDatabaseManager::IsCsdWhitelistKillSwitchOn() {
 bool V4LocalDatabaseManager::CheckBrowseUrl(const GURL& url, Client* client) {
   // TODO(vakh): Implement this skeleton.
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  if (!enabled_)
+  if (!enabled_) {
     return true;
+  }
 
   // Don't defer the resource load.
   return true;
@@ -141,13 +148,23 @@ void V4LocalDatabaseManager::StartOnIOThread(
     const V4ProtocolConfig& config) {
   SafeBrowsingDatabaseManager::StartOnIOThread(request_context_getter, config);
 
-#if defined(OS_WIN) || defined (OS_LINUX) || defined (OS_MACOSX)
+  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,28 +176,72 @@ 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;
+void V4LocalDatabaseManager::SetupDatabase() {
+  DCHECK(!base_path_.empty());
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
+  // 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);
+  }
+
+  // TODO(vakh): list_info_map should probably be a hard-coded map.
+  ListInfoMap list_info_map;
+
+  // Do not create the database on the IO thread since this may be an expensive
+  // operation. Instead, do that on the task_runner and when the new database
+  // has been created, swap it out on the IO thread.
+  NewDatabaseReadyCallback db_ready_callback = base::Bind(
+      &V4LocalDatabaseManager::DatabaseReady, base::Unretained(this));
+  V4Database::Create(task_runner_, base_path_, list_info_map,
+                     db_ready_callback);
 }
 
-void V4LocalDatabaseManager::StopOnIOThread(bool shutdown) {
+void V4LocalDatabaseManager::DatabaseReady(
+    std::unique_ptr<V4Database> v4_database) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
-  // Delete the V4UpdateProtocolManager.
-  // This cancels any in-flight update request.
+  v4_database_ = std::move(v4_database);
+
+  // The following check is needed because it is possible that by the time the
+  // database is ready, the user decides to turn off safebrowsing checks and we
+  // destroy v4_update_protocol_manager_

[Scott Hess - ex-Googler, 2016/05/16 07:55:25]

Does this mean that StopOnIOThread() was called?  If so, then  should the test
be be against enabled_, and the database also not be set?

[vakh (use Gerrit instead), 2016/05/16 18:36:15]

Done. It's cleaner.

   if (v4_update_protocol_manager_.get()) {
-    v4_update_protocol_manager_.reset();
+    // The database is in place. Start fetching updates now.
+    v4_update_protocol_manager_->ScheduleNextUpdate();
   }
+}
+
+void V4LocalDatabaseManager::StopOnIOThread(bool shutdown) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
   enabled_ = false;
+
+  // Delete the V4Database.
+  // Any pending writes to disk are completed.
+  v4_database_.reset();

[Scott Hess - ex-Googler, 2016/05/16 07:55:25]

Actually, thinking of the above made me wonder - if the database is created on
task_runner_, should this reset happen on the io thread?  Or should it be
something like task_runner_->DeleteSoon()?

[vakh (use Gerrit instead), 2016/05/16 18:36:15]

[I may have misunderstood you here.]
The database is created on the task runner, but v4_database_ is set on the IO
thread only (in DatabaseReady) so it's probably OK.

[Scott Hess - ex-Googler, 2016/05/16 19:49:52]

The variable manipulation is fine, but the reset() will call ~V4Database(),
which may not be appropriate to call on the I/O thread, for the same basic
reasons V4Database() isn't being called on the I/O thread.

Since V4Database is handling how things are vectored to the task_runner_, having
this call DeleteSoon() directly seems like a layering violation.  Maybe it makes
more sense to have the destructor version of Create():
// static
void V4Database::Destroy(std::unique_ptr<V4Database> v4_database) {
  v4_database->task_runner_->DeleteSoon(v4_database);
}
That way the caller has to pass an owned reference in, and you don't have to
worry about stale refs.

I can think of two general reasons.  One is simply that you should destruct on
the same thread you constructed on, because it's likely that the reason driving
one decision will be the same for the other (like if a database or store has a
cached file descriptor, that needs to be closed on the task_runner_).  The other
is that you probably need a way to order the destructor after all previous
messages have drained.  Any outstanding queries posted to the task_runner_ could
race with destruction of the database instance, unless destruction itself is
posted to the task_runner_.

If the above doesn't ring true, let me know and I'll walk over to discuss in
person.  If I'm wrong, it would suck to have it implemented :-).

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

Seriously - my expectation is you'll think "Oh, yeah, I see what you mean. 
Easily fixed."  If it's "WAT?" or "OMG, that's an entire new nest of spiders",
maybe it would be better to put a pin in this issue and address it in a followup
CL.

[vakh (use Gerrit instead), 2016/05/16 20:50:21]

Done. Good catch!
Or: Oh, yeah, I see what you mean. Easily fixed.

+
+  // Delete the V4UpdateProtocolManager.
+  // This cancels any in-flight update request.
+  v4_update_protocol_manager_.reset();
+
   SafeBrowsingDatabaseManager::StopOnIOThread(shutdown);
 }
 
 void V4LocalDatabaseManager::UpdateRequestCompleted(
     const std::vector<ListUpdateResponse>& responses) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+
   // TODO(vakh): Updates downloaded. Store them on disk and record new state.
+  v4_update_protocol_manager_->ScheduleNextUpdate();
 }
 
 }  // namespace safe_browsing