Parse the hash prefix map from the V4Store on disk.

components/safe_browsing_db/v4_store.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 "base/base64.h"
 #include "base/bind.h"
 #include "base/files/file_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/strings/stringprintf.h"
@@ skipping 22 matching lines @@
 void RecordStoreWriteResult(StoreWriteResult result) {
   UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.V4StoreWriteResult", result,
                             STORE_WRITE_RESULT_MAX);
 }
 
 void RecordApplyUpdateResult(ApplyUpdateResult result) {
   UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.V4ApplyUpdateResult", result,
                             APPLY_UPDATE_RESULT_MAX);
 }
 
+void RecordApplyUpdateResultWhenReadingFromDisk(ApplyUpdateResult result) {
+  UMA_HISTOGRAM_ENUMERATION(
+      "SafeBrowsing.V4ApplyUpdateResultWhenReadingFromDisk", result,

[Nathan Parker, 2016/07/14 22:36:12]

Is this really an update?  It's more just a load.

[vakh (use Gerrit instead), 2016/07/15 00:07:30]

Yes, it depends on the naming strategy in the reader's head.
I chose this name because it tells me that the result to expect is an enum
ApplyUpdateResult for the operation WhenReadingFromDisk.

The enum should probably be named differently.

What happens when reading from disk is a subset of what happens when an update
is applied so the enum is named after the super set operation ApplyUpdate.

+      APPLY_UPDATE_RESULT_MAX);
+}
+
 // Returns the name of the temporary file used to buffer data for
 // |filename|.  Exported for unit tests.
 const base::FilePath TemporaryFileForFilename(const base::FilePath& filename) {
   return base::FilePath(filename.value() + FILE_PATH_LITERAL("_new"));
 }
 
 }  // namespace
 
 std::ostream& operator<<(std::ostream& os, const V4Store& store) {
   os << store.DebugString();
@@ skipping 299 matching lines @@
   if (file_format.version_number() != kFileVersion) {
     DVLOG(1) << "File version incompatible: " << file_format.version_number()
              << "; expected: " << kFileVersion;
     return FILE_VERSION_INCOMPATIBLE_FAILURE;
   }
 
   if (!file_format.has_list_update_response()) {
     return HASH_PREFIX_INFO_MISSING_FAILURE;
   }
 
-  ListUpdateResponse list_update_response = file_format.list_update_response();
-  state_ = list_update_response.new_client_state();
-  // TODO(vakh): Do more with what's read from the disk.
+  const ListUpdateResponse& response = file_format.list_update_response();
+  HashPrefixMap read_from_disk_map;
+  ApplyUpdateResult apply_update_result = UpdateHashPrefixMapFromAdditions(

[Nathan Parker, 2016/07/14 22:36:12]

While I think this will work, it does a lot of extra work to look in the current
hash_prefix_map_ for every single hash that's getting loaded.  It would be good
to short-circuit that since we know hash_prefix_map_ is empty and there are no
removals.  This will save a chunk of CPU at startup (where it's most at a
premium).

You might want a separate function here that would just copy the proto's
string(s) straight into the map, assuming they aren't compressed.  If they are
compressed, you could do a streaming-decompress into the string owned by
hash_prefix_map_.

Or, maybe within UpdateHashPrefixMapFromAdditions() you could have
short-circuits to do this: If removals is empty and some hash_prefix_map_
entries are empty, you can just move the corresponding update result string
directly into the hash_prefix_map_. I think that may be more complicated than a
separate function. 


OH and I found some useful proto functions that let you do zero-copy add/remove
of big strings:
  void set_allocated_foo(string* value);
  string* release_foo();

[vakh (use Gerrit instead), 2016/07/14 23:51:57]

Umm, that doesn't happen. I may have misunderstood you so please correct me if
that's the case.
The UpdateHashPrefixMapFromAdditions doesn't look at the hash_prefix_map_ at
all. It is a static method.

All it does is take an instance of additions() and generate a hash_prefix_map
out of it *without merging*.
(Merge is done for partial updates by MergeUpdate() method.)
Could you be confusing UpdateHashPrefixMapFromAdditions with ApplyUpdate() or
MergeUpdate() ?
Thanks. I'll investigate their use and make changes in a separate CL.

[Nathan Parker, 2016/07/15 00:35:31]

Blarg. Indeed, that's exactly how I was reading it. Sorry for the confusion,
ignore that comment.

+      response.additions(), &read_from_disk_map);
+  RecordApplyUpdateResultWhenReadingFromDisk(apply_update_result);
+  if (apply_update_result != APPLY_UPDATE_SUCCESS) {
+    return HASH_PREFIX_MAP_GENERATION_FAILURE;
+  }
 
+  hash_prefix_map_ = read_from_disk_map;

[Nathan Parker, 2016/07/14 22:36:12]

This does a copy of the map.

[vakh (use Gerrit instead), 2016/07/14 23:51:57]

Done.

+  state_ = response.new_client_state();
   return READ_SUCCESS;
 }
 
 StoreWriteResult V4Store::WriteToDisk(
     std::unique_ptr<ListUpdateResponse> response) const {
   // Do not write partial updates to the disk.
   // After merging the updates, the ListUpdateResponse passed to this method
   // should be a FULL_UPDATE.
   if (!response->has_response_type() ||
       response->response_type() != ListUpdateResponse::FULL_UPDATE) {
@@ skipping 20 matching lines @@
 
   if (!base::Move(new_filename, store_path_)) {
     DVLOG(1) << "store_path_: " << store_path_.value();
     return UNABLE_TO_RENAME_FAILURE;
   }
 
   return WRITE_SUCCESS;
 }
 
 }  // namespace safe_browsing