Fix for 0-length file problem in content verification

Fix for 0-length file problem in extension content verification

The problem is that we were generating zero hashes when there
were no input bytes, when we should have generated one.

To make this fix a little easier to test, I moved the code for
generating hashes of content into a utility funciton in
computed_hashes.{h,cc}. Also, in order to fix things for any
profiles that might have incorrect computed_hashes.json files, I
changed the format of it to add the notion of a version. When the
format or version isn't recognized, the code will return an error
from the Init function, and then the code that uses it elsewhere
in the content verification code will automatically try to
recreate it.

BUG=399251
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287740

[Ken Rockot(use gerrit already), 2014-08-05 23:10:32]

lgtm

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
File extensions/browser/computed_hashes.cc (right):

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:24: }
nit: It's still a pretty small block, but perhaps as it grows it's worth adding
the closing  // namespace

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:29: }
nit on existing code: Missing blank line

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:112: }
nit on existing code: Missing blank line.

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:160: DCHECK(bytes_to_read >= 0);
Sort of an invalid DCHECK because a size_t is always non-negative.

How about instead DCHECKing before the std::min that offset <= contents.size()?

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:166: std::string* buffer =
&(hashes->back());
Can you not use a reference here instead? Or is that discouraged for some
reason?

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:170: if (bytes_to_read == 0)
nit: Might want to comment to make this escape more explicit (specifically that
it's only reachable if this is the first block).

It took me a second read to understand why this function wouldn't always output
a final empty-block-hash regardless of |contents|.

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:172: else
nit: no need for the else

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
File extensions/browser/computed_hashes_unittest.cc (right):

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes_unittest.cc:15: std::string
Base64Encode(std::string data) {
nit: Might as well pass |data| as a const ref?

[asargent_no_longer_on_chrome, 2014-08-06 04:47:33]

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
File extensions/browser/computed_hashes.cc (right):

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:24: }
On 2014/08/05 23:10:32, Ken Rockot wrote:
> nit: It's still a pretty small block, but perhaps as it grows it's worth
adding
> the closing  // namespace 

Done.

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:29: }
On 2014/08/05 23:10:32, Ken Rockot wrote:
> nit on existing code: Missing blank line

Done.

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:112: }
On 2014/08/05 23:10:32, Ken Rockot wrote:
> nit on existing code: Missing blank line.

Done.

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:160: DCHECK(bytes_to_read >= 0);
On 2014/08/05 23:10:32, Ken Rockot wrote:
> Sort of an invalid DCHECK because a size_t is always non-negative.
> 
> How about instead DCHECKing before the std::min that offset <=
contents.size()?

Ah, good catch. Done.

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:166: std::string* buffer =
&(hashes->back());
On 2014/08/05 23:10:32, Ken Rockot wrote:
> Can you not use a reference here instead? Or is that discouraged for some
> reason?

I don't think there's a rule about it one way or another. I guess I'm in the
habit of using pointers in these kinds of situations to avoid accidentally
making a copy, and also I need to use a pointer to it below on line 168.

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:170: if (bytes_to_read == 0)
On 2014/08/05 23:10:32, Ken Rockot wrote:
> nit: Might want to comment to make this escape more explicit (specifically
that
> it's only reachable if this is the first block).
> 
> It took me a second read to understand why this function wouldn't always
output
> a final empty-block-hash regardless of |contents|.

Done.

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes.cc:172: else
On 2014/08/05 23:10:32, Ken Rockot wrote:
> nit: no need for the else

Done.

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
File extensions/browser/computed_hashes_unittest.cc (right):

https://codereview.chromium.org/436563004/diff/40001/extensions/browser/compu...
extensions/browser/computed_hashes_unittest.cc:15: std::string
Base64Encode(std::string data) {
On 2014/08/05 23:10:32, Ken Rockot wrote:
> nit: Might as well pass |data| as a const ref?

Good catch, done.

[asargent_no_longer_on_chrome, 2014-08-06 04:47:43]

The CQ bit was checked by asargent@chromium.org

[commit-bot: I haz the power, 2014-08-06 04:50:31]

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

[commit-bot: I haz the power, 2014-08-06 09:13:44]

Change committed as 287740