Take Murmur2 hash of untransformed icon when creating WebAPK part 1

Take Murmur2 hash of untransformed icon when creating WebAPK

This CL makes WebApkInstaller take the Murmur2 hash of the bitmap at the icon
URL prior to any transformations being applied to the bitmap (such as
encoding/decoding the bitmap). The icon hash is used to determine whether the
icon that the user sees matches the icon of a WebAPK that the server generated
for another user.

The icon could be dynamically generated and:
- use the same icon URL for all users
- be visually different for each user

If the hashes match the server can vend the WebAPK that it
previously generated to other users. Vending previously generated WebAPKs is
faster than creating a new WebAPK for each "create WebAPK" request.

BUG=624059
TEST=WebApkIconHasherTest

Committed: https://crrev.com/ccf35305af4fba3e1cd73bb9ce032fe9063d0eea
Cr-Commit-Position: refs/heads/master@{#412586}

[pkotwicz, 2016-08-11 03:45:46]

Description was changed from

==========
Take Murmur2 hash of untransformed icon when creating WebAPK

This CL makes WebApkInstaller take the Murmur2 hash of the bitmap at the icon
URL prior to any transformations being applied to the bitmap (such as
encoding/decoding the bitmap). The WebAPK server crawls the internet and
generates WebAPKs for any Web Manifests that it finds. The icon hash is used to
determine whether the icon that the user sees matches the icon of a WebAPK that
the server generated while crawling the internet.

BUG=624059
==========

to

==========
Take Murmur2 hash of untransformed icon when creating WebAPK

This CL makes WebApkInstaller take the Murmur2 hash of the bitmap at the icon
URL prior to any transformations being applied to the bitmap (such as
encoding/decoding the bitmap). The WebAPK server crawls the internet and
generates WebAPKs for any Web Manifests that it finds. The icon hash is used to
determine whether the icon that the user sees matches the icon of a WebAPK that
the server generated while crawling the internet, hence the requirement of
taking
the Murmur2 hash prior to any transformations of the bitmap.

BUG=624059
==========

[pkotwicz, 2016-08-11 03:45:46]

pkotwicz@chromium.org changed reviewers:
+ yfriedman@chromium.org

[pkotwicz, 2016-08-11 03:58:12]

pkotwicz@chromium.org changed reviewers:
- yfriedman@chromium.org

[pkotwicz, 2016-08-11 22:57:18]

Patchset #2 (id:20001) has been deleted

[pkotwicz, 2016-08-11 22:57:23]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2016-08-11 23:12:55]

Patchset #1 (id:40001) has been deleted

[pkotwicz, 2016-08-11 23:14:05]

pkotwicz@chromium.org changed reviewers:
+ yfriedman@chromium.org

[pkotwicz, 2016-08-11 23:14:06]

Yaron, can you please take a look?

I still need to make ManifestUpgradeDetector use the Murmur2 hash of the
untransformed bitmap bytes

[Yaron, 2016-08-12 21:24:06]

https://codereview.chromium.org/2231843003/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2231843003/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:154: // the bitmap). The
WebAPK server crawls the internet and generates WebAPKs
it doesn't actually crawl the internet per-say and it's an implementation
detail. I would simplify it and just say the hash is used for caching. Please
also update CL description accordingly

https://codereview.chromium.org/2231843003/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:160: // should be fast because
the icon should be in the HTTP cache.
have you tested that locally? is there a way to set flags so that it only loads
from cache?

[pkotwicz, 2016-08-15 16:10:57]

Description was changed from

==========
Take Murmur2 hash of untransformed icon when creating WebAPK

This CL makes WebApkInstaller take the Murmur2 hash of the bitmap at the icon
URL prior to any transformations being applied to the bitmap (such as
encoding/decoding the bitmap). The WebAPK server crawls the internet and
generates WebAPKs for any Web Manifests that it finds. The icon hash is used to
determine whether the icon that the user sees matches the icon of a WebAPK that
the server generated while crawling the internet, hence the requirement of
taking
the Murmur2 hash prior to any transformations of the bitmap.

BUG=624059
==========

to

==========
Take Murmur2 hash of untransformed icon when creating WebAPK

This CL makes WebApkInstaller take the Murmur2 hash of the bitmap at the icon
URL prior to any transformations being applied to the bitmap (such as
encoding/decoding the bitmap). The icon hash is used to determine whether the
icon that the user sees matches the icon of a WebAPK that the server generated
for another user.

The icon could be dynamically generated and:
- use the same icon URL for all users
- be visually different for each user

If the hashes match the server can vend the WebAPK that it
previously generated to other users. Vending previously generated WebAPKs is
faster than creating a new WebAPK for each "create WebAPK" request.

BUG=624059
==========

[pkotwicz, 2016-08-15 17:19:08]

Patchset #2 (id:80001) has been deleted

[pkotwicz, 2016-08-15 17:19:26]

Yaron can you please take another look?

https://codereview.chromium.org/2231843003/diff/60001/chrome/browser/android/...
File chrome/browser/android/webapk/webapk_installer.cc (right):

https://codereview.chromium.org/2231843003/diff/60001/chrome/browser/android/...
chrome/browser/android/webapk/webapk_installer.cc:160: // should be fast because
the icon should be in the HTTP cache.
Yes, I have tested locally and the icon is loaded from the cache. (I checked
URLFetcher::IsCached())

There is a flag so that URLFetcher loads only from the cache:
net::ONLY_FROM_CACHE. I don't want to use it though. (I don't know whether it is
possible for the icon data to be evicted from the cache)

[Yaron, 2016-08-16 00:35:23]

lgtm

[pkotwicz, 2016-08-16 18:34:50]

Patchset #3 (id:120001) has been deleted

[pkotwicz, 2016-08-16 18:34:57]

Patchset #2 (id:100001) has been deleted

[pkotwicz, 2016-08-16 18:49:27]

pkotwicz@chromium.org changed reviewers:
+ rsesek@chromium.org

[pkotwicz, 2016-08-16 18:49:28]

Adding Robert to look at webapk_icon_hasher.cc from a security stand point.

This CL takes gets the Murmur2 hash from arbitrary images from the web in the
browser process. Based on offline conversation, I believe that you said that
this was acceptable

[Robert Sesek, 2016-08-16 19:55:57]

Yes, this is OK from a security perspective.

I'd also encourage you to write a test for the WebApkIconHasher (either
standalone or part of the WebApkInstallerTest). We have some test data images
(e.g. chrome/test/data/android/google.png, or add your own) that you could use
with net::SpawnedTestServer or a FakeURLFetcher (may need to inject a
URLFetcherFactory). Then you can test that the hash is correct for the expected
image.

https://codereview.chromium.org/2231843003/diff/160001/chrome/browser/android...
File chrome/browser/android/webapk/webapk_icon_hasher.cc (right):

https://codereview.chromium.org/2231843003/diff/160001/chrome/browser/android...
chrome/browser/android/webapk/webapk_icon_hasher.cc:44: // WARNING: We are
running in the browser process. |raw_image_data| is the
Thanks for the security-conscious comment!

https://codereview.chromium.org/2231843003/diff/160001/chrome/browser/android...
File chrome/browser/android/webapk/webapk_installer_unittest.cc (right):

https://codereview.chromium.org/2231843003/diff/160001/chrome/browser/android...
chrome/browser/android/webapk/webapk_installer_unittest.cc:163: icon_url_ =
icon_url;
nit: over-indented

[pkotwicz, 2016-08-17 01:15:00]

Patchset #4 (id:180001) has been deleted

[pkotwicz, 2016-08-17 01:15:09]

Patchset #4 (id:200001) has been deleted

[pkotwicz, 2016-08-17 01:15:58]

Description was changed from

==========
Take Murmur2 hash of untransformed icon when creating WebAPK

This CL makes WebApkInstaller take the Murmur2 hash of the bitmap at the icon
URL prior to any transformations being applied to the bitmap (such as
encoding/decoding the bitmap). The icon hash is used to determine whether the
icon that the user sees matches the icon of a WebAPK that the server generated
for another user.

The icon could be dynamically generated and:
- use the same icon URL for all users
- be visually different for each user

If the hashes match the server can vend the WebAPK that it
previously generated to other users. Vending previously generated WebAPKs is
faster than creating a new WebAPK for each "create WebAPK" request.

BUG=624059
==========

to

==========
Take Murmur2 hash of untransformed icon when creating WebAPK

This CL makes WebApkInstaller take the Murmur2 hash of the bitmap at the icon
URL prior to any transformations being applied to the bitmap (such as
encoding/decoding the bitmap). The icon hash is used to determine whether the
icon that the user sees matches the icon of a WebAPK that the server generated
for another user.

The icon could be dynamically generated and:
- use the same icon URL for all users
- be visually different for each user

If the hashes match the server can vend the WebAPK that it
previously generated to other users. Vending previously generated WebAPKs is
faster than creating a new WebAPK for each "create WebAPK" request.

BUG=624059
TEST=WebApkIconHasherTest
==========

[pkotwicz, 2016-08-17 01:16:46]

Robert, can you please take another look?

I have added unit tests for webapk_icon_hasher.cc as you suggested

[Robert Sesek, 2016-08-17 16:06:22]

LGTM w/ a nit

https://codereview.chromium.org/2231843003/diff/220001/chrome/browser/android...
File chrome/browser/android/webapk/webapk_icon_hasher_unittest.cc (right):

https://codereview.chromium.org/2231843003/diff/220001/chrome/browser/android...
chrome/browser/android/webapk/webapk_icon_hasher_unittest.cc:26: const char*
kIconUrl = "/android/google.png";
const char kIconUrl[] will avoid creating a static initializer (and below on
line 29).

[pkotwicz, 2016-08-17 17:31:00]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2016-08-17 17:31:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
yfriedman@chromium.org, rsesek@chromium.org
Link to the patchset: https://codereview.chromium.org/2231843003/#ps240001
(title: "Merge branch 'master' into webapk_builder_impl2_hash")

[commit-bot: I haz the power, 2016-08-17 17:31:28]

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-08-17 18:13:57]

Description was changed from

==========
Take Murmur2 hash of untransformed icon when creating WebAPK

This CL makes WebApkInstaller take the Murmur2 hash of the bitmap at the icon
URL prior to any transformations being applied to the bitmap (such as
encoding/decoding the bitmap). The icon hash is used to determine whether the
icon that the user sees matches the icon of a WebAPK that the server generated
for another user.

The icon could be dynamically generated and:
- use the same icon URL for all users
- be visually different for each user

If the hashes match the server can vend the WebAPK that it
previously generated to other users. Vending previously generated WebAPKs is
faster than creating a new WebAPK for each "create WebAPK" request.

BUG=624059
TEST=WebApkIconHasherTest
==========

to

==========
Take Murmur2 hash of untransformed icon when creating WebAPK

This CL makes WebApkInstaller take the Murmur2 hash of the bitmap at the icon
URL prior to any transformations being applied to the bitmap (such as
encoding/decoding the bitmap). The icon hash is used to determine whether the
icon that the user sees matches the icon of a WebAPK that the server generated
for another user.

The icon could be dynamically generated and:
- use the same icon URL for all users
- be visually different for each user

If the hashes match the server can vend the WebAPK that it
previously generated to other users. Vending previously generated WebAPKs is
faster than creating a new WebAPK for each "create WebAPK" request.

BUG=624059
TEST=WebApkIconHasherTest
==========

[commit-bot: I haz the power, 2016-08-17 18:14:00]

Committed patchset #5 (id:240001)

[commit-bot: I haz the power, 2016-08-17 18:23:49]

Description was changed from

==========
Take Murmur2 hash of untransformed icon when creating WebAPK

This CL makes WebApkInstaller take the Murmur2 hash of the bitmap at the icon
URL prior to any transformations being applied to the bitmap (such as
encoding/decoding the bitmap). The icon hash is used to determine whether the
icon that the user sees matches the icon of a WebAPK that the server generated
for another user.

The icon could be dynamically generated and:
- use the same icon URL for all users
- be visually different for each user

If the hashes match the server can vend the WebAPK that it
previously generated to other users. Vending previously generated WebAPKs is
faster than creating a new WebAPK for each "create WebAPK" request.

BUG=624059
TEST=WebApkIconHasherTest
==========

to

==========
Take Murmur2 hash of untransformed icon when creating WebAPK

This CL makes WebApkInstaller take the Murmur2 hash of the bitmap at the icon
URL prior to any transformations being applied to the bitmap (such as
encoding/decoding the bitmap). The icon hash is used to determine whether the
icon that the user sees matches the icon of a WebAPK that the server generated
for another user.

The icon could be dynamically generated and:
- use the same icon URL for all users
- be visually different for each user

If the hashes match the server can vend the WebAPK that it
previously generated to other users. Vending previously generated WebAPKs is
faster than creating a new WebAPK for each "create WebAPK" request.

BUG=624059
TEST=WebApkIconHasherTest

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

[commit-bot: I haz the power, 2016-08-17 18:23:50]

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