PVer4: Test checksum on startup outside the hotpath of DB load

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 476c0588702ceb07dc33df31304f16925ed589e2..9e63faad458f50e851c58ffd9cfcfc750c53f4d0 100644
--- a/components/safe_browsing_db/v4_store.cc
+++ b/components/safe_browsing_db/v4_store.cc
@@ -130,7 +130,7 @@ std::string V4Store::DebugString() const {
 }
 
 bool V4Store::Reset() {
-  // TODO(vakh): Implement skeleton.
+  hash_prefix_map_.clear();

[Nathan Parker, 2016/10/03 02:26:49]

This should delete the file too, according to the .h file.  And just return
void, since there's no possibility of another value.

[vakh (use Gerrit instead), 2016/10/04 07:14:39]

Done. Don't need to delete the file since it would be over-written on next
update.

   state_ = "";
   return true;
 }
@@ -409,7 +409,25 @@ ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map,
                                        const HashPrefixMap& additions_map,
                                        const RepeatedField<int32>* raw_removals,
                                        const std::string& expected_checksum) {
+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
   DCHECK(hash_prefix_map_.empty());
+
+  bool calculate_checksum = !expected_checksum.empty();
+  if (old_prefixes_map.empty()) {
+    // If the old map is empty, which it is at startup, then just copy over the
+    // additions map.
+    DCHECK(!raw_removals);
+    hash_prefix_map_ = additions_map;
+
+    if (calculate_checksum) {
+      task_runner_->PostTask(

[Nathan Parker, 2016/10/03 02:26:49]

Add a comment to why this is not run synchronously.

[vakh (use Gerrit instead), 2016/10/04 07:14:39]

Done.

+          FROM_HERE, base::Bind(&V4Store::VerifyChecksum,

[Nathan Parker, 2016/10/03 02:26:49]

How many CPU seconds will this take, when it does run?  If it's substantian
(>100's of ms of CPU), we really should put it off till after startup is really
done.  Otherwise, it'll contend with other startup tasks for CPU time.

You could post it with a 5 minute delay.

[vakh (use Gerrit instead), 2016/10/04 07:14:39]

It takes 2-3 seconds on my machine.
That's a good point but it shouldn't contend with other important tasks because
it is scheduled on the task runner, which by design, is for less critical tasks.
All important tasks should be scheduled on the IO thread, which gets preference
over task runner.
[I ran an experiment and scheduled a task on the IO thread and another on the
task runner in MergeUpdate. The one on IO thread consistently ran much earlier
than the one on task runner].
As I explained above, this should not be necessary if the checksum verification
is scheduled on the task runner.

+                                base::Unretained(this), expected_checksum));
+    }
+
+    return APPLY_UPDATE_SUCCESS;
+  }
+
   hash_prefix_map_.clear();
   ReserveSpaceInPrefixMap(old_prefixes_map, &hash_prefix_map_);
   ReserveSpaceInPrefixMap(additions_map, &hash_prefix_map_);
@@ -431,7 +449,6 @@ ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map,
   // At least one of the maps still has elements that need to be merged into the
   // new store.
 
-  bool calculate_checksum = !expected_checksum.empty();
   std::unique_ptr<crypto::SecureHash> checksum_ctx(
       crypto::SecureHash::Create(crypto::SecureHash::SHA256));
 
@@ -461,7 +478,7 @@ ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map,
       next_smallest_prefix_size = next_smallest_prefix_old.size();
 
       // Update the iterator map, which means that we have merged one hash
-      // prefix of size |next_size_for_old| from the old store.
+      // prefix of size |next_smallest_prefix_size| from the old store.
       old_iterator_map[next_smallest_prefix_size] += next_smallest_prefix_size;
 
       if (!raw_removals || removals_iter == raw_removals->end() ||
@@ -520,7 +537,9 @@ ApplyUpdateResult V4Store::MergeUpdate(const HashPrefixMap& old_prefixes_map,
       base::Base64Encode(checksum, &checksum_base64);
       base::Base64Encode(expected_checksum, &expected_checksum_base64);
       DVLOG(1) << "Failure: Checksum mismatch: calculated: " << checksum_base64
-               << " expected: " << expected_checksum_base64;
+               << "; expected: " << expected_checksum_base64
+               << "; store: " << *this;
+      ;
       return CHECKSUM_MISMATCH_FAILURE;
     }
   }
@@ -650,4 +669,48 @@ bool V4Store::HashPrefixMatches(const HashPrefix& hash_prefix,
   }
 }
 
+void V4Store::VerifyChecksum(const std::string& expected_checksum) {

[Nathan Parker, 2016/10/03 02:26:49]

Maybe call this VerifyChecksumDelayed, so as to differentiate it from the
checksum check above.

[vakh (use Gerrit instead), 2016/10/04 07:14:39]

Done.

+  DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+  IteratorMap iterator_map;
+  HashPrefix next_smallest_prefix;
+  InitializeIteratorMap(hash_prefix_map_, &iterator_map);
+  bool has_unmerged = GetNextSmallestUnmergedPrefix(
+      hash_prefix_map_, iterator_map, &next_smallest_prefix);
+
+  std::unique_ptr<crypto::SecureHash> checksum_ctx(
+      crypto::SecureHash::Create(crypto::SecureHash::SHA256));
+  while (has_unmerged) {
+    PrefixSize next_smallest_prefix_size;
+    next_smallest_prefix_size = next_smallest_prefix.size();
+
+    // Update the iterator map, which means that we have read one hash
+    // prefix of size |next_smallest_prefix_size| from hash_prefix_map_.
+    iterator_map[next_smallest_prefix_size] += next_smallest_prefix_size;
+
+    checksum_ctx->Update(base::string_as_array(&next_smallest_prefix),
+                         next_smallest_prefix_size);
+
+    // Find the next smallest unmerged element in the map.
+    has_unmerged = GetNextSmallestUnmergedPrefix(hash_prefix_map_, iterator_map,
+                                                 &next_smallest_prefix);
+  }
+
+  std::string checksum(crypto::kSHA256Length, 0);
+  checksum_ctx->Finish(base::string_as_array(&checksum), checksum.size());
+  if (checksum == expected_checksum) {
+    return;
+  }
+
+  std::string checksum_base64, expected_checksum_base64;
+  base::Base64Encode(checksum, &checksum_base64);
+  base::Base64Encode(expected_checksum, &expected_checksum_base64);
+  DVLOG(1) << "Failure: Checksum mismatch: calculated: " << checksum_base64
+           << "; expected: " << expected_checksum_base64
+           << "; store: " << *this;
+  RecordApplyUpdateResultWhenReadingFromDisk(CHECKSUM_MISMATCH_FAILURE);

[Nathan Parker, 2016/10/03 02:26:49]

This should probably be a different enum, since checksum failures due to disk
corruption are quite different than those from bad merges.

[vakh (use Gerrit instead), 2016/10/04 07:14:39]

Yes, but it is different. This one is:
RecordApplyUpdateResultWhenReadingFromDisk. The other one is logged using:
RecordApplyUpdateResult

+
+  Reset();

[Nathan Parker, 2016/10/03 02:26:49]

This is a race if someone is accessing the store on the IO thread right now. 
Maybe Reset should post to the IO thread to do the clearing.

[vakh (use Gerrit instead), 2016/10/04 07:14:39]

Done.

+}
+
 }  // namespace safe_browsing