Fix content verification code for undreadable and deleted files.

Fix content verification code for undreadable and deleted files.

It seems that corruption examples in crbug.com/699420 can cause extension
resource files to go missing or become unreadable.
Fix a bug where we never sent any notification to ContentVerifyJob,
in particular: we never called ContentVerifyJob::DoneReading().

Override URLRequestFileJob::OnOpenComplete in URLRequestExtensionJob
to catch these cases.

Also make ContentHashReader not skip legitimate deleted files: the
files that are listed in verified_contents.json. It will still skip
non-existent resources, e.g. xhrs requesting non-existent file (see
crbug.com/404802 for example).

BUG=703888
Test=Install an extension (not app) in chromeos. Chromeos because you want
content verification enforcement to kick in. Other way could be to use
--extension-content-verification=enforce_strict flag in other platform.
Either:
1) Delete background script file. Content verifcation would kick in when
you try to load/run the extension.
2) Same as above^^^, instead of deleting the file, remove the file
permission. In linux you can just do "chmod -r background.js". Expect
content verification failure.
In both cases extension should be reinstalled in the end.

Review-Url: https://codereview.chromium.org/2771953003
Cr-Commit-Position: refs/heads/master@{#460607}
Committed: https://chromium.googlesource.com/chromium/src/+/d6dbb26e31818eed0097654ecb89aaddc2c64f8d

[lazyboy, 2017-03-24 01:15:16]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[lazyboy, 2017-03-24 01:15:36]

lazyboy@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2017-03-24 01:15:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 01:27:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 01:27:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2017-03-24 02:01:45]

(also looks like maybe a compile error on the bots?)

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:61: if
(!base::CreateDirectory(full_path.DirName()))
In practice, shouldn't this be created as part of CreateUniqueTempDir()?

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:134: int count,
Is it worth verifying the bytes read against expectations?

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:153: loop_runner_ =
new content::MessageLoopRunner();
I typically prefer base::RunLoop for this, since the Quit() syntax is a little
easier and more forgiving (e.g., you can call Quit() when the loop isn't
running, etc).  Is there a reason to use MessageLoopRunner here?

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:165:
std::unique_ptr<ExtensionId> waiting_for_extension_id_;
nit: I think base::Optional is a better fit here.

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:185: testing_profile_
= GetTestingProfile();
nitty nit: maybe just inline:
testing_profile_ = TestingProfile::Builder().Build();

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:503:
ContentVerifyJob::SetDelegateForTests(&test_job_delegate);
nit: clean up state; unset this at the end.

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:535: // Valid and
redable foo.js.
*readable

https://codereview.chromium.org/2771953003/diff/20001/extensions/browser/cont...
File extensions/browser/content_verify_job.cc (right):

https://codereview.chromium.org/2771953003/diff/20001/extensions/browser/cont...
extensions/browser/content_verify_job.cc:146: if (!done_reading_ &&
current_hash_byte_count_ <= 0)
unrelated to this cl, but when can current_hash_byte_count_ be < 0?

https://codereview.chromium.org/2771953003/diff/20001/extensions/browser/cont...
extensions/browser/content_verify_job.cc:149: // This happens when we fail to
read the resource, compute empty content's
nitty nit: s/,/;||.

https://codereview.chromium.org/2771953003/diff/20001/extensions/browser/exte...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2771953003/diff/20001/extensions/browser/exte...
extensions/browser/extension_protocols.cc:241: if (result < 0) {
This block would benefit from some comments. :)

[lazyboy, 2017-03-24 18:27:08]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 18:27:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2017-03-24 18:27:48]

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:61: if
(!base::CreateDirectory(full_path.DirName()))
On 2017/03/24 02:01:44, Devlin wrote:
> In practice, shouldn't this be created as part of CreateUniqueTempDir()?

Of course. I copy pasted this function.
Done.

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:134: int count,
On 2017/03/24 02:01:44, Devlin wrote:
> Is it worth verifying the bytes read against expectations?

Not in particular for this test, but doesn't hurt.
Done.

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:153: loop_runner_ =
new content::MessageLoopRunner();
On 2017/03/24 02:01:44, Devlin wrote:
> I typically prefer base::RunLoop for this, since the Quit() syntax is a little
> easier and more forgiving (e.g., you can call Quit() when the loop isn't
> running, etc).  Is there a reason to use MessageLoopRunner here?

Thanks.
Done.

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:165:
std::unique_ptr<ExtensionId> waiting_for_extension_id_;
On 2017/03/24 02:01:44, Devlin wrote:
> nit: I think base::Optional is a better fit here.

Done.

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:185: testing_profile_
= GetTestingProfile();
On 2017/03/24 02:01:44, Devlin wrote:
> nitty nit: maybe just inline:
> testing_profile_ = TestingProfile::Builder().Build();

Done.

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:503:
ContentVerifyJob::SetDelegateForTests(&test_job_delegate);
On 2017/03/24 02:01:44, Devlin wrote:
> nit: clean up state; unset this at the end.

Done.

https://codereview.chromium.org/2771953003/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:535: // Valid and
redable foo.js.
On 2017/03/24 02:01:44, Devlin wrote:
> *readable

Done.

https://codereview.chromium.org/2771953003/diff/20001/extensions/browser/cont...
File extensions/browser/content_verify_job.cc (right):

https://codereview.chromium.org/2771953003/diff/20001/extensions/browser/cont...
extensions/browser/content_verify_job.cc:146: if (!done_reading_ &&
current_hash_byte_count_ <= 0)
On 2017/03/24 02:01:44, Devlin wrote:
> unrelated to this cl, but when can current_hash_byte_count_ be < 0?

Yes I noticed this and didn't want to change it at first.
Shouldn't ever be < 0, changed to == 0.

https://codereview.chromium.org/2771953003/diff/20001/extensions/browser/cont...
extensions/browser/content_verify_job.cc:149: // This happens when we fail to
read the resource, compute empty content's
On 2017/03/24 02:01:45, Devlin wrote:
> nitty nit: s/,/;||.

Done.

https://codereview.chromium.org/2771953003/diff/20001/extensions/browser/exte...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2771953003/diff/20001/extensions/browser/exte...
extensions/browser/extension_protocols.cc:241: if (result < 0) {
On 2017/03/24 02:01:45, Devlin wrote:
> This block would benefit from some comments. :)

Done.

[Devlin, 2017-03-24 18:50:10]

lgtm; just a few more tiny nits.

https://codereview.chromium.org/2771953003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/2771953003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:61: return
(static_cast<size_t>(result) == content.size());
nit: wrapping parens unnecessary

https://codereview.chromium.org/2771953003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:148: if
(seen_done_reading_extension_ids_.count(id))
This is actually unnecessary, since RunLoop::Run() will quit immediately if
Quit() has already been called.  But it doesn't hurt, so up to you.

https://codereview.chromium.org/2771953003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:154: void Reset() {
And since Quit() can have preemptive effects, we need to reset the run loop
here, too (sadly, there's no RunLoop::Reset, so I think we just need to create a
new one here.)

Alternatively, you could design this to be a single-use class, which is also
okay.

https://codereview.chromium.org/2771953003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:170: class
ScopedVerifyJobDelegateOverride {
Optional: is there a reason to not just have JobDelegate handle scoping itself?

https://codereview.chromium.org/2771953003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:548:
EXPECT_TRUE(extension.get());
nit: may as well ASSERT here; we'll crash below anyways.

https://codereview.chromium.org/2771953003/diff/60001/extensions/browser/exte...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2771953003/diff/60001/extensions/browser/exte...
extensions/browser/extension_protocols.cc:242: // File open failure. |result| is
net:: error code.
Let's expand on this a bit - something like:
// This can happen when the file is unreadable (which can happen during
// corruption or third-party interaction), or when the file itself is
// empty. In both of these cases, we need to be sure to inform the
// verification job that we've finished reading so that it can proceed;
// see crbug.com/nnn.

[commit-bot: I haz the power, 2017-03-24 19:04:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 19:04:44]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[lazyboy, 2017-03-24 19:22:56]

https://codereview.chromium.org/2771953003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/2771953003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:61: return
(static_cast<size_t>(result) == content.size());
On 2017/03/24 18:50:09, Devlin wrote:
> nit: wrapping parens unnecessary

Done.

https://codereview.chromium.org/2771953003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:148: if
(seen_done_reading_extension_ids_.count(id))
On 2017/03/24 18:50:09, Devlin wrote:
> This is actually unnecessary, since RunLoop::Run() will quit immediately if
> Quit() has already been called.  But it doesn't hurt, so up to you.

Assigning waiting_for_extension_id_ in that case seems a bit confusing to me.
Going to keep this one.

https://codereview.chromium.org/2771953003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:154: void Reset() {
On 2017/03/24 18:50:09, Devlin wrote:
> And since Quit() can have preemptive effects, we need to reset the run loop
> here, too (sadly, there's no RunLoop::Reset, so I think we just need to create
a
> new one here.)
> 
> Alternatively, you could design this to be a single-use class, which is also
> okay.

Added new RunLoop here. Done.

https://codereview.chromium.org/2771953003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:170: class
ScopedVerifyJobDelegateOverride {
On 2017/03/24 18:50:09, Devlin wrote:
> Optional: is there a reason to not just have JobDelegate handle scoping
itself?

Ah thanks, fixed.

https://codereview.chromium.org/2771953003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/extension_protocols_unittest.cc:548:
EXPECT_TRUE(extension.get());
On 2017/03/24 18:50:09, Devlin wrote:
> nit: may as well ASSERT here; we'll crash below anyways.

Done.

https://codereview.chromium.org/2771953003/diff/60001/extensions/browser/exte...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2771953003/diff/60001/extensions/browser/exte...
extensions/browser/extension_protocols.cc:242: // File open failure. |result| is
net:: error code.
On 2017/03/24 18:50:09, Devlin wrote:
> Let's expand on this a bit - something like:
> // This can happen when the file is unreadable (which can happen during
> // corruption or third-party interaction), or when the file itself is
> // empty. In both of these cases, we need to be sure to inform the
> // verification job that we've finished reading so that it can proceed;
> // see crbug.com/nnn.

Done.
The empty case is crbug.com/703892, which is a bug in URLRequestFileJob.

[lazyboy, 2017-03-24 19:23:07]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 19:23:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 20:01:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 20:01:16]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[lazyboy, 2017-03-25 00:38:09]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-25 00:38:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-25 01:22:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-25 01:22:08]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-03-27 23:02:22]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 23:03:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 23:47:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 23:47:18]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-03-28 01:25:23]

Description was changed from

==========
Fix content verification code for undreadable and deleted files.

It seems that corruption examples in crbug.com/699420 can cause extension
resource files to go missing or become unreadable.
Fix a bug where we never sent any notification to ContentVerifyJob,
in particular: we never called ContentVerifyJob::DoneReading().

Override URLRequestFileJob::OnOpenComplete in URLRequestExtensionJob
to catch these cases.

BUG=703888
Test=Install an extension (not app) in chromeos. Chromeos because you want
content verification enforcement to kick in. Other way could be to use
--extension-content-verification=enforce_strict flag in other platform.
Either:
1) Make a background script's contents empty, i.e. make it 0 bytes. Content
verifcation would kick in when you try to load/run the extension.
2) Same as above^^^, instead of making contents empty, remove the file
permission. In linux you can just do "chmod -r background.js". Expect
content verification failure.
In both cases extension should be reinstalled in the end.
==========

to

==========
Fix content verification code for undreadable and deleted files.

It seems that corruption examples in crbug.com/699420 can cause extension
resource files to go missing or become unreadable.
Fix a bug where we never sent any notification to ContentVerifyJob,
in particular: we never called ContentVerifyJob::DoneReading().

Override URLRequestFileJob::OnOpenComplete in URLRequestExtensionJob
to catch these cases.

BUG=703888
Test=Install an extension (not app) in chromeos. Chromeos because you want
content verification enforcement to kick in. Other way could be to use
--extension-content-verification=enforce_strict flag in other platform.
Either:
1) Delete background script file. Content verifcation would kick in when
you try to load/run the extension.
2) Same as above^^^, instead of making contents empty, remove the file
permission. In linux you can just do "chmod -r background.js". Expect
content verification failure.
In both cases extension should be reinstalled in the end.
==========

[lazyboy, 2017-03-28 01:26:19]

Description was changed from

==========
Fix content verification code for undreadable and deleted files.

It seems that corruption examples in crbug.com/699420 can cause extension
resource files to go missing or become unreadable.
Fix a bug where we never sent any notification to ContentVerifyJob,
in particular: we never called ContentVerifyJob::DoneReading().

Override URLRequestFileJob::OnOpenComplete in URLRequestExtensionJob
to catch these cases.

BUG=703888
Test=Install an extension (not app) in chromeos. Chromeos because you want
content verification enforcement to kick in. Other way could be to use
--extension-content-verification=enforce_strict flag in other platform.
Either:
1) Delete background script file. Content verifcation would kick in when
you try to load/run the extension.
2) Same as above^^^, instead of making contents empty, remove the file
permission. In linux you can just do "chmod -r background.js". Expect
content verification failure.
In both cases extension should be reinstalled in the end.
==========

to

==========
Fix content verification code for undreadable and deleted files.

It seems that corruption examples in crbug.com/699420 can cause extension
resource files to go missing or become unreadable.
Fix a bug where we never sent any notification to ContentVerifyJob,
in particular: we never called ContentVerifyJob::DoneReading().

Override URLRequestFileJob::OnOpenComplete in URLRequestExtensionJob
to catch these cases.

BUG=703888
Test=Install an extension (not app) in chromeos. Chromeos because you want
content verification enforcement to kick in. Other way could be to use
--extension-content-verification=enforce_strict flag in other platform.
Either:
1) Delete background script file. Content verifcation would kick in when
you try to load/run the extension.
2) Same as above^^^, instead of deleting the file, remove the file
permission. In linux you can just do "chmod -r background.js". Expect
content verification failure.
In both cases extension should be reinstalled in the end.
==========

[lazyboy, 2017-03-28 01:50:19]

https://codereview.chromium.org/2771953003/diff/120001/extensions/browser/con...
File extensions/browser/content_verify_job.cc (right):

https://codereview.chromium.org/2771953003/diff/120001/extensions/browser/con...
extensions/browser/content_verify_job.cc:136: if (hashes_ready_) {
Ugh, unfortunately, I'm struggling with edge cases with the CL I keep
discovering :(

1)
if hashes_ready_ = false (which can happen for early resources of an extension),
that means we will compute hashes and then land in
ContentVerifyJob::OnHashesReady(). For a deleted file F1,
hash_reader_->content_exists() will be false and we will skip verifying that
file on line 172. That was added to fix a legit bug: http://crbug.com/404802
where extension should be able to xhr non-existent resources and not trigger a
verification failure.

2)
However, if hashes_ready_ = true then we have found or calculated
computed_hashes.json. But depending on which point we have populated that
computed_hashes.json, it may or may not have F1's entry in it.
In both cases FinishBlock() will return false and we will send error.

So clearly, depending on whether we get OnHashesReady() first or DoneReading()
first, we will be inconsistent.

The discovery of this issue through 404802 certainly made things trickier, we
have to find a better way to identify an extension's static resources.

[lazyboy, 2017-03-29 02:14:33]

Description was changed from

==========
Fix content verification code for undreadable and deleted files.

It seems that corruption examples in crbug.com/699420 can cause extension
resource files to go missing or become unreadable.
Fix a bug where we never sent any notification to ContentVerifyJob,
in particular: we never called ContentVerifyJob::DoneReading().

Override URLRequestFileJob::OnOpenComplete in URLRequestExtensionJob
to catch these cases.

BUG=703888
Test=Install an extension (not app) in chromeos. Chromeos because you want
content verification enforcement to kick in. Other way could be to use
--extension-content-verification=enforce_strict flag in other platform.
Either:
1) Delete background script file. Content verifcation would kick in when
you try to load/run the extension.
2) Same as above^^^, instead of deleting the file, remove the file
permission. In linux you can just do "chmod -r background.js". Expect
content verification failure.
In both cases extension should be reinstalled in the end.
==========

to

==========
Fix content verification code for undreadable and deleted files.

It seems that corruption examples in crbug.com/699420 can cause extension
resource files to go missing or become unreadable.
Fix a bug where we never sent any notification to ContentVerifyJob,
in particular: we never called ContentVerifyJob::DoneReading().

Override URLRequestFileJob::OnOpenComplete in URLRequestExtensionJob
to catch these cases.

Also make ContentHashReader not skip legitimate deleted files: the
files that are listed in verified_contents.json. It will still skip
non-existent resources, e.g. xhrs requesting non-existent file (see
crbug.com/404802 for example).

BUG=703888
Test=Install an extension (not app) in chromeos. Chromeos because you want
content verification enforcement to kick in. Other way could be to use
--extension-content-verification=enforce_strict flag in other platform.
Either:
1) Delete background script file. Content verifcation would kick in when
you try to load/run the extension.
2) Same as above^^^, instead of deleting the file, remove the file
permission. In linux you can just do "chmod -r background.js". Expect
content verification failure.
In both cases extension should be reinstalled in the end.
==========

[lazyboy, 2017-03-29 02:16:31]

https://codereview.chromium.org/2771953003/diff/120001/extensions/browser/con...
File extensions/browser/content_verify_job.cc (right):

https://codereview.chromium.org/2771953003/diff/120001/extensions/browser/con...
extensions/browser/content_verify_job.cc:136: if (hashes_ready_) {
On 2017/03/28 01:50:19, lazyboy wrote:
> Ugh, unfortunately, I'm struggling with edge cases with the CL I keep
> discovering :(
> 
> 1)
> if hashes_ready_ = false (which can happen for early resources of an
extension),
> that means we will compute hashes and then land in
> ContentVerifyJob::OnHashesReady(). For a deleted file F1,
> hash_reader_->content_exists() will be false and we will skip verifying that
> file on line 172. That was added to fix a legit bug: http://crbug.com/404802
> where extension should be able to xhr non-existent resources and not trigger a
> verification failure.
> 
> 2)
> However, if hashes_ready_ = true then we have found or calculated
> computed_hashes.json. But depending on which point we have populated that
> computed_hashes.json, it may or may not have F1's entry in it.
> In both cases FinishBlock() will return false and we will send error.
> 
> So clearly, depending on whether we get OnHashesReady() first or DoneReading()
> first, we will be inconsistent.
> 
> The discovery of this issue through 404802 certainly made things trickier, we
> have to find a better way to identify an extension's static resources.

This has been resolved in patch set 9. See the diff from PS 8 for this.
PTAL.

[Devlin, 2017-03-29 15:51:57]

Still mostly good, but a few more comments

https://codereview.chromium.org/2771953003/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/content_verifier_browsertest.cc (right):

https://codereview.chromium.org/2771953003/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/content_verifier_browsertest.cc:253: :
Result::SUCCESS;
Result::FAILURE?

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
File extensions/browser/content_verify_job_unittest.cc (right):

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:8: #include
"base/memory/weak_ptr.h"
needed?

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:26:
extensions::ContentHashReader* CreateContentHashReader(
nit: return a refptr

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:37:
extensions::ContentVerifyJob::FailureReason reason) {}
nit: no extensions:: prefix (also the rest of this file)

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:65: return failure_reason_;
maybe make failure_reason_ a base::Optional<>, and then DCHECK/EXPECT_TRUE it
here?

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:88: browser_threads_.reset(new
content::TestBrowserThreadBundle(
prefer base::MakeUnique

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:131:
GetTestPath(base::FilePath(FILE_PATH_LITERAL("with_verified_contents")));
I don't see this - did you forget to add it to the patch?

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:186: // Expect
content-verification failure.
nitty nit: comment probably unnecessary with the comment on line 168 + the
expect_eq

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:213: }
Since we're adding unit tests for this (yay! :)), can we add a quick one for a
hash mismatch on a non-empty file?  e.g. just rewriting background.js with
"foo"?

[lazyboy, 2017-03-29 17:11:58]

Patchset #10 (id:180001) has been deleted

[lazyboy, 2017-03-29 17:12:51]

Thanks for the review, comments addressed in PS #10

https://codereview.chromium.org/2771953003/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/content_verifier_browsertest.cc (right):

https://codereview.chromium.org/2771953003/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/content_verifier_browsertest.cc:253: :
Result::SUCCESS;
On 2017/03/29 15:51:56, Devlin wrote:
> Result::FAILURE?

Done, thanks.

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
File extensions/browser/content_verify_job_unittest.cc (right):

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:8: #include
"base/memory/weak_ptr.h"
On 2017/03/29 15:51:57, Devlin wrote:
> needed?

No, removed.

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:26:
extensions::ContentHashReader* CreateContentHashReader(
On 2017/03/29 15:51:57, Devlin wrote:
> nit: return a refptr

Done.

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:37:
extensions::ContentVerifyJob::FailureReason reason) {}
On 2017/03/29 15:51:57, Devlin wrote:
> nit: no extensions:: prefix (also the rest of this file)

Done.

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:65: return failure_reason_;
On 2017/03/29 15:51:56, Devlin wrote:
> maybe make failure_reason_ a base::Optional<>, and then DCHECK/EXPECT_TRUE it
> here?

Done.

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:88: browser_threads_.reset(new
content::TestBrowserThreadBundle(
On 2017/03/29 15:51:57, Devlin wrote:
> prefer base::MakeUnique

Done.

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:131:
GetTestPath(base::FilePath(FILE_PATH_LITERAL("with_verified_contents")));
On 2017/03/29 15:51:57, Devlin wrote:
> I don't see this - did you forget to add it to the patch?

Yes, added now.

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:186: // Expect
content-verification failure.
On 2017/03/29 15:51:56, Devlin wrote:
> nitty nit: comment probably unnecessary with the comment on line 168 + the
> expect_eq

Done.

https://codereview.chromium.org/2771953003/diff/160001/extensions/browser/con...
extensions/browser/content_verify_job_unittest.cc:213: }
On 2017/03/29 15:51:57, Devlin wrote:
> Since we're adding unit tests for this (yay! :)), can we add a quick one for a
> hash mismatch on a non-empty file?  e.g. just rewriting background.js with
> "foo"?

Good idea, added.

[Devlin, 2017-03-29 18:23:44]

lgtm

[lazyboy, 2017-03-29 19:58:50]

The CQ bit was checked by lazyboy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 19:59:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lazyboy, 2017-03-29 23:53:15]

The CQ bit was unchecked by lazyboy@chromium.org

[lazyboy, 2017-03-29 23:53:24]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2017-03-29 23:54:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 00:57:35]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1490831604184630,
"parent_rev": "36eeff88e474d2e562f2d656bb94d0e37770e245", "commit_rev":
"d6dbb26e31818eed0097654ecb89aaddc2c64f8d"}

[commit-bot: I haz the power, 2017-03-30 00:58:19]

Description was changed from

==========
Fix content verification code for undreadable and deleted files.

It seems that corruption examples in crbug.com/699420 can cause extension
resource files to go missing or become unreadable.
Fix a bug where we never sent any notification to ContentVerifyJob,
in particular: we never called ContentVerifyJob::DoneReading().

Override URLRequestFileJob::OnOpenComplete in URLRequestExtensionJob
to catch these cases.

Also make ContentHashReader not skip legitimate deleted files: the
files that are listed in verified_contents.json. It will still skip
non-existent resources, e.g. xhrs requesting non-existent file (see
crbug.com/404802 for example).

BUG=703888
Test=Install an extension (not app) in chromeos. Chromeos because you want
content verification enforcement to kick in. Other way could be to use
--extension-content-verification=enforce_strict flag in other platform.
Either:
1) Delete background script file. Content verifcation would kick in when
you try to load/run the extension.
2) Same as above^^^, instead of deleting the file, remove the file
permission. In linux you can just do "chmod -r background.js". Expect
content verification failure.
In both cases extension should be reinstalled in the end.
==========

to

==========
Fix content verification code for undreadable and deleted files.

It seems that corruption examples in crbug.com/699420 can cause extension
resource files to go missing or become unreadable.
Fix a bug where we never sent any notification to ContentVerifyJob,
in particular: we never called ContentVerifyJob::DoneReading().

Override URLRequestFileJob::OnOpenComplete in URLRequestExtensionJob
to catch these cases.

Also make ContentHashReader not skip legitimate deleted files: the
files that are listed in verified_contents.json. It will still skip
non-existent resources, e.g. xhrs requesting non-existent file (see
crbug.com/404802 for example).

BUG=703888
Test=Install an extension (not app) in chromeos. Chromeos because you want
content verification enforcement to kick in. Other way could be to use
--extension-content-verification=enforce_strict flag in other platform.
Either:
1) Delete background script file. Content verifcation would kick in when
you try to load/run the extension.
2) Same as above^^^, instead of deleting the file, remove the file
permission. In linux you can just do "chmod -r background.js". Expect
content verification failure.
In both cases extension should be reinstalled in the end.

Review-Url: https://codereview.chromium.org/2771953003
Cr-Commit-Position: refs/heads/master@{#460607}
Committed:
https://chromium.googlesource.com/chromium/src/+/d6dbb26e31818eed0097654ecb89...
==========

[commit-bot: I haz the power, 2017-03-30 00:58:20]

Committed patchset #10 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/d6dbb26e31818eed0097654ecb89...