Fix several problems with the content verification code

extensions/browser/content_verify_job.cc

 // Copyright 2014 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 "extensions/browser/content_verify_job.h"
 
 #include <algorithm>
 
 #include "base/stl_util.h"
 #include "base/task_runner_util.h"
@@ skipping 31 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
   base::PostTaskAndReplyWithResult(
       content::BrowserThread::GetBlockingPool(),
       FROM_HERE,
       base::Bind(&ContentHashReader::Init, hash_reader_),
       base::Bind(&ContentVerifyJob::OnHashesReady, this));
 }
 
 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);
     if (reason != NONE)
       return DispatchFailureCallback(reason);
   }
   if (!hashes_ready_) {
     queue_.append(data, count);
     return;
   }
   DCHECK_GE(count, 0);
   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);
 
     if (!current_hash_.get()) {
       current_hash_byte_count_ = 0;
       current_hash_.reset(
           crypto::SecureHash::Create(crypto::SecureHash::SHA256));
     }
     // Compute how many bytes we should hash, and add them to the current hash.
     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;
     total_bytes_read_ += bytes_to_hash;
 
     // If we finished reading a block worth of data, finish computing the hash
     // for it and make sure the expected hash matches.
     if (current_hash_byte_count_ == hash_reader_->block_size())
       FinishBlock();
   }
 }
 
 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());
     if (reason != NONE) {
       DispatchFailureCallback(reason);
       return;
     }
   }
   done_reading_ = true;
   if (hashes_ready_)
     FinishBlock();
 }
 
 void ContentVerifyJob::FinishBlock() {
   if (current_hash_byte_count_ <= 0)
     return;
   std::string final(crypto::kSHA256Length, 0);
   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;
   current_block_++;
 }
 
 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()) {
     std::string tmp;
     queue_.swap(tmp);
     BytesRead(tmp.size(), string_as_array(&tmp));
   }
   if (done_reading_)
     FinishBlock();
 }
 
 // static
 void ContentVerifyJob::SetDelegateForTests(TestDelegate* delegate) {
   g_test_delegate = 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();
   }
 }
 
 }  // namespace extensions