Fix extension content verification out-of-band hash fetching

Fix extension content verification out-of-band hash fetching

When we changed content verification to strict mode (crbug.com/601512),
it turns out that the code for downloading missing
verified_contents.json files from the webstore had two longstanding bugs
that we never noticed due to insufficient tests:

a) Returning the results of a boolean expression as an integer
b) Use after free of an object

The end result is that when the patch for crbug.com/601512 hit stable,
many users became unable to run the EFF's HTTPS Everywhere and Privacy
Badger extensions because those are uploaded via a special process where
the webstore does not have a copy of their private key and cannot
include the verified_contents.json file in the .crx file; rather we rely
on being able to download it after install but fail due to the above
mentioned bugs.

This patch fixes the bugs and also introduces a change which clears the
corruption "disable reason" on extension autoupdate, so that users who
had the extension disabled because of this bug can get it re-enabled
automatically by an extension autoupdate.

BUG=643814
Committed: https://crrev.com/e4de9f973eae513486d2912e1793e943fce70014
Cr-Commit-Position: refs/heads/master@{#418748}

[asargent_no_longer_on_chrome, 2016-09-14 04:05:35]

asargent@chromium.org changed reviewers:
+ lazyboy@chromium.org

[asargent_no_longer_on_chrome, 2016-09-14 04:11:18]

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

[commit-bot: I haz the power, 2016-09-14 04:11:29]

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, 2016-09-14 05:21:51]

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

[commit-bot: I haz the power, 2016-09-14 05:21:52]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2016-09-14 22:23:20]

lgtm % nits/suggestions

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

https://codereview.chromium.org/2336403002/diff/1/extensions/browser/content_...
extensions/browser/content_hash_fetcher.cc:261: if
(!base::CreateDirectoryAndGetError(dir, NULL))
nit: change to nullptr

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

https://codereview.chromium.org/2336403002/diff/1/extensions/browser/content_...
extensions/browser/content_hash_fetcher_unittest.cc:79:
base::WeakPtrFactory<ContentHashFetcherWaiter> weak_factory_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2336403002/diff/1/extensions/browser/content_...
extensions/browser/content_hash_fetcher_unittest.cc:188:
UnzipToTempDirAndLoad(test_dir_base.AppendASCII("source.zip"));
Make sure there wasn't any verified_contents.json in the beginning?
EXPECT_FALSE(base::PathExists(file_util::GetVerifiedContentsPath(extension->path())));

https://codereview.chromium.org/2336403002/diff/1/extensions/browser/content_...
extensions/browser/content_hash_fetcher_unittest.cc:204: ASSERT_NE(nullptr,
result.get());
ASSERT_TRUE(result.get())?

[asargent_no_longer_on_chrome, 2016-09-14 23:26:24]

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

https://codereview.chromium.org/2336403002/diff/1/extensions/browser/content_...
extensions/browser/content_hash_fetcher.cc:261: if
(!base::CreateDirectoryAndGetError(dir, NULL))
On 2016/09/14 22:23:20, lazyboy wrote:
> nit: change to nullptr

Done.

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

https://codereview.chromium.org/2336403002/diff/1/extensions/browser/content_...
extensions/browser/content_hash_fetcher_unittest.cc:79:
base::WeakPtrFactory<ContentHashFetcherWaiter> weak_factory_;
On 2016/09/14 22:23:20, lazyboy wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2336403002/diff/1/extensions/browser/content_...
extensions/browser/content_hash_fetcher_unittest.cc:188:
UnzipToTempDirAndLoad(test_dir_base.AppendASCII("source.zip"));
On 2016/09/14 22:23:20, lazyboy wrote:
> Make sure there wasn't any verified_contents.json in the beginning?
>
EXPECT_FALSE(base::PathExists(file_util::GetVerifiedContentsPath(extension->path())));

Great idea, done.

https://codereview.chromium.org/2336403002/diff/1/extensions/browser/content_...
extensions/browser/content_hash_fetcher_unittest.cc:204: ASSERT_NE(nullptr,
result.get());
On 2016/09/14 22:23:20, lazyboy wrote:
> ASSERT_TRUE(result.get())?

Done.

[asargent_no_longer_on_chrome, 2016-09-14 23:28:18]

The CQ bit was checked by asargent@chromium.org

[asargent_no_longer_on_chrome, 2016-09-14 23:28:18]

The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/2336403002/#ps20001
(title: "review feedback")

[commit-bot: I haz the power, 2016-09-14 23:29:02]

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, 2016-09-15 01:40:51]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-15 01:43:13]

Description was changed from

==========
Fix extension content verification out-of-band hash fetching

When we changed content verification to strict mode (crbug.com/601512),
it turns out that the code for downloading missing
verified_contents.json files from the webstore had two longstanding bugs
that we never noticed due to insufficient tests:

a) Returning the results of a boolean expression as an integer
b) Use after free of an object

The end result is that when the patch for crbug.com/601512 hit stable,
many users became unable to run the EFF's HTTPS Everywhere and Privacy
Badger extensions because those are uploaded via a special process where
the webstore does not have a copy of their private key and cannot
include the verified_contents.json file in the .crx file; rather we rely
on being able to download it after install but fail due to the above
mentioned bugs.

This patch fixes the bugs and also introduces a change which clears the
corruption "disable reason" on extension autoupdate, so that users who
had the extension disabled because of this bug can get it re-enabled
automatically by an extension autoupdate.

BUG=643814
==========

to

==========
Fix extension content verification out-of-band hash fetching

When we changed content verification to strict mode (crbug.com/601512),
it turns out that the code for downloading missing
verified_contents.json files from the webstore had two longstanding bugs
that we never noticed due to insufficient tests:

a) Returning the results of a boolean expression as an integer
b) Use after free of an object

The end result is that when the patch for crbug.com/601512 hit stable,
many users became unable to run the EFF's HTTPS Everywhere and Privacy
Badger extensions because those are uploaded via a special process where
the webstore does not have a copy of their private key and cannot
include the verified_contents.json file in the .crx file; rather we rely
on being able to download it after install but fail due to the above
mentioned bugs.

This patch fixes the bugs and also introduces a change which clears the
corruption "disable reason" on extension autoupdate, so that users who
had the extension disabled because of this bug can get it re-enabled
automatically by an extension autoupdate.

BUG=643814

Committed: https://crrev.com/e4de9f973eae513486d2912e1793e943fce70014
Cr-Commit-Position: refs/heads/master@{#418748}
==========

[commit-bot: I haz the power, 2016-09-15 01:43:14]

Patchset 2 (id:??) landed as
https://crrev.com/e4de9f973eae513486d2912e1793e943fce70014
Cr-Commit-Position: refs/heads/master@{#418748}