[Payment Request] Fixed potential use-after-free of RenderFrameHost.

InstallablePaymentAppCrawler stores a RenderFrameHost raw pointer and
uses it in asynchronously when DownloadAndDecodeWebAppIcon() is run as
a callback for downloading the web app manifest. This creates a possible
use-after-free situation.

This CL applies a limited fix: the RenderFrameHost* is converted to its
GlobalFrameRoutingId to be stored inside InstallablePaymentAppCrawler.

A better fix is to use WeakPtr<RenderFrameHost> everywhere in payments
code. However, this requires changing the content public API to expose
a RenderFrameHost::GetWeakPtr() method. Since this CL is intended to be
merged to M82, this more risky work is left as a followup for
crbug.com/1058840.

The instantiation of InstallablePaymentAppCrawler happens synchronously
with the instantiation of PaymentRequest, the risk of use-after-free as
a result of PaymentRequest's storage of RenderFrameHost* is small.

Bug: 1061110
Change-Id: I1428c6006201834ee341dd4546bd297116a5f380
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2106387Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Commit-Queue: Danyao Wang <danyao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751454}

components/payments/content/installable_payment_app_crawler.cc

@@ -33,7 +33,6 @@
 namespace payments {
 
 // TODO(crbug.com/782270): Use cache to accelerate crawling procedure.
-// TODO(crbug.com/782270): Add integration tests for this class.
 InstallablePaymentAppCrawler::InstallablePaymentAppCrawler(
     const url::Origin& merchant_origin,
     content::RenderFrameHost* initiator_render_frame_host,
@@ -44,7 +43,13 @@ InstallablePaymentAppCrawler::InstallablePaymentAppCrawler(
     : WebContentsObserver(web_contents),
       log_(web_contents),
       merchant_origin_(merchant_origin),
-      initiator_render_frame_host_(initiator_render_frame_host),
+      initiator_frame_routing_id_(
+          initiator_render_frame_host &&
+                  initiator_render_frame_host->GetProcess()
+              ? content::GlobalFrameRoutingId(
+                    initiator_render_frame_host->GetProcess()->GetID(),
+                    initiator_render_frame_host->GetRoutingID())
+              : content::GlobalFrameRoutingId()),
       downloader_(downloader),
       parser_(parser),
       number_of_payment_method_manifest_to_download_(0),
@@ -407,13 +412,21 @@ void InstallablePaymentAppCrawler::DownloadAndDecodeWebAppIcon(
 
   number_of_web_app_icons_to_download_and_decode_++;
 
-  content::GlobalFrameRoutingId frame_routing_id;
-  if (initiator_render_frame_host_ &&
-      initiator_render_frame_host_->GetProcess()) {
-    frame_routing_id = content::GlobalFrameRoutingId(
-        initiator_render_frame_host_->GetProcess()->GetID(),
-        initiator_render_frame_host_->GetRoutingID());
+  // If the initiator frame doesn't exists any more, e.g. the frame has
+  // navigated away, don't download the icon.
+  // TODO(crbug.com/1058840): Move this sanity check to ManifestIconDownloader
+  // after DownloadImage refactor is done.
+  content::RenderFrameHost* render_frame_host =
+      content::RenderFrameHost::FromID(initiator_frame_routing_id_);
+  if (!render_frame_host || !render_frame_host->IsCurrent() ||
+      content::WebContents::FromRenderFrameHost(render_frame_host) !=
+          web_contents()) {
+    // Shortcircuit to the done callback with an empty bitmap.
+    OnPaymentWebAppIconDownloadAndDecoded(method_manifest_url,
+                                          web_app_manifest_url, SkBitmap());
+    return;
   }
+
   bool can_download_icon = content::ManifestIconDownloader::Download(
       web_contents(), downloader_->FindTestServerURL(best_icon_url),
       IconSizeCalculator::IdealIconHeight(native_view),
@@ -423,7 +436,7 @@ void InstallablePaymentAppCrawler::DownloadAndDecodeWebAppIcon(
           weak_ptr_factory_.GetWeakPtr(), method_manifest_url,
           web_app_manifest_url),
       false, /* square_only */
-      frame_routing_id);
+      initiator_frame_routing_id_);
   DCHECK(can_download_icon);
 }
 

components/payments/content/installable_payment_app_crawler.h

@@ -19,6 +19,7 @@
 #include "components/payments/content/utility/payment_manifest_parser.h"
 #include "components/payments/content/web_app_manifest.h"
 #include "components/payments/core/payment_manifest_downloader.h"
+#include "content/public/browser/global_routing_id.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "third_party/blink/public/mojom/payments/payment_request.mojom.h"
 #include "url/origin.h"
@@ -111,7 +112,7 @@ class InstallablePaymentAppCrawler : public content::WebContentsObserver {
 
   DeveloperConsoleLogger log_;
   const url::Origin merchant_origin_;
-  content::RenderFrameHost* initiator_render_frame_host_;
+  const content::GlobalFrameRoutingId initiator_frame_routing_id_;
   PaymentManifestDownloader* downloader_;
   PaymentManifestParser* parser_;
   FinishedCrawlingCallback callback_;