PVer4: DecodeHashes needs to sort the output of the Rice decoder

components/safe_browsing_db/v4_store.cc

Index: components/safe_browsing_db/v4_store.cc
diff --git a/components/safe_browsing_db/v4_store.cc b/components/safe_browsing_db/v4_store.cc
index 45c003f28a2af78a3fe945b2413efcc4ada7821e..3bff9088beb630447233577a86627a45bc1bd4f9 100644
--- a/components/safe_browsing_db/v4_store.cc
+++ b/components/safe_browsing_db/v4_store.cc
@@ -202,6 +202,7 @@ ApplyUpdateResult V4Store::ProcessUpdate(
   }
 
   state_ = response->new_client_state();
+  DVLOG(1) << "Update successful: " << *this;

[Nathan Parker, 2016/08/12 23:01:49]

These are fairly verbose -- You should plan to remove most of them once the code
is stable.

[vakh (use Gerrit instead), 2016/08/13 00:15:14]

Ack. Done.
Also: http://crbug.com/637468

   return APPLY_UPDATE_SUCCESS;
 }
 
@@ -209,6 +210,9 @@ void V4Store::ApplyUpdate(
     std::unique_ptr<ListUpdateResponse> response,
     const scoped_refptr<base::SingleThreadTaskRunner>& callback_task_runner,
     UpdatedStoreReadyCallback callback) {
+  DVLOG(1) << "ApplyUpdate: response_size: " << response->ByteSize() << " : "
+           << *this;
+
   std::unique_ptr<V4Store> new_store(
       new V4Store(this->task_runner_, this->store_path_));
 
@@ -221,7 +225,8 @@ void V4Store::ApplyUpdate(
         new_store->ProcessFullUpdateAndWriteToDisk(std::move(response));
   } else {
     apply_update_result = UNEXPECTED_RESPONSE_TYPE_FAILURE;
-    NOTREACHED() << "Unexpected response type: " << response->response_type();
+    NOTREACHED() << "Failure: Unexpected response type: "
+                 << response->response_type();
   }
 
   if (apply_update_result == APPLY_UPDATE_SUCCESS) {
@@ -229,7 +234,7 @@ void V4Store::ApplyUpdate(
     callback_task_runner->PostTask(
         FROM_HERE, base::Bind(callback, base::Passed(&new_store)));
   } else {
-    DVLOG(1) << "ApplyUpdate failed: reason: " << apply_update_result
+    DVLOG(1) << "Failure: ApplyUpdate: reason: " << apply_update_result
              << "; store: " << *this;
     // new_store failed updating. Pass a nullptr to the callback.
     callback_task_runner->PostTask(FROM_HERE, base::Bind(callback, nullptr));
@@ -301,6 +306,7 @@ ApplyUpdateResult V4Store::AddUnlumpedHashes(PrefixSize prefix_size,
   if (lumped_hashes.size() % prefix_size != 0) {
     return ADDITIONS_SIZE_UNEXPECTED_FAILURE;
   }
+
   // TODO(vakh): Figure out a way to avoid the following copy operation.
   (*additions_map)[prefix_size] = lumped_hashes;
   return APPLY_UPDATE_SUCCESS;
@@ -395,6 +401,7 @@ ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map,
     // list, something is clearly wrong. Discard the update.
     if (old_has_unmerged && additions_has_unmerged &&
         next_smallest_prefix_old == next_smallest_prefix_additions) {
+      DVLOG(1) << "Failure: ADDITIONS_HAS_EXISTING_PREFIX_FAILURE";

[Nathan Parker, 2016/08/12 23:01:49]

Can you log this further up instead?  Just logging the enum # is usually
sufficient, since it should be rarely used.

Same with those below.

[vakh (use Gerrit instead), 2016/08/13 00:15:14]

Done.

       return ADDITIONS_HAS_EXISTING_PREFIX_FAILURE;
     }
 
@@ -457,6 +464,7 @@ ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map,
   }
 
   if (raw_removals && removals_iter != raw_removals->end()) {
+    DVLOG(1) << "Failure: REMOVALS_INDEX_TOO_LARGE_FAILURE";
     return REMOVALS_INDEX_TOO_LARGE_FAILURE;
   }
 
@@ -467,8 +475,8 @@ ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map,
       std::string checksum_base64, expected_checksum_base64;
       base::Base64Encode(checksum, &checksum_base64);
       base::Base64Encode(expected_checksum, &expected_checksum_base64);
-      DVLOG(1) << "Checksum failed: calculated: " << checksum_base64
-               << "expected: " << expected_checksum_base64;
+      DVLOG(1) << "Failure: Checksum mismatch: calculated: " << checksum_base64
+               << " expected: " << expected_checksum_base64;
       return CHECKSUM_MISMATCH_FAILURE;
     }
   }
@@ -495,7 +503,7 @@ StoreReadResult V4Store::ReadFromDisk() {
   }
 
   if (file_format.magic_number() != kFileMagic) {
-    DVLOG(1) << "Unexpected magic number found in file: "
+    DVLOG(1) << "Failure: Unexpected magic number found in file: "

[Nathan Parker, 2016/08/12 23:01:49]

These DVLOGs actually seem like more clutter than they're worth, since they just
mean the file is corrupted.  We don't need to maintain code to tell a developer
which part of the file is corrupted.

Same with file version and response type.  If someday we start getting those
errors, we can easily add a line to log it.

[vakh (use Gerrit instead), 2016/08/13 00:15:14]

Done.

              << file_format.magic_number();
     return UNEXPECTED_MAGIC_NUMBER_FAILURE;
   }
@@ -503,8 +511,8 @@ StoreReadResult V4Store::ReadFromDisk() {
   UMA_HISTOGRAM_SPARSE_SLOWLY("SafeBrowsing.V4StoreVersionRead",
                               file_format.version_number());
   if (file_format.version_number() != kFileVersion) {
-    DVLOG(1) << "File version incompatible: " << file_format.version_number()
-             << "; expected: " << kFileVersion;
+    DVLOG(1) << "Failure: File version incompatible: "
+             << file_format.version_number() << "; expected: " << kFileVersion;
     return FILE_VERSION_INCOMPATIBLE_FAILURE;
   }
 
@@ -531,9 +539,9 @@ StoreWriteResult V4Store::WriteToDisk(
   // should be a FULL_UPDATE.
   if (!response->has_response_type() ||
       response->response_type() != ListUpdateResponse::FULL_UPDATE) {
-    DVLOG(1) << "response->has_response_type(): "
-             << response->has_response_type();
-    DVLOG(1) << "response->response_type(): " << response->response_type();
+    DVLOG(1) << "Failure: response->has_response_type(): "
+             << response->has_response_type()
+             << " : response->response_type(): " << response->response_type();
     return INVALID_RESPONSE_TYPE_FAILURE;
   }