Fix several problems with the content verification code

extensions/browser/content_verify_job.cc

Index: extensions/browser/content_verify_job.cc
diff --git a/extensions/browser/content_verify_job.cc b/extensions/browser/content_verify_job.cc
index 5eec659d331fc24881aeedfe8dc2b5101f96cd0b..299d936baf2939d38e13bdf2e02bbddc1c4e8dc4 100644
--- a/extensions/browser/content_verify_job.cc
+++ b/extensions/browser/content_verify_job.cc
@@ -49,6 +49,8 @@ void ContentVerifyJob::Start() {
 
 void ContentVerifyJob::BytesRead(int count, const char* data) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (failure_callback_.is_null())

[Ken Rockot(use gerrit already), 2014/06/17 16:43:43]

I think it would be easier to understand if you just had an explicit failed_
flag in ContentVerifyJob, which could be set by DispatchFailureCallback. It took
me a while to convince myself that this is what's really meant by
failure_callback_.is_null() being true.

[asargent_no_longer_on_chrome, 2014/06/18 00:13:51]

Good suggestion. Done. 

+    return;
   if (g_test_delegate) {
     FailureReason reason =
         g_test_delegate->BytesRead(hash_reader_->extension_id(), count, data);
@@ -63,6 +65,9 @@ void ContentVerifyJob::BytesRead(int count, const char* data) {
   int bytes_added = 0;
 
   while (bytes_added < count) {
+    if (failure_callback_.is_null())

[Ken Rockot(use gerrit already), 2014/06/17 16:43:43]

In this specific case, if FinishBlock() is the only thing that can lead to
failure within this loop and it only has one failure mode (hash mismatch), could
you just have it return a bool success indicator? Then there's no need to test a
hard-to-follow failure side effect. Something like

    [kill the failure_callback_.is_null() test]
    
    ... all this intermediate code up to line 91...
    
    if (current_hash_byte_count_ == hash_reader_->block_size() &&
        !FinishBlock()) {
      return DispatchFailureCallback(HASH_MISMATCH);
    }

+      return;
+
     if (current_block_ >= hash_reader_->block_count())
       return DispatchFailureCallback(HASH_MISMATCH);
 
@@ -75,6 +80,7 @@ void ContentVerifyJob::BytesRead(int count, const char* data) {
     int bytes_to_hash =
         std::min(hash_reader_->block_size() - current_hash_byte_count_,
                  count - bytes_added);
+    DCHECK(bytes_to_hash > 0);
     current_hash_->Update(data + bytes_added, bytes_to_hash);
     bytes_added += bytes_to_hash;
     current_hash_byte_count_ += bytes_to_hash;
@@ -89,6 +95,8 @@ void ContentVerifyJob::BytesRead(int count, const char* data) {
 
 void ContentVerifyJob::DoneReading() {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (failure_callback_.is_null())
+    return;
   if (g_test_delegate) {
     FailureReason reason =
         g_test_delegate->DoneReading(hash_reader_->extension_id());
@@ -109,11 +117,9 @@ void ContentVerifyJob::FinishBlock() {
   current_hash_->Finish(string_as_array(&final), final.size());
 
   const std::string* expected_hash = NULL;
-  if (!hash_reader_->GetHashForBlock(current_block_, &expected_hash))
-    return DispatchFailureCallback(HASH_MISMATCH);
-
-  if (*expected_hash != final)
-    return DispatchFailureCallback(HASH_MISMATCH);
+  if (!hash_reader_->GetHashForBlock(current_block_, &expected_hash) ||
+      *expected_hash != final)
+    DispatchFailureCallback(HASH_MISMATCH);
 
   current_hash_.reset();
   current_hash_byte_count_ = 0;
@@ -121,8 +127,14 @@ void ContentVerifyJob::FinishBlock() {
 }
 
 void ContentVerifyJob::OnHashesReady(bool success) {
-  if (!success && !g_test_delegate)
-    return DispatchFailureCallback(NO_HASHES);
+  if (!success && !g_test_delegate) {
+    if (hash_reader_->have_verified_contents() &&
+        hash_reader_->have_computed_hashes())
+      DispatchFailureCallback(NO_HASHES_FOR_FILE);
+    else
+      DispatchFailureCallback(MISSING_ALL_HASHES);
+    return;
+  }
 
   hashes_ready_ = true;
   if (!queue_.empty()) {
@@ -141,6 +153,9 @@ void ContentVerifyJob::SetDelegateForTests(TestDelegate* delegate) {
 
 void ContentVerifyJob::DispatchFailureCallback(FailureReason reason) {
   if (!failure_callback_.is_null()) {
+    VLOG(1) << "job failed for " << hash_reader_->extension_id() << " "
+            << hash_reader_->relative_path().MaybeAsASCII()
+            << " reason:" << reason;
     failure_callback_.Run(reason);
     failure_callback_.Reset();
   }