Fetch incremental updates. Store new state in V4Store.

components/safe_browsing_db/v4_database.cc

Index: components/safe_browsing_db/v4_database.cc
diff --git a/components/safe_browsing_db/v4_database.cc b/components/safe_browsing_db/v4_database.cc
index 32d0494b457f710b69147514791eb9e90514127d..8d76b9f0502cbfae0f0ca4e817a323b64cbfe2dd 100644
--- a/components/safe_browsing_db/v4_database.cc
+++ b/components/safe_browsing_db/v4_database.cc
@@ -19,56 +19,69 @@ namespace safe_browsing {
 V4StoreFactory* V4Database::factory_ = NULL;
 
 // static
-void V4Database::Create(
+bool V4Database::Create(
     const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
     const base::FilePath& base_path,
-    const ListInfoMap& list_info_map,
-    NewDatabaseReadyCallback callback) {
+    const StoreFileNameMap& store_file_name_map,
+    DatabaseUpdatedCallback db_updated_callback,
+    NewDatabaseReadyCallback new_db_callback) {
   // Create the database, which may be a lengthy operation, on the
   // db_task_runner, but once that is done, call the caller back on this
   // thread.
+  if (!base_path.IsAbsolute() || store_file_name_map.empty()) {

[Nathan Parker, 2016/06/15 21:24:11]

Could just dcheck on these, and then the caller doesn't need to decide what to
do with a false return value, assuming this just indicates a programming error

[vakh (use Gerrit instead), 2016/06/16 01:07:34]

Done.

+    return false;
+  }
+
   const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner =
       base::MessageLoop::current()->task_runner();
   db_task_runner->PostTask(
-      FROM_HERE,
-      base::Bind(&V4Database::CreateOnTaskRunner, db_task_runner, base_path,
-                 list_info_map, callback_task_runner, callback));
+      FROM_HERE, base::Bind(&V4Database::CreateOnTaskRunner, db_task_runner,
+                            base_path, store_file_name_map, db_updated_callback,
+                            callback_task_runner, new_db_callback));
+  return true;
 }
 
 // static
 void V4Database::CreateOnTaskRunner(
     const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
     const base::FilePath& base_path,
-    const ListInfoMap& list_info_map,
+    const StoreFileNameMap& store_file_name_map,
+    DatabaseUpdatedCallback db_updated_callback,
     const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner,
-    NewDatabaseReadyCallback callback) {
+    NewDatabaseReadyCallback new_db_callback) {
   DCHECK(db_task_runner->RunsTasksOnCurrentThread());
-  DCHECK(base_path.IsAbsolute());
 
   if (!factory_) {
     factory_ = new V4StoreFactory();
     ANNOTATE_LEAKING_OBJECT_PTR(factory_);
   }
   auto store_map = base::MakeUnique<StoreMap>();
-  for (const auto& list_info : list_info_map) {
-    const UpdateListIdentifier& update_list_identifier = list_info.first;
-    const base::FilePath store_path = base_path.AppendASCII(list_info.second);
+  for (const auto& store_info : store_file_name_map) {
+    const UpdateListIdentifier& update_list_identifier = store_info.first;
+    const base::FilePath store_path = base_path.AppendASCII(store_info.second);
     (*store_map)[update_list_identifier].reset(
         factory_->CreateV4Store(db_task_runner, store_path));
   }
-  std::unique_ptr<V4Database> v4_database(
-      new V4Database(db_task_runner, std::move(store_map)));
+  std::unique_ptr<V4Database> v4_database(new V4Database(
+      db_task_runner, std::move(store_map), db_updated_callback));
 
-  // Database is done loading, pass it to the callback on the caller's thread.
+  // Database is done loading, pass it to the new_db_callback on the caller's
+  // thread.
   callback_task_runner->PostTask(
-      FROM_HERE, base::Bind(callback, base::Passed(&v4_database)));
+      FROM_HERE, base::Bind(new_db_callback, base::Passed(&v4_database)));
 }
 
 V4Database::V4Database(
     const scoped_refptr<base::SequencedTaskRunner>& db_task_runner,
-    std::unique_ptr<StoreMap> store_map)
-    : db_task_runner_(db_task_runner), store_map_(std::move(store_map)) {
+    std::unique_ptr<StoreMap> store_map,
+    DatabaseUpdatedCallback db_updated_callback)
+    : db_task_runner_(db_task_runner),
+      store_map_(std::move(store_map)),
+      store_state_map_(base::MakeUnique<StoreStateMap>()),
+      db_updated_callback_(db_updated_callback),
+      pending_store_updates_(0) {
   DCHECK(db_task_runner->RunsTasksOnCurrentThread());
+  UpdateStoreStateMap();
   // TODO(vakh): Implement skeleton
 }
 
@@ -84,15 +97,69 @@ V4Database::~V4Database() {
   DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 }
 
+void V4Database::ApplyUpdate(const std::vector<ListUpdateResponse>& responses) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(!pending_store_updates_);
+
+  // Post the V4Store update task on the task runner but get the callback on the
+  // current thread (IO thread).
+  const scoped_refptr<base::SingleThreadTaskRunner>& current_task_runner =
+      base::MessageLoop::current()->task_runner();
+  for (const auto& response : responses) {
+    UpdateListIdentifier identifier(response);
+    StoreMap::const_iterator iter = store_map_->find(identifier);
+    if (iter != store_map_->end()) {
+      const auto& old_store = iter->second;
+      if (old_store->state() != response.new_client_state()) {
+        // A different state implies there are updates to process.
+        UpdatedStoreReadyCallback store_ready_callback = base::Bind(
+            &V4Database::UpdatedStoreReady, base::Unretained(this), identifier);
+        db_task_runner_->PostTask(

[Nathan Parker, 2016/06/15 21:24:11]

Increment before posting, since otherwise the posted task could finish before it
increments.

[vakh (use Gerrit instead), 2016/06/16 01:07:34]

Done. Though that's not a problem since the synchronization is achieved by
having it call back UpdatedStoreReady which can't run until the current method
finishes since they both run on the IO thread.

+            FROM_HERE,
+            base::Bind(&V4Store::ApplyUpdate, base::Unretained(old_store.get()),
+                       response, current_task_runner, store_ready_callback));
+        pending_store_updates_++;
+      }
+    } else {
+      DVLOG(1) << "Got update for unexpected identifier: " << identifier;

[Nathan Parker, 2016/06/15 21:24:12]

I'd say this is more severe, and could be a DCHECK.

[vakh (use Gerrit instead), 2016/06/16 01:07:35]

Done.

+    }
+  }
+
+  if (!pending_store_updates_) {
+    db_updated_callback_.Run();
+  }
+}
+
+void V4Database::UpdatedStoreReady(UpdateListIdentifier identifier,
+                                   std::unique_ptr<V4Store> new_store) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(pending_store_updates_);
+  (*store_state_map_)[identifier] = new_store->state();
+  (*store_map_)[identifier] = std::move(new_store);
+
+  // Since this method is always called on the IO thread, there can't be a race
+  // condition in decrementing pending_store_updates_.
+  pending_store_updates_--;
+  if (!pending_store_updates_) {
+    db_updated_callback_.Run();
+  }
+}
+
 bool V4Database::ResetDatabase() {
   DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
   bool reset_success = true;
-  for (const auto& store_id_and_store : *store_map_) {
-    if (!store_id_and_store.second->Reset()) {
+  for (const auto& store_map_iter : *store_map_) {
+    if (!store_map_iter.second->Reset()) {
       reset_success = false;
     }
   }
   return reset_success;
 }
 
+void V4Database::UpdateStoreStateMap() {
+  for (const auto& store_map_iter : *store_map_) {
+    (*store_state_map_)[store_map_iter.first] = store_map_iter.second->state();

[Nathan Parker, 2016/06/15 21:24:13]

I'm confused: is the store_state_map just a view into the store_map?  If so why
store it separately?

[vakh (use Gerrit instead), 2016/06/16 01:07:35]

Yes, it is a view.
If we do not have a store_state_map_, we would need to get the state of the
stores from store_map_ for each update. And since most updates do not lead to
state changes, most of the time, the state of each store would be the same as
last time.

So, instead of looking up the state from the stores for each update, the
store_state_map  provides that information right away.

[Nathan Parker, 2016/06/17 21:34:19]

What if you created this map on the heap only when it was time to schedule an
update, and passed it to the protocol manager (passing ownership of it)?  That
way you remove the copy here and the protocol manager doesn't have to keep a ptr
back into this class.  That would decouple them even more.  It should be the
case that the state should not change before the update manager completes its
next fetch, yes?

Keeping two data structures in sync can increase the risk of bugs where they're
not in sync.  And we'd need semantics to ensure they're accessed on the correct
threads (I think they are, but it's not obvious from the code).

If you think the current way is better then go ahead and submit and we'll hash
it out later (file a bug?).  I don't want this point to hold you up. 

[vakh (use Gerrit instead), 2016/06/21 00:29:36]

Done. See patch 10.

+  }
+}
+
 }  // namespace safe_browsing