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..20711340207600c6affd40dba037259c81a88159 100644
--- a/components/safe_browsing_db/v4_database.cc
+++ b/components/safe_browsing_db/v4_database.cc
@@ -19,56 +19,67 @@ 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.
+  DCHECK(base_path.IsAbsolute());
+  DCHECK(!store_file_name_map.empty());
+
   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;

[Scott Hess - ex-Googler, 2016/06/17 22:53:42]

This seems like it always returns true.  Since it's static, it can't be virtual,
so I'm confused why the change from void?

[vakh (use Gerrit instead), 2016/06/20 22:28:42]

I was initially returning false for the DCHECKS above (instead of the DCHECKs)
but nparker@ said that this should be a harder fail so changed it to DCHECK, but
forgot to revert the return type chage. Done now.

 }
 
 // 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)),
+      db_updated_callback_(db_updated_callback),
+      pending_store_updates_(0) {
   DCHECK(db_task_runner->RunsTasksOnCurrentThread());
+  UpdateStoreStateMap();
   // TODO(vakh): Implement skeleton
 }
 
@@ -84,15 +95,70 @@ 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()) {

[Scott Hess - ex-Googler, 2016/06/17 22:53:42]

I'm not entirely liking the auto for |response| and |old_store|.  Maybe I'm just
too traditionalist, but it feels like almost anything could pop out, here.

[vakh (use Gerrit instead), 2016/06/20 22:28:42]

"for (const auto& iter ...)" is a commonly used and encouraged pattern so
leaving the response as is, but I've changed the declaration for old_store.

[Scott Hess - ex-Googler, 2016/06/21 21:03:44]

Yeah, I'm just feeling like this case is not incredibly obvious what the various
calls are doing, without popping over to the header to look at the definition of
ListUpdateResponse or StoreMap.  I mean like state() and new_client_state(). 
It's easy enough to guess the intention, but harder to see if the intention
departed from what's actually happening :-).

[vakh (use Gerrit instead), 2016/06/21 23:19:35]

Fair enough. Done. Changed for response.
For the other occurrences, those are map iterators and I'd prefer keeping them
that way.

[Scott Hess - ex-Googler, 2016/06/24 23:01:18]

Acknowledged.  I feel like this looks less magical.

+        // A different state implies there are updates to process.
+        pending_store_updates_++;
+        UpdatedStoreReadyCallback store_ready_callback = base::Bind(
+            &V4Database::UpdatedStoreReady, base::Unretained(this), identifier);
+        db_task_runner_->PostTask(
+            FROM_HERE,
+            base::Bind(&V4Store::ApplyUpdate, base::Unretained(old_store.get()),
+                       response, current_task_runner, store_ready_callback));

[Scott Hess - ex-Googler, 2016/06/17 22:53:42]

I'm slightly questioning whether this is better done as a series of separately
posted per-store tasks, or a single db-level task which iterates the stores.  I
can see that there are cases where the set of responses will be greater than the
set of stores updated, but it feels like the common case is that all of the
responses result in updates.

Moving this to a db-level task would mean that you wouldn't have to track
pending_store_updates_, and you also wouldn't have to have store_ready_callback
coalescing to call db_updated_callback_.  In fact, in that case you could
probably pass db_updated_callback as a parameter to ApplyUpdate() and get rid of
the member variable (which seems like kind of a wart).

[vakh (use Gerrit instead), 2016/06/20 22:28:42]

Writing a store to disk is a long operation so the idea is to keep individual
tasks on the task runner short in order to not hog the runner for all stores at
once. I'm not much familiar with the guidelines about this.
Do you know if it is OK to run long operations on the task runner? If it is,
then doing the update all at once is definitely better.

[Scott Hess - ex-Googler, 2016/06/21 21:03:44]

I wouldn't be upset about running long jobs on the task runner - as far as I can
tell, there's no particular recommendation or best practices on such things.  If
the infrastructure needs to make guarantees on whether things can be dispatched,
it should either be making assertions about task runtime, or using a pool with
appropriate serialization.

If you think a single long-running task is poor practice, it still might make
sense to post a single background task which chains to itself to pump stores
through.  But since each task is asymmetric, I'm not sure how valuable that
would really be.  It might make more sense if they ran in parallel, but that
also may be too aggressive.

[By pumping in the background, I mean a task which takes a collection of stores
and a callback, then repeatedly does:
 - remove a store from the collection.
 - write that store.
 - if empty, post the callback.
 - else, repost the task with the reduced collection.
That removes the outstanding counter, and allows to have a single point to
dispatch the updated callback.
]

[vakh (use Gerrit instead), 2016/06/23 06:23:49]

If you feel strongly about making this change, I can go ahead and do it.
However, I get the sense that both of us agree and disagree with each other, and
none of us feels strongly about it.
The current implementation is along the lines of what I proposed in the design
document so I propose that we keep the current implementation. If at a later
time we have a strong and evidence-based reason to change it, we'll do it then.
WDYT?

[Scott Hess - ex-Googler, 2016/06/24 23:01:18]

OK, I do think it's a style thing rather than a real functional issue.

[vakh (use Gerrit instead), 2016/06/27 19:38:26]

Acknowledged.

+      }
+    } else {
+      DVLOG(1) << "Got update for unexpected identifier: " << identifier;
+      NOTREACHED();

[Scott Hess - ex-Googler, 2016/06/17 22:53:42]

The two lines just create a poor DCHECK, don't they?  DVLOG() means only in
debug mode, NOTREACHED() will abort in debug.

You can probably say NOTREACHED() << "Got update...", too.

[vakh (use Gerrit instead), 2016/06/20 22:28:42]

Done.

+    }
+  }
+
+  if (!pending_store_updates_) {
+    db_updated_callback_.Run();
+  }

[Scott Hess - ex-Googler, 2016/06/17 22:53:42]

Should this be a post rather than a direct run, so that the callback runs async
in either case?

[vakh (use Gerrit instead), 2016/06/20 22:28:42]

Done.

+}
+
+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_.

[Scott Hess - ex-Googler, 2016/06/17 22:53:42]

Do we have any sort of thread annotations?  This comment seems like it belongs
with pending_store_updates_ definition, because it needs to apply to all
accesses, not just this one.

Maybe it would make sense to add a wrapper for accesses to enforce thread usage
guidelines, and have the decrement wrapper chain invoke the callback?

[vakh (use Gerrit instead), 2016/06/20 22:28:42]

Sorry, I'm not sure what you mean.
Done.
Do you know of any example that I can look at to understand how this can be done
and what exactly it does?

[Scott Hess - ex-Googler, 2016/06/21 21:03:43]

In some systems (perhaps not Chromium), you can annotate variables or accesses
to say "This can only happen on thread X", and then automated tooling will
verify that's happening.  Simply making a comment like this is brittle.
What I mean is accessors coded like:

  void set_pending_store_updates(size_t p) {
    DCHECK_CURRENTLY_ON(BrowserThread::IO);
    pending_store_updates_ = p;
  }
  size_t pending_store_updates() const {
    DCHECK_CURRENTLY_ON(BrowserThread::IO);
    return pending_store_updates_;
  }
  void decrement_pending_store_updates() {
    DCHECK_CURRENTLY_ON(BrowserThread::IO);
    pending_store_updates_--;
    if (!pending_store_updates_) {
      db_updated_callback_.Run();
    }
  }

It's a bit heavyweight and redundant, but it does enforce your invariant.

OTOH, it is a bit heavyweight to read!  I think having the comment on the
variable declaration is better, and it's not like this is a general-use class.

[vakh (use Gerrit instead), 2016/06/21 23:19:35]

Acknowledged. I've moved the comment next to the variable declaration.
Yes, I agree that it is a bit heavyweight to read, specially when this is for a
private variable so any code can already read/modify it without having to use
those interfaces.

+  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();
+  }
+}
+
 }  // namespace safe_browsing