More implementation details of extension content verification

More implementation details of extension content verification

-Added classes for fetching/reading file hashes (.cc details are still
  mostly stubs in this CL though)
-Added a delegate class for ContentVerifier to rework how clients interact
  with it
-Added code in ContentVerifyJob that uses the hash reader class

BUG=369895
R=rockot@chromium.org

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

[Ken Rockot(use gerrit already), 2014-05-09 23:51:15]

LGTM but some questions & comments

https://codereview.chromium.org/280013003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/280013003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:132: public
extensions::ContentVerifierDelegate {
If I could have one wish, just one wish, it would be that nobody ever added any
more responsibility to ExtensionService.

Any chance this delegation could be done by a new object? Like some kind of
ContentVerificationDelegateThatDisablesExtensionsWhichFailVerification that
ExtensionSystemImpl owns? Maybe with a slightly shorter name than that.

https://codereview.chromium.org/280013003/diff/20001/extensions/browser/conte...
File extensions/browser/content_verify_job.h (right):

https://codereview.chromium.org/280013003/diff/20001/extensions/browser/conte...
extensions/browser/content_verify_job.h:46: ContentVerifyJob(ContentHashReader*
hash_reader,
This puzzled me a little bit - it seems like ContentVerifyJob does asynchronous
work but only calls back somewhere on failure? Is it just that callers should
never care about the successful case (because the job just goes off and has side
effects and that's that?)

[asargent_no_longer_on_chrome, 2014-05-10 00:12:43]

https://codereview.chromium.org/280013003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/280013003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.h:132: public
extensions::ContentVerifierDelegate {
On 2014/05/09 23:51:16, Ken Rockot wrote:
> If I could have one wish, just one wish, it would be that nobody ever added
any
> more responsibility to ExtensionService.
> 
> Any chance this delegation could be done by a new object? Like some kind of
> ContentVerificationDelegateThatDisablesExtensionsWhichFailVerification that
> ExtensionSystemImpl owns? Maybe with a slightly shorter name than that.

=)

Funny story: I originally didn't have ExtensionService involved with this code
at all because I was afraid of the dumping ground effect, and *yoz* suggested I
move it here in this comment on my earlier CL:

https://codereview.chromium.org/266963003/diff/40001/chrome/browser/extension...


Anyhow, when it was just that one function it seemed ok, but now that the
interface has grown I think you're right. I'll try moving this into an object
inside ExtensionSystemImpl.

https://codereview.chromium.org/280013003/diff/20001/extensions/browser/conte...
File extensions/browser/content_verify_job.h (right):

https://codereview.chromium.org/280013003/diff/20001/extensions/browser/conte...
extensions/browser/content_verify_job.h:46: ContentVerifyJob(ContentHashReader*
hash_reader,
On 2014/05/09 23:51:16, Ken Rockot wrote:
> This puzzled me a little bit - it seems like ContentVerifyJob does
asynchronous
> work but only calls back somewhere on failure? Is it just that callers should
> never care about the successful case (because the job just goes off and has
side
> effects and that's that?)

Yeah, right now we're asynchronously checking the bytes as they are being read
off the disk, and disabling the extension if any mismatches are detected. This
is because I haven't had time to do speed optimizations yet and was afraid of
dramatically slowing down extension performance if I made the file reading in
extension_protocols.cc block while checking the hashes. Eventually we'd like to
get to the point that we can do the checking inline and avoid ever serving up
"bad" bytes.

[asargent_no_longer_on_chrome, 2014-05-10 03:36:23]

Ok, moved to having a little standalone delegate impl class inside
extension_system_impl.cc.

[asargent_no_longer_on_chrome, 2014-05-10 03:36:28]

The CQ bit was checked by asargent@chromium.org

[commit-bot: I haz the power, 2014-05-10 03:39:05]

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

[asargent_no_longer_on_chrome, 2014-05-10 05:51:13]

The CQ bit was checked by asargent@chromium.org

[commit-bot: I haz the power, 2014-05-10 05:52:47]

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

[asargent_no_longer_on_chrome, 2014-05-10 20:44:44]

Committed patchset #3 manually as r269633 (presubmit successful).