[Payment] Refetch missing icon for already installed PH.

This cl refetches missing icons for already installed payment apps with
web app manifests. Screencast of the fix is attached to the bug.

Bug: 1069010
Change-Id: Ibd1e191f199295fb8301a4202cc7512b66408f05
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2264860
Commit-Queue: Sahel Sharify <sahel@chromium.org>
Reviewed-by: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#782182}

components/payments/content/installable_payment_app_crawler.cc

@@ -10,6 +10,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/payments/content/icon/icon_size.h"
+#include "components/payments/core/features.h"
 #include "components/payments/core/native_error_strings.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_task_traits.h"
@@ -61,18 +62,32 @@ InstallablePaymentAppCrawler::~InstallablePaymentAppCrawler() {}
 
 void InstallablePaymentAppCrawler::Start(
     const std::vector<mojom::PaymentMethodDataPtr>& requested_method_data,
+    std::set<GURL> method_manifest_urls_for_icon_refetch,
     FinishedCrawlingCallback callback,
     base::OnceClosure finished_using_resources) {
   callback_ = std::move(callback);
   finished_using_resources_ = std::move(finished_using_resources);
 
   std::set<GURL> manifests_to_download;
-  for (const auto& method_data : requested_method_data) {
-    if (!base::IsStringUTF8(method_data->supported_method))
-      continue;
-    GURL url = GURL(method_data->supported_method);
-    if (url.is_valid()) {
-      manifests_to_download.insert(url);
+  if (method_manifest_urls_for_icon_refetch.empty()) {
+    // Crawl for JIT installable web apps.
+    crawling_mode_ = CrawlingMode::kJustInTimeInstallation;
+    for (const auto& method_data : requested_method_data) {
+      if (!base::IsStringUTF8(method_data->supported_method))
+        continue;
+      GURL url = GURL(method_data->supported_method);
+      if (url.is_valid()) {
+        manifests_to_download.insert(url);
+      }
+    }
+  } else {
+    // Crawl to refetch missing icons of already installed apps.
+    crawling_mode_ = CrawlingMode::kMissingIconRefetch;
+    method_manifest_urls_for_icon_refetch_ =
+        std::move(method_manifest_urls_for_icon_refetch);
+    for (const auto& method : method_manifest_urls_for_icon_refetch_) {
+      DCHECK(method.is_valid());
+      manifests_to_download.insert(method);
     }
   }
 
@@ -246,7 +261,10 @@ void InstallablePaymentAppCrawler::OnPaymentWebAppInstallationInfo(
   if (CompleteAndStorePaymentWebAppInfoIfValid(
           method_manifest_url, web_app_manifest_url, std::move(app_info))) {
     if (!DownloadAndDecodeWebAppIcon(method_manifest_url, web_app_manifest_url,
-                                     std::move(icons))) {
+                                     std::move(icons)) &&
+        crawling_mode_ == CrawlingMode::kJustInTimeInstallation &&
+        !base::FeatureList::IsEnabled(
+            features::kAllowJITInstallationWhenAppIconIsMissing)) {
       std::string error_message = base::ReplaceStringPlaceholders(
           errors::kInvalidWebAppIcon, {web_app_manifest_url.spec()}, nullptr);
       SetFirstError(error_message);
@@ -339,7 +357,9 @@ bool InstallablePaymentAppCrawler::CompleteAndStorePaymentWebAppInfoIfValid(
         downloader_->FindTestServerURL(GURL(app_info->sw_scope)).spec();
   }
 
-  installable_apps_[method_manifest_url] = std::move(app_info);
+  if (crawling_mode_ == CrawlingMode::kJustInTimeInstallation) {
+    installable_apps_[method_manifest_url] = std::move(app_info);
+  }
 
   return true;
 }
@@ -463,20 +483,38 @@ void InstallablePaymentAppCrawler::OnPaymentWebAppIconDownloadAndDecoded(
     const GURL& web_app_manifest_url,
     const SkBitmap& icon) {
   number_of_web_app_icons_to_download_and_decode_--;
-  auto it = installable_apps_.find(method_manifest_url);
-  DCHECK(it != installable_apps_.end());
-  DCHECK(IsSameOriginWith(GURL(it->second->sw_scope), web_app_manifest_url));
-  if (icon.drawsNothing()) {
-    log_.Error(
-        "Failed to download or decode the icon from web app manifest \"" +
-        web_app_manifest_url.spec() + "\" for payment handler manifest \"" +
-        method_manifest_url.spec() + "\".");
-    std::string error_message = base::ReplaceStringPlaceholders(
-        errors::kInvalidWebAppIcon, {web_app_manifest_url.spec()}, nullptr);
-    SetFirstError(error_message);
-    installable_apps_.erase(it);
+
+  if (crawling_mode_ == CrawlingMode::kJustInTimeInstallation) {
+    auto it = installable_apps_.find(method_manifest_url);
+    DCHECK(it != installable_apps_.end());
+    DCHECK(IsSameOriginWith(GURL(it->second->sw_scope), web_app_manifest_url));
+    if (icon.drawsNothing() &&
+        !base::FeatureList::IsEnabled(
+            features::kAllowJITInstallationWhenAppIconIsMissing)) {
+      log_.Error(
+          "Failed to download or decode the icon from web app manifest \"" +
+          web_app_manifest_url.spec() + "\" for payment handler manifest \"" +
+          method_manifest_url.spec() + "\".");
+      std::string error_message = base::ReplaceStringPlaceholders(
+          errors::kInvalidWebAppIcon, {web_app_manifest_url.spec()}, nullptr);
+      SetFirstError(error_message);
+      installable_apps_.erase(it);
+    } else {
+      it->second->icon = std::make_unique<SkBitmap>(icon);
+    }
   } else {
-    it->second->icon = std::make_unique<SkBitmap>(icon);
+    DCHECK_EQ(CrawlingMode::kMissingIconRefetch, crawling_mode_);
+    auto it = method_manifest_urls_for_icon_refetch_.find(method_manifest_url);
+    DCHECK(it != method_manifest_urls_for_icon_refetch_.end());
+    if (icon.drawsNothing()) {
+      log_.Warn("Failed to refetch a valid icon from web app manifest \"" +
+                web_app_manifest_url.spec() +
+                "\" for payment handler manifest \"" +
+                method_manifest_url.spec() + "\".");
+    } else {
+      refetched_icons_.insert(std::pair<GURL, std::unique_ptr<SkBitmap>>(
+          web_app_manifest_url, std::make_unique<SkBitmap>(icon)));
+    }
   }
 
   FinishCrawlingPaymentAppsIfReady();
@@ -491,7 +529,8 @@ void InstallablePaymentAppCrawler::FinishCrawlingPaymentAppsIfReady() {
     return;
   }
 
-  std::move(callback_).Run(std::move(installable_apps_), first_error_message_);
+  std::move(callback_).Run(std::move(installable_apps_),
+                           std::move(refetched_icons_), first_error_message_);
   std::move(finished_using_resources_).Run();
 }
 

components/payments/content/installable_payment_app_crawler.h

@@ -40,8 +40,18 @@ class InstallablePaymentAppCrawler : public content::WebContentsObserver {
  public:
   using FinishedCrawlingCallback = base::OnceCallback<void(
       std::map<GURL, std::unique_ptr<WebAppInstallationInfo>>,
+      std::map<GURL, std::unique_ptr<SkBitmap>>,
       const std::string& error_message)>;
 
+  enum class CrawlingMode {
+    // In this mode the crawler will crawl for finding JIT installable payment
+    // apps.
+    kJustInTimeInstallation,
+    // In this mode the crawler will crawl for downloading missing icons for
+    // already installed payment apps.
+    kMissingIconRefetch,
+  };
+
   // |merchant_origin| is the origin of the iframe that created the
   // PaymentRequest object. It is used by security features like
   // 'Sec-Fetch-Site' and 'Cross-Origin-Resource-Policy'.
@@ -66,14 +76,15 @@ class InstallablePaymentAppCrawler : public content::WebContentsObserver {
   // then this object is safe to be deleted.
   void Start(
       const std::vector<mojom::PaymentMethodDataPtr>& requested_method_data,
+      std::set<GURL> method_manifest_urls_for_icon_refetch,
       FinishedCrawlingCallback callback,
       base::OnceClosure finished_using_resources);
 
   void IgnorePortInOriginComparisonForTesting();
 
- private:
   bool IsSameOriginWith(const GURL& a, const GURL& b);
 
+ private:
   void OnPaymentMethodManifestDownloaded(
       const GURL& method_manifest_url,
       const GURL& method_manifest_url_after_redirects,
@@ -127,6 +138,8 @@ class InstallablePaymentAppCrawler : public content::WebContentsObserver {
   size_t number_of_web_app_icons_to_download_and_decode_;
   std::set<GURL> downloaded_web_app_manifests_;
   std::map<GURL, std::unique_ptr<WebAppInstallationInfo>> installable_apps_;
+  std::map<GURL, std::unique_ptr<SkBitmap>> refetched_icons_;
+  std::set<GURL> method_manifest_urls_for_icon_refetch_;
 
   // The first error message (if any) to be forwarded to the merchant when
   // rejecting the promise returned from PaymentRequest.show().
@@ -134,6 +147,8 @@ class InstallablePaymentAppCrawler : public content::WebContentsObserver {
 
   bool ignore_port_in_origin_comparison_for_testing_ = false;
 
+  CrawlingMode crawling_mode_ = CrawlingMode::kJustInTimeInstallation;
+
   base::WeakPtrFactory<InstallablePaymentAppCrawler> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(InstallablePaymentAppCrawler);

components/payments/content/service_worker_payment_app_finder.cc

@@ -22,6 +22,7 @@
 #include "components/payments/core/features.h"
 #include "components/payments/core/method_strings.h"
 #include "components/payments/core/payment_manifest_downloader.h"
+#include "components/payments/core/url_util.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_frame_host.h"
@@ -220,12 +221,35 @@ class SelfDeletingServiceWorkerPaymentAppFinder
                              const std::string& error_message) {
     if (first_error_message_.empty())
       first_error_message_ = error_message;
-    if (apps.empty() && crawler_ != nullptr) {
+
+    std::set<GURL> method_manifest_urls_for_icon_refetch;
+    installed_apps_ = std::move(apps);
+    for (auto& app : installed_apps_) {
+      if (app.second->icon.get() && !app.second->icon.get()->drawsNothing()) {
+        continue;
+      }
+
+      for (const auto& method : app.second->enabled_methods) {
+        // Only payment methods with manifests are eligible for refetching the
+        // icon of their installed payment apps.
+        GURL method_manifest_url = GURL(method);
+        if (!UrlUtil::IsValidUrlBasedPaymentMethodIdentifier(
+                method_manifest_url)) {
+          continue;
+        }
+        method_manifest_urls_for_icon_refetch.insert(method_manifest_url);
+      }
+    }
+
+    if ((installed_apps_.empty() ||
+         !method_manifest_urls_for_icon_refetch.empty()) &&
+        crawler_ != nullptr) {
       // Crawls installable web payment apps if no web payment apps have been
-      // installed.
+      // installed or when an installed app is missing an icon.
       is_payment_app_crawler_finished_using_resources_ = false;
       crawler_->Start(
           requested_method_data_,
+          std::move(method_manifest_urls_for_icon_refetch),
           base::BindOnce(
               &SelfDeletingServiceWorkerPaymentAppFinder::OnPaymentAppsCrawled,
               weak_ptr_factory_.GetWeakPtr()),
@@ -239,18 +263,36 @@ class SelfDeletingServiceWorkerPaymentAppFinder
     crawler_.reset();
 
     std::move(callback_).Run(
-        std::move(apps),
+        std::move(installed_apps_),
         ServiceWorkerPaymentAppFinder::InstallablePaymentApps(),
         first_error_message_);
   }
 
   void OnPaymentAppsCrawled(
       std::map<GURL, std::unique_ptr<WebAppInstallationInfo>> apps_info,
+      std::map<GURL, std::unique_ptr<SkBitmap>> refetched_icons,
       const std::string& error_message) {
     if (first_error_message_.empty())
       first_error_message_ = error_message;
-    std::move(callback_).Run(content::PaymentAppProvider::PaymentApps(),
-                             std::move(apps_info), first_error_message_);
+
+    for (auto& refetched_icon : refetched_icons) {
+      GURL web_app_manifest_url = refetched_icon.first;
+      for (auto& app : installed_apps_) {
+        // It is possible (unlikely) to have multiple apps with same origins.
+        // The proper validation is to store web_app_manifest_url in
+        // StoredPaymentApp and confirm that it is the same as the
+        // web_app_manifest_url from which icon is fetched.
+        if (crawler_->IsSameOriginWith(GURL(app.second->scope),
+                                       web_app_manifest_url)) {
+          // TODO(crbug.com/1069010): Update the payment app database with the
+          // new icon.
+          app.second->icon = std::move(refetched_icon.second);
+          break;
+        }
+      }
+    }
+    std::move(callback_).Run(std::move(installed_apps_), std::move(apps_info),
+                             first_error_message_);
   }
 
   void OnPaymentAppsCrawlerFinishedUsingResources() {
@@ -304,6 +346,8 @@ class SelfDeletingServiceWorkerPaymentAppFinder
 
   bool ignore_port_in_origin_comparison_for_testing_ = false;
 
+  content::PaymentAppProvider::PaymentApps installed_apps_;
+
   base::WeakPtrFactory<SelfDeletingServiceWorkerPaymentAppFinder>
       weak_ptr_factory_{this};
 };

components/payments/core/features.cc

@@ -62,5 +62,9 @@ const base::Feature kDownRankJustInTimePaymentApp{
 const base::Feature kPaymentHandlerPopUpSizeWindow{
     "PaymentHandlerPopUpSizeWindow", base::FEATURE_ENABLED_BY_DEFAULT};
 
+const base::Feature kAllowJITInstallationWhenAppIconIsMissing{
+    "AllowJITInstallationWhenAppIconIsMissing",
+    base::FEATURE_DISABLED_BY_DEFAULT};
+
 }  // namespace features
 }  // namespace payments

components/payments/core/features.h

@@ -72,6 +72,9 @@ extern const base::Feature kDownRankJustInTimePaymentApp;
 // window size.
 extern const base::Feature kPaymentHandlerPopUpSizeWindow;
 
+// Used to test icon refetch for JIT installed apps with missing icons.
+extern const base::Feature kAllowJITInstallationWhenAppIconIsMissing;
+
 }  // namespace features
 }  // namespace payments