Take Murmur2 hash of untransformed icon when creating WebAPK part 2/2

chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/webapk/manifest_upgrade_detector_fetcher.h"
 
 #include <jni.h>
 #include <vector>
 
 #include "base/android/jni_string.h"
 #include "chrome/browser/android/shortcut_helper.h"
-#include "chrome/browser/android/shortcut_info.h"
+#include "chrome/browser/android/webapk/webapk_icon_hasher.h"
 #include "chrome/browser/android/webapk/webapk_web_manifest_checker.h"
 #include "chrome/browser/installable/installable_manager.h"
+#include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/manifest.h"
 #include "jni/ManifestUpgradeDetectorFetcher_jni.h"
 #include "third_party/smhasher/src/MurmurHash2.h"
 #include "ui/gfx/android/java_bitmap.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "url/gurl.h"
 
 using base::android::JavaParamRef;
 using base::android::ScopedJavaLocalRef;
 
 namespace {
 
-// The seed to use for the murmur2 hash.
-const uint64_t kMurmur2HashSeed = 0;
-
-// Computes a murmur2 hash of |bitmap|'s PNG encoded bytes.
-uint64_t ComputeBitmapHash(const SkBitmap& bitmap) {
-  std::vector<unsigned char> png_bytes;
-  gfx::PNGCodec::EncodeBGRASkBitmap(bitmap, false, &png_bytes);
-  return MurmurHash64B(&png_bytes.front(), png_bytes.size(), kMurmur2HashSeed);
-}
-
 // Returns whether the given |url| is within the scope of the |scope| url.
 bool IsInScope(const GURL& url, const GURL& scope) {
   return base::StartsWith(url.spec(), scope.spec(),
                          base::CompareCase::SENSITIVE);
 }
 
 }  // anonymous namespace
 
 jlong Initialize(JNIEnv* env,
                  const JavaParamRef<jobject>& obj,
                  const JavaParamRef<jstring>& java_scope_url,
                  const JavaParamRef<jstring>& java_web_manifest_url) {
   GURL scope(base::android::ConvertJavaStringToUTF8(env, java_scope_url));
   GURL web_manifest_url(base::android::ConvertJavaStringToUTF8(
       env, java_web_manifest_url));
   ManifestUpgradeDetectorFetcher* fetcher =
       new ManifestUpgradeDetectorFetcher(env, obj, scope, web_manifest_url);
   return reinterpret_cast<intptr_t>(fetcher);
 }
 
 ManifestUpgradeDetectorFetcher::ManifestUpgradeDetectorFetcher(
     JNIEnv* env,
     jobject obj,
     const GURL& scope,
     const GURL& web_manifest_url)
     : content::WebContentsObserver(nullptr),
       scope_(scope),
       web_manifest_url_(web_manifest_url),
+      info_(GURL()),
       weak_ptr_factory_(this) {
   java_ref_.Reset(env, obj);
 }
 
 ManifestUpgradeDetectorFetcher::~ManifestUpgradeDetectorFetcher() {
 }
 
 // static
 bool ManifestUpgradeDetectorFetcher::Register(JNIEnv* env) {
   return RegisterNativesImpl(env);
@@ skipping 50 matching lines @@
   // associate with a Web Manifest. In such case, we ignore the empty manifest
   // and continue observing the WebContents's loading until we find a page that
   // links to the Web Manifest that we are looking for.
   // If the manifest URL is different from the current one, we will continue
   // observing too. It is based on our assumption that it is invalid for
   // web developers to change the Web Manifest location. When it does
   // change, we will treat the new Web Manifest as the one of another WebAPK.
   if (data.manifest.IsEmpty() || web_manifest_url_ != data.manifest_url)
     return;
 
-  // TODO(pkotwicz): Tell Java side that the Web Manifest was fetched but the
-  // Web Manifest is not WebAPK-compatible. (http://crbug.com/639536)
-  if (!data.is_installable ||
+  if (data.error_code != NO_ERROR_DETECTED ||

[Xi Han, 2016/09/12 19:19:05]

Why don't check data.is_installable?

[pkotwicz, 2016/09/12 19:43:59]

Because data.is_installable is true if there is a problem with the icon

AppBannerManager::OnDidPerformInstallableCheck() also checks the error code

[Xi Han, 2016/09/12 19:58:49]

Got it, thanks!

       !AreWebManifestUrlsWebApkCompatible(data.manifest)) {
+    OnGotNonWebApkCompatibleManifest();
     return;
   }
 
-  ShortcutInfo info(GURL::EmptyGURL());
-  info.UpdateFromManifest(data.manifest);
-  info.manifest_url = data.manifest_url;
-  info.icon_url = data.icon_url;
-  OnDataAvailable(info, (data.icon ? *data.icon : SkBitmap()));
+  info_.UpdateFromManifest(data.manifest);
+  info_.manifest_url = data.manifest_url;
+  info_.icon_url = data.icon_url;
+  icon_ = *data.icon;
+
+  icon_hasher_.reset(new WebApkIconHasher());
+  Profile* profile =
+      Profile::FromBrowserContext(web_contents()->GetBrowserContext());
+  icon_hasher_->DownloadAndComputeMurmur2Hash(

[dominickn, 2016/09/13 01:09:35]

We already have the parsed icon in data.icon and icon_. It seems quite wasteful
to download it again. Is it possible to split up downloading and computing the
hash so you can just pass the SkBitmap and use its bytes? You would only be
accessing bytes, not decoding anything, so I don't think there's a security
issue.

That may also have the advantage of making this synchronous, so you could just
stack-initialise icon_hasher_ rather than having it as a member.

[pkotwicz, 2016/09/13 22:23:33]

An SkBitmap is a decoded icon. The SkBitmap does not know about the image's
source bytes.

I considered modifying WebContents::DownloadImage() and ImageDownloadCallback to
return a Murmur2 hash of the icon's unmodified bytes. I decided against this
because WebAPKs are an experimental feature.

The icon downloaded by WebApkIconHasher should be in the HTTP cache so the
download should be cheap (no additional network traffic).

I agree that this is wasteful but I do not have a better idea. Dominick, do you
have any ideas?

[dominickn, 2016/09/14 06:58:43]

Acknowledged. I was worried that would be the case. Making
WebContents::DownloadImage return a hash is also suboptimal because then there
would be redundant work for a bunch of other places which use that method.

Instead, would it be sufficient to change InstallableManager and have it run
ManifestIconSelector::FindBestMatchingIcon when
InstallableParams::check_installable is true? Then the upgrade detector would
not fetch an icon, but it would have the URL which it could then pass off to the
hasher. If the hash comes back different we'd need to run InstallableManager
again to get the icon.

This doesn't solve the redownloading when you install the WebAPK initially to
compute the hash for the first time, but it'll make each subsequent upgrade
request more efficient (I'm imagining that WebAPKs won't need to be upgraded too
often, so this will halve the number of image bytes each upgrade request has to
fetch). WDYT?

By the way, this proposal shouldn't be in this CL - we should have metrics for
how often the upgrade detection runs, and how often it says that an upgrade is
needed (and what the reason for the upgrade is). If we find that upgrades hardly
ever happen, it'll be worth optimizing the data they consume. Are there plans to
add upgrade request metrics?

+      profile->GetRequestContext(),
+      data.icon_url,
+      base::Bind(&ManifestUpgradeDetectorFetcher::OnGotIconMurmur2Hash,
+                 weak_ptr_factory_.GetWeakPtr()));
 }
 
-void ManifestUpgradeDetectorFetcher::OnDataAvailable(
+void ManifestUpgradeDetectorFetcher::OnGotIconMurmur2Hash(
+    const std::string& icon_murmur2_hash) {
+  icon_hasher_.reset();
+
+  if (icon_murmur2_hash.empty()) {
+    OnGotNonWebApkCompatibleManifest();
+    return;
+  }
+
+  OnGotWebApkCompatibleManifest(info_, icon_murmur2_hash, icon_);
+}
+
+void ManifestUpgradeDetectorFetcher::OnGotWebApkCompatibleManifest(
     const ShortcutInfo& info,
+    const std::string& icon_murmur2_hash,
     const SkBitmap& icon_bitmap) {
   JNIEnv* env = base::android::AttachCurrentThread();
 
   ScopedJavaLocalRef<jstring> java_url =
       base::android::ConvertUTF8ToJavaString(env, info.url.spec());
   ScopedJavaLocalRef<jstring> java_scope =
       base::android::ConvertUTF8ToJavaString(env, info.scope.spec());
   ScopedJavaLocalRef<jstring> java_name =
       base::android::ConvertUTF16ToJavaString(env, info.name);
   ScopedJavaLocalRef<jstring> java_short_name =
       base::android::ConvertUTF16ToJavaString(env, info.short_name);
   ScopedJavaLocalRef<jstring> java_icon_url =
       base::android::ConvertUTF8ToJavaString(env, info.icon_url.spec());
-  ScopedJavaLocalRef<jobject> java_bitmap;
-  uint64_t icon_murmur2_hash = 0;
-  if (icon_bitmap.getSize()) {
-    java_bitmap = gfx::ConvertToJavaBitmap(&icon_bitmap);
-    // TODO(pkotwicz): Get hash of untransformed icon's bytes (with no
-    // encoding/decoding).
-    icon_murmur2_hash = ComputeBitmapHash(icon_bitmap);
-  }
+  ScopedJavaLocalRef<jstring> java_icon_murmur2_hash =
+      base::android::ConvertUTF8ToJavaString(env, icon_murmur2_hash);
+  ScopedJavaLocalRef<jobject> java_bitmap =
+      gfx::ConvertToJavaBitmap(&icon_bitmap);
 
-  Java_ManifestUpgradeDetectorFetcher_onDataAvailable(
+  Java_ManifestUpgradeDetectorFetcher_onGotWebApkCompatibleManifest(
       env, java_ref_, java_url, java_scope, java_name, java_short_name,
-      java_icon_url, icon_murmur2_hash, java_bitmap, info.display,
+      java_icon_url, java_icon_murmur2_hash, java_bitmap, info.display,
       info.orientation, info.theme_color, info.background_color);
 }
+
+void ManifestUpgradeDetectorFetcher::OnGotNonWebApkCompatibleManifest() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  Java_ManifestUpgradeDetectorFetcher_onGotNonWebApkCompatibleManifest(
+      env, java_ref_);
+}