[Content verification] Fix a bug for files that are multiple of 4k bytes.

[Content verification] Fix a bug for files that are multiple of 4k bytes.

There was an off-by-one error in ContentVerifyJob::FinishBlock, that
would try to look beyond the last 4k block if the total file size was
multiple of 4k block. The out of bound block would not be returned
by hash_reader_->GetHashForBlock, so content verification would always
fail. e.g.

For an 8k file:
FinishBlock() -> for bytes 0-4095
FinishBlock() -> for bytes 4096-8191
Then an additional FinishBlock() from either ContentVerifyJob::DoneReading or
ContentVerifyJob::OnHashesReady would get called.

Ideally we shouldn't be calling the third FinishBlock() in this case, but
because how the code is laid out currently:
1) hash fetching and reading file content to compute hashes in any order, and
2) handling zero byte and deleted files,
It is less intrusive to add a bail out in FinishBlock to handle this case.

The bug was exposed by https://codereview.chromium.org/2771953003.

This CL also adds regression tests.

BUG=720597
Test=See bug description for reproduction.

Review-Url: https://codereview.chromium.org/2928693002
Cr-Commit-Position: refs/heads/master@{#477751}
Committed: https://chromium.googlesource.com/chromium/src/+/2557ca6ae520912f73ff133c4371a45a2cb43161

[lazyboy, 2017-06-07 03:10:21]

Description was changed from

==========
[Content verfication] Fix a bug for files that are multiple of 4k bytes.

There was an off-by-one error in ContentVerifyJob::FinishBlock, that
would try to look beyond the last 4k block if the total file size was
multiple of 4k block, e.g.

For an 8k file:
FinishBlock() -> for bytes 0-4095
FinishBlock() -> for bytes 4096-8191
Then an additional FinishBlock() from either ContentVerifyJob::DoneReading or
ContentVerifyJob::OnHashesReady would get called.

Ideally we shouldn't be calling the third FinishBlock() in this case, but
because how the code is laid out currently with:
1) hash fetching and reading file content to compute hashes in any order, and
2) handling zero byte and deleted files,
It is less intrusive to add a bail out in FinishBlock to handle this case.

The bug was exposed by https://codereview.chromium.org/2771953003.

This CL also adds regression tests.

BUG=720597
Test=See bug description for reproduction.
==========

to

==========
[Content verfication] Fix a bug for files that are multiple of 4k bytes.

There was an off-by-one error in ContentVerifyJob::FinishBlock, that
would try to look beyond the last 4k block if the total file size was
multiple of 4k block. The out of bound block would not be returned
by hash_reader_->GetHashForBlock, so content verification would always
fail. e.g.

For an 8k file:
FinishBlock() -> for bytes 0-4095
FinishBlock() -> for bytes 4096-8191
Then an additional FinishBlock() from either ContentVerifyJob::DoneReading or
ContentVerifyJob::OnHashesReady would get called.

Ideally we shouldn't be calling the third FinishBlock() in this case, but
because how the code is laid out currently:
1) hash fetching and reading file content to compute hashes in any order, and
2) handling zero byte and deleted files,
It is less intrusive to add a bail out in FinishBlock to handle this case.

The bug was exposed by https://codereview.chromium.org/2771953003.

This CL also adds regression tests.

BUG=720597
Test=See bug description for reproduction.
==========

[lazyboy, 2017-06-07 03:10:52]

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

[commit-bot: I haz the power, 2017-06-07 03:11:05]

Dry run: CQ is trying da patch.

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

[lazyboy, 2017-06-07 03:12:49]

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

[lazyboy, 2017-06-07 03:16:48]

Description was changed from

==========
[Content verfication] Fix a bug for files that are multiple of 4k bytes.

There was an off-by-one error in ContentVerifyJob::FinishBlock, that
would try to look beyond the last 4k block if the total file size was
multiple of 4k block. The out of bound block would not be returned
by hash_reader_->GetHashForBlock, so content verification would always
fail. e.g.

For an 8k file:
FinishBlock() -> for bytes 0-4095
FinishBlock() -> for bytes 4096-8191
Then an additional FinishBlock() from either ContentVerifyJob::DoneReading or
ContentVerifyJob::OnHashesReady would get called.

Ideally we shouldn't be calling the third FinishBlock() in this case, but
because how the code is laid out currently:
1) hash fetching and reading file content to compute hashes in any order, and
2) handling zero byte and deleted files,
It is less intrusive to add a bail out in FinishBlock to handle this case.

The bug was exposed by https://codereview.chromium.org/2771953003.

This CL also adds regression tests.

BUG=720597
Test=See bug description for reproduction.
==========

to

==========
[Content verification] Fix a bug for files that are multiple of 4k bytes.

There was an off-by-one error in ContentVerifyJob::FinishBlock, that
would try to look beyond the last 4k block if the total file size was
multiple of 4k block. The out of bound block would not be returned
by hash_reader_->GetHashForBlock, so content verification would always
fail. e.g.

For an 8k file:
FinishBlock() -> for bytes 0-4095
FinishBlock() -> for bytes 4096-8191
Then an additional FinishBlock() from either ContentVerifyJob::DoneReading or
ContentVerifyJob::OnHashesReady would get called.

Ideally we shouldn't be calling the third FinishBlock() in this case, but
because how the code is laid out currently:
1) hash fetching and reading file content to compute hashes in any order, and
2) handling zero byte and deleted files,
It is less intrusive to add a bail out in FinishBlock to handle this case.

The bug was exposed by https://codereview.chromium.org/2771953003.

This CL also adds regression tests.

BUG=720597
Test=See bug description for reproduction.
==========

[commit-bot: I haz the power, 2017-06-07 03:52:39]

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

[commit-bot: I haz the power, 2017-06-07 03:52:40]

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_...)

[Devlin, 2017-06-07 13:27:32]

lgtm

https://codereview.chromium.org/2928693002/diff/1/extensions/browser/content_...
File extensions/browser/content_verify_job_unittest.cc (right):

https://codereview.chromium.org/2928693002/diff/1/extensions/browser/content_...
extensions/browser/content_verify_job_unittest.cc:280: for (size_t i = 0; i <
arraysize(files_to_test); ++i) {
nit: why not const auto& file_to_test : files_to_test?

https://codereview.chromium.org/2928693002/diff/1/extensions/browser/content_...
extensions/browser/content_verify_job_unittest.cc:284:
base::ReadFileToString(unzipped_path.Append(files_to_test[i]), &contents);
as a sanity check, can we expect_eq the size of the string?

[lazyboy, 2017-06-07 18:54:15]

https://codereview.chromium.org/2928693002/diff/1/extensions/browser/content_...
File extensions/browser/content_verify_job_unittest.cc (right):

https://codereview.chromium.org/2928693002/diff/1/extensions/browser/content_...
extensions/browser/content_verify_job_unittest.cc:280: for (size_t i = 0; i <
arraysize(files_to_test); ++i) {
On 2017/06/07 13:27:31, Devlin (slow through June 8) wrote:
> nit: why not const auto& file_to_test : files_to_test?

Done.

https://codereview.chromium.org/2928693002/diff/1/extensions/browser/content_...
extensions/browser/content_verify_job_unittest.cc:284:
base::ReadFileToString(unzipped_path.Append(files_to_test[i]), &contents);
On 2017/06/07 13:27:31, Devlin (slow through June 8) wrote:
> as a sanity check, can we expect_eq the size of the string?

Done.

[lazyboy, 2017-06-07 18:54:26]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2017-06-07 18:54:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2928693002/#ps20001
(title: "address comments")

[commit-bot: I haz the power, 2017-06-07 18:55: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-06-07 19:12:53]

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

[commit-bot: I haz the power, 2017-06-07 19:12:55]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[lazyboy, 2017-06-07 19:16:35]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2017-06-07 19:17:27]

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-06-07 20:29:44]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496862995452980,
"parent_rev": "9576b7bd92fc9c368cd0200a0105b8244396bd3f", "commit_rev":
"2557ca6ae520912f73ff133c4371a45a2cb43161"}

[commit-bot: I haz the power, 2017-06-07 20:29:55]

Description was changed from

==========
[Content verification] Fix a bug for files that are multiple of 4k bytes.

There was an off-by-one error in ContentVerifyJob::FinishBlock, that
would try to look beyond the last 4k block if the total file size was
multiple of 4k block. The out of bound block would not be returned
by hash_reader_->GetHashForBlock, so content verification would always
fail. e.g.

For an 8k file:
FinishBlock() -> for bytes 0-4095
FinishBlock() -> for bytes 4096-8191
Then an additional FinishBlock() from either ContentVerifyJob::DoneReading or
ContentVerifyJob::OnHashesReady would get called.

Ideally we shouldn't be calling the third FinishBlock() in this case, but
because how the code is laid out currently:
1) hash fetching and reading file content to compute hashes in any order, and
2) handling zero byte and deleted files,
It is less intrusive to add a bail out in FinishBlock to handle this case.

The bug was exposed by https://codereview.chromium.org/2771953003.

This CL also adds regression tests.

BUG=720597
Test=See bug description for reproduction.
==========

to

==========
[Content verification] Fix a bug for files that are multiple of 4k bytes.

There was an off-by-one error in ContentVerifyJob::FinishBlock, that
would try to look beyond the last 4k block if the total file size was
multiple of 4k block. The out of bound block would not be returned
by hash_reader_->GetHashForBlock, so content verification would always
fail. e.g.

For an 8k file:
FinishBlock() -> for bytes 0-4095
FinishBlock() -> for bytes 4096-8191
Then an additional FinishBlock() from either ContentVerifyJob::DoneReading or
ContentVerifyJob::OnHashesReady would get called.

Ideally we shouldn't be calling the third FinishBlock() in this case, but
because how the code is laid out currently:
1) hash fetching and reading file content to compute hashes in any order, and
2) handling zero byte and deleted files,
It is less intrusive to add a bail out in FinishBlock to handle this case.

The bug was exposed by https://codereview.chromium.org/2771953003.

This CL also adds regression tests.

BUG=720597
Test=See bug description for reproduction.

Review-Url: https://codereview.chromium.org/2928693002
Cr-Commit-Position: refs/heads/master@{#477751}
Committed:
https://chromium.googlesource.com/chromium/src/+/2557ca6ae520912f73ff133c4371...
==========

[commit-bot: I haz the power, 2017-06-07 20:29:56]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/2557ca6ae520912f73ff133c4371...