Small: Serialize the store's hash_prefix_map_ to file in WriteToDisk()

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/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
@@ skipping 201 matching lines @@
 
 ApplyUpdateResult V4Store::ProcessPartialUpdateAndWriteToDisk(
     const std::string& metric,
     const HashPrefixMap& hash_prefix_map_old,
     std::unique_ptr<ListUpdateResponse> response) {
   DCHECK(response->has_response_type());
   DCHECK_EQ(ListUpdateResponse::PARTIAL_UPDATE, response->response_type());
 
   ApplyUpdateResult result =
       ProcessUpdate(metric, hash_prefix_map_old, response);
+  if (result == APPLY_UPDATE_SUCCESS) {
+    Checksum checksum = response->checksum();
+    response.reset();
+    RecordStoreWriteResult(WriteToDisk(checksum));
+  }
   return result;
 }
 
 ApplyUpdateResult V4Store::ProcessFullUpdateAndWriteToDisk(
     const std::string& metric,
     std::unique_ptr<ListUpdateResponse> response) {
   ApplyUpdateResult result = ProcessFullUpdate(metric, response);
   if (result == APPLY_UPDATE_SUCCESS) {
-    RecordStoreWriteResult(WriteToDisk(std::move(response)));
+    Checksum checksum = response->checksum();
+    response.reset();
+    RecordStoreWriteResult(WriteToDisk(checksum));
   }
   return result;
 }
 
 ApplyUpdateResult V4Store::ProcessFullUpdate(
     const std::string& metric,
     const std::unique_ptr<ListUpdateResponse>& response) {
   DCHECK(response->has_response_type());
   DCHECK_EQ(ListUpdateResponse::FULL_UPDATE, response->response_type());
   // TODO(vakh): For a full update, we don't need to process the update in
@@ skipping 394 matching lines @@
   }
 
   if (contents.empty()) {
     return FILE_EMPTY_FAILURE;
   }
 
   V4StoreFileFormat file_format;
   if (!file_format.ParseFromString(contents)) {
     return PROTO_PARSING_FAILURE;
   }
+  contents.clear();

[Nathan Parker, 2016/10/14 00:10:05]

I think this doesn't actually free memory. You probably want to call
shrink_to_fit() after, or else declare the string as a unique_ptr so you can
reset it.

[Scott Hess - ex-Googler, 2016/10/14 00:16:18]

The canonical way to release string memory is something like
std::string().swap(contents), though something horrible like
ignore_result(std::move(contents)) would probably work, too.  Of course you
could just scope it appropriately, albeit at some cost to rearrange your
variable declarations.

Also IMHO, don't unique_ptr it.  It works, but it's not much work to avoid the
extra heap allocation.

 
   if (file_format.magic_number() != kFileMagic) {
     return UNEXPECTED_MAGIC_NUMBER_FAILURE;
   }
 
   UMA_HISTOGRAM_SPARSE_SLOWLY("SafeBrowsing.V4StoreVersionRead",
                               file_format.version_number());
   if (file_format.version_number() != kFileVersion) {
     return FILE_VERSION_INCOMPATIBLE_FAILURE;
   }
 
   if (!file_format.has_list_update_response()) {
     return HASH_PREFIX_INFO_MISSING_FAILURE;
   }
 
   std::unique_ptr<ListUpdateResponse> response(new ListUpdateResponse);
   response->Swap(file_format.mutable_list_update_response());
   ApplyUpdateResult apply_update_result =
       ProcessFullUpdate(kReadFromDisk, response);
   RecordApplyUpdateResult(kReadFromDisk, apply_update_result, store_path_);
   if (apply_update_result != APPLY_UPDATE_SUCCESS) {
     hash_prefix_map_.clear();
     return HASH_PREFIX_MAP_GENERATION_FAILURE;
   }
   RecordApplyUpdateTime(kReadFromDisk, TimeTicks::Now() - before, store_path_);
 
   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) {
-    DVLOG(1) << "Failure: response->has_response_type(): "
-             << response->has_response_type()
-             << " : response->response_type(): " << response->response_type();
-    return INVALID_RESPONSE_TYPE_FAILURE;
+StoreWriteResult V4Store::WriteToDisk(const Checksum& checksum) const {
+  V4StoreFileFormat file_format;
+  ListUpdateResponse* lur = file_format.mutable_list_update_response();
+  *(lur->mutable_checksum()) = checksum;
+  lur->set_new_client_state(state_);
+  lur->set_response_type(ListUpdateResponse::FULL_UPDATE);
+  for (auto map_iter : hash_prefix_map_) {
+    ThreatEntrySet* additions = lur->add_additions();
+    // TODO(vakh): Write RICE encoded hash prefixes on disk. Not doing so
+    // currently since it takes a long time to decode them on startup, which
+    // blocks resource load. See: http://crbug.com/654819
+    additions->set_compression_type(RAW);
+    additions->mutable_raw_hashes()->set_prefix_size(map_iter.first);
+    additions->mutable_raw_hashes()->set_raw_hashes(map_iter.second);
   }
 
   // Attempt writing to a temporary file first and at the end, swap the files.
   const base::FilePath new_filename = TemporaryFileForFilename(store_path_);
 
-  V4StoreFileFormat file_format;
   file_format.set_magic_number(kFileMagic);
   file_format.set_version_number(kFileVersion);
-  ListUpdateResponse* response_to_write =
-      file_format.mutable_list_update_response();
-  response_to_write->Swap(response.get());
   std::string file_format_string;
   file_format.SerializeToString(&file_format_string);
   size_t written = base::WriteFile(new_filename, file_format_string.data(),
                                    file_format_string.size());
   DCHECK_EQ(file_format_string.size(), written);
 
   if (!base::Move(new_filename, store_path_)) {
     return UNABLE_TO_RENAME_FAILURE;
   }
 
@@ skipping 88 matching lines @@
                << "; expected: " << expected_checksum_b64
                << "; store: " << *this;
 #endif
       return false;
     }
   }
   return true;
 }
 
 }  // namespace safe_browsing