SafeBrowsing PVer4: Send mutable response to the database and the stores

components/safe_browsing_db/v4_database.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <memory>
 
 #include "base/callback.h"
 #include "base/debug/leak_annotations.h"
 #include "base/files/file_util.h"
 #include "base/memory/ptr_util.h"
@@ skipping 74 matching lines @@
   V4Database* v4_database_raw = v4_database.release();
   if (v4_database_raw) {
     v4_database_raw->db_task_runner_->DeleteSoon(FROM_HERE, v4_database_raw);
   }
 }
 
 V4Database::~V4Database() {
   DCHECK(db_task_runner_->RunsTasksOnCurrentThread());
 }
 
-void V4Database::ApplyUpdate(const std::vector<ListUpdateResponse>& responses,
-                             DatabaseUpdatedCallback db_updated_callback) {
+void V4Database::ApplyUpdate(
+    std::unique_ptr<ParsedServerResponse> parsed_server_response,
+    DatabaseUpdatedCallback db_updated_callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DCHECK(!pending_store_updates_);
   DCHECK(db_updated_callback_.is_null());
 
   db_updated_callback_ = db_updated_callback;
 
   // Post the V4Store update task on the task runner but get the callback on the
   // current thread.
   const scoped_refptr<base::SingleThreadTaskRunner>& current_task_runner =
       base::MessageLoop::current()->task_runner();
-  for (const ListUpdateResponse& response : responses) {
-    UpdateListIdentifier identifier(response);
+  for (std::unique_ptr<ListUpdateResponse>& response :

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

I'm not entirely comfortable with the non-const &, here.  I'm not entirely sure
where the style guide covers this case, either to allow or disallow.  Does not
having it be a reference still steal the values as it iterates?  If so, I think
that would be preferable (as-is, I think the std::move(response) line actually
is where the value moves out of the collection).

[Nathan Parker, 2016/06/28 19:00:37]

I think for(..:..) loops by default copy the elements, so if copying the
unique_ptr actually does a move(), then just removing the '&' would do the
trick.  If that doesn't work, then the non-const-ref seems like the next
cleanest.

[vakh (use Gerrit instead), 2016/06/28 21:34:14]

Removing the "&" also leads to a compile-time error:
../../components/safe_browsing_db/v4_database.cc:108:50: error: call to deleted
constructor of 'const std::unique_ptr<ListUpdateResponse>' (aka 'const
unique_ptr<safe_browsing::FetchThreatListUpdatesResponse_ListUpdateResponse>')

[vakh (use Gerrit instead), 2016/06/28 21:34:14]

1. Adding constness leads to a compile-time error:
../../base/bind_helpers.h:419:13: error: 'mutable' and 'const' cannot be mixed

2. There's lots of precedent:
https://cs.chromium.org/search/?q=%22for+(std::unique_ptr%22&sq=package:chrom...

[Scott Hess - ex-Googler, 2016/06/28 22:22:25]

Apologies - that's expected, because you need to modify it, I think the const
will break both the ways you mentioned.  I was meaning that if it's a reference,
it should be const, but if it can't be const, it shouldn't be a reference
(that's my interpretation of the style discussions I recall).
To me that looks like a mixture of unique_ptr and unique_ptr&, which implies
that unique_ptr would work fine on its own (non-const, non-&).

My target, here, is that unique_ptr& says that there's a unique_ptr in the
response, and this is a reference to it, whereas unique_ptr says that _this_ is
the owning pointer.  The difference (I think) is when the referenced data will
be unwound.  As a reference, it will be released either at the std::move(), or
when the function exits (if the various if statements were false, leaving the
element in the container).  As a plain non-reference unique_ptr, it will always
be either forwarded or released on each iteration, so the container will always
be gradually emptying.

I feel like pulling the elements out of the container one-by-one is more
intuitive, but if you think using the ref is clearer, I'll defer to that.

[vakh (use Gerrit instead), 2016/06/29 19:58:52]

I am going ahead with the current implementation for the following reasons:
1. Can't use 'const' since I need to modify this proto later.
2. Can't drop '&' since it leads to a compile-time error. See above.
3. The object does get moved and ownership passed even with non-const reference.
I added the following logs:
[ RUN      ] V4DatabaseTest.TestApplyUpdateWithNewStates
[120220:120220:0629/123925:618849993560:VERBOSE1:v4_database.cc(119)] response
(before): 0x1e0c71c2ed30
[120220:120220:0629/123925:618849993621:VERBOSE1:v4_database.cc(125)] response
(after): 0
[120220:120220:0629/123925:618849993656:VERBOSE1:v4_database.cc(119)] response
(before): 0x1e0c71c2e180
[120220:120220:0629/123925:618849993687:VERBOSE1:v4_database.cc(125)] response
(after): 0

[Scott Hess - ex-Googler, 2016/06/29 20:15:58]

Acknowledged.  I wonder where we'd be if move semantics were _not_ intuitive?

+       *parsed_server_response) {
+    UpdateListIdentifier identifier(response.get());
     StoreMap::const_iterator iter = store_map_->find(identifier);
     if (iter != store_map_->end()) {
       const std::unique_ptr<V4Store>& old_store = iter->second;
-      if (old_store->state() != response.new_client_state()) {
+      if (old_store->state() != response->new_client_state()) {
         // 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));
+                       base::Passed(std::move(response)), current_task_runner,
+                       store_ready_callback));
       }
     } else {
       NOTREACHED() << "Got update for unexpected identifier: " << identifier;
     }
   }
 
   if (!pending_store_updates_) {
     current_task_runner->PostTask(FROM_HERE, db_updated_callback_);
     db_updated_callback_.Reset();
   }
@@ skipping 26 matching lines @@
 std::unique_ptr<StoreStateMap> V4Database::GetStoreStateMap() {
   std::unique_ptr<StoreStateMap> store_state_map =
       base::MakeUnique<StoreStateMap>();
   for (const auto& store_map_iter : *store_map_) {
     (*store_state_map)[store_map_iter.first] = store_map_iter.second->state();
   }
   return store_state_map;
 }
 
 }  // namespace safe_browsing