Fix several problems with the content verification code

Fix several problems with the content verification code

-While we're doing the initial scan of files to build up
 computed_hashes.json, if we notice a mismatch, report that right
 away. This also fixes problems where if you modify/delete
 computed_hashes.json, enforcement would never work.

-Avoid an infinite loop in ContentVerifyJob by making sure we stop
 checking newly read bytes after we've already reported a failure
 (we were stuck failing to advance to the next block after reporting
  a failure)

BUG=375992, 375953
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278071

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

Mostly good, just a few questions/comments.

https://codereview.chromium.org/329303007/diff/60001/extensions/browser/conte...
File extensions/browser/content_hash_fetcher.cc (right):

https://codereview.chromium.org/329303007/diff/60001/extensions/browser/conte...
extensions/browser/content_hash_fetcher.cc:475: jobs_.erase(found);
What if there are two jobs matching key now, one with force=true and one with
force=false? Is such a thing even possible?

https://codereview.chromium.org/329303007/diff/60001/extensions/browser/conte...
File extensions/browser/content_verify_job.cc (right):

https://codereview.chromium.org/329303007/diff/60001/extensions/browser/conte...
extensions/browser/content_verify_job.cc:52: if (failure_callback_.is_null())
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.

https://codereview.chromium.org/329303007/diff/60001/extensions/browser/conte...
extensions/browser/content_verify_job.cc:68: if (failure_callback_.is_null())
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);
    }

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

PTAL

https://codereview.chromium.org/329303007/diff/60001/extensions/browser/conte...
File extensions/browser/content_hash_fetcher.cc (right):

https://codereview.chromium.org/329303007/diff/60001/extensions/browser/conte...
extensions/browser/content_hash_fetcher.cc:475: jobs_.erase(found);
On 2014/06/17 16:43:43, Ken Rockot wrote:
> What if there are two jobs matching key now, one with force=true and one with
> force=false? Is such a thing even possible?

Ah, good question! Up at line 436 of this patch set I was trying to ensure that
we would remove a force=false job if we needed to (by the fact that we'd replace
its entry in the map, decrementing its ref count), but had the logic slightly
wrong and also I realized I wasn't explicitly cancelling it either there or
here, so it was possible that it could still progress to causing the callback to
fire which isn't desirable. 

I have fixed this in the new patch set.

https://codereview.chromium.org/329303007/diff/60001/extensions/browser/conte...
File extensions/browser/content_verify_job.cc (right):

https://codereview.chromium.org/329303007/diff/60001/extensions/browser/conte...
extensions/browser/content_verify_job.cc:52: if (failure_callback_.is_null())
On 2014/06/17 16:43:43, Ken Rockot wrote:
> 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.

Good suggestion. Done.

[Ken Rockot(use gerrit already), 2014-06-18 00:22:49]

lgtm

[asargent_no_longer_on_chrome, 2014-06-18 05:10:36]

The CQ bit was checked by asargent@chromium.org

[commit-bot: I haz the power, 2014-06-18 05:13:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/asargent@chromium.org/329303007/80001

[commit-bot: I haz the power, 2014-06-18 15:05:10]

Change committed as 278071