[Web Payment][Android] Check for null web contents & render frame host.

Before this patch, Chrome on Android could attempt to use null
WebContents* or RenderFrameHost* pointer derived from the corresponding
Java objects, while an iframe is being removed, which caused a crash.

This patch adds WebContents.isDestroyed() check in Java and null checks
for RenderFrameHost* pointers in C++ derived from corresponding Java
  objects.

After this patch, Chrome does not crash when an iframe is being removed
on Android during PaymentRequest operation.

Bug: 1125614
Change-Id: Id390a6eceaa3c8ccfcc496583ee82c9e6bb2a20c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2401318
Commit-Queue: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Liquan (Max) Gu <maxlg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805722}

chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactoryInterface.java

@@ -13,6 +13,10 @@ public interface PaymentAppFactoryInterface {
      * 3) Call delegate.onPaymentAppCreated(app) for apps that match the method data.
      * 4) Call delegate.onDoneCreatingPaymentApps(this) exactly once.
      *
+     * If called while the RenderFrameHost object is still available in Java, but its counterparts
+     * has been deleted in C++, then none of the `delegate` methods are expected to be called,
+     * because the frame is being unloaded.
+     *
      * @param delegate Provides information about payment request and receives a list of payment
      * apps.
      */

chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestFactory.java

@@ -132,6 +132,7 @@ public class PaymentRequestFactory implements InterfaceFactory<PaymentRequest> {
         @Override
         public String getInvalidSslCertificateErrorMessage() {
             WebContents webContents = getWebContents();
+            if (webContents == null || webContents.isDestroyed()) return null;
             if (!OriginSecurityChecker.isSchemeCryptographic(webContents.getLastCommittedUrl())) {
                 return null;
             }
@@ -146,7 +147,9 @@ public class PaymentRequestFactory implements InterfaceFactory<PaymentRequest> {
 
         @Override
         public boolean prefsCanMakePayment() {
-            return UserPrefs.get(Profile.fromWebContents(getWebContents()))
+            WebContents webContents = getWebContents();
+            return webContents != null && !webContents.isDestroyed()
+                    && UserPrefs.get(Profile.fromWebContents(webContents))
                                .getBoolean(Pref.CAN_MAKE_PAYMENT_ENABLED);
         }
 
@@ -161,6 +164,7 @@ public class PaymentRequestFactory implements InterfaceFactory<PaymentRequest> {
             return activity != null ? mPackageManager.getTwaPackageName(activity) : null;
         }
 
+        @Nullable
         private WebContents getWebContents() {
             return WebContentsStatics.fromRenderFrameHost(mRenderFrameHost);
         }
@@ -196,6 +200,8 @@ public class PaymentRequestFactory implements InterfaceFactory<PaymentRequest> {
         }
 
         WebContents webContents = WebContentsStatics.fromRenderFrameHost(mRenderFrameHost);
+        if (webContents == null || webContents.isDestroyed()) return new InvalidPaymentRequest();
+
         return ComponentPaymentRequestImpl.createPaymentRequest(mRenderFrameHost,
                 /*isOffTheRecord=*/delegate.isOffTheRecord(webContents),
                 /*skipUiForBasicCard=*/delegate.skipUiForBasicCard(),

chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java

@@ -141,10 +141,10 @@ public class ServiceWorkerPaymentAppBridge {
     /**
      * Notify closing the opened payment app window.
      *
-     * @param webContents The web contents in the opened window.
+     * @param webContents The web contents in the opened window. Can be null.
      */
-    public static void onClosingPaymentAppWindow(WebContents webContents) {
-        if (webContents.isDestroyed()) return;
+    public static void onClosingPaymentAppWindow(@Nullable WebContents webContents) {
+        if (webContents == null || webContents.isDestroyed()) return;
         ServiceWorkerPaymentAppBridgeJni.get().onClosingPaymentAppWindow(
                 webContents, PaymentEventResponseType.PAYMENT_HANDLER_WINDOW_CLOSING);
     }

chrome/browser/payments/BUILD.gn

@@ -15,6 +15,7 @@ source_set("browsertests") {
     "iframe_csp_browsertest.cc",
     "ignore_payment_method_browsertest.cc",
     "journey_logger_browsertest.cc",
+    "load_and_remove_iframe_with_many_payment_requests_browsertest.cc",
     "payment_handler_capabilities_browsertest.cc",
     "payment_handler_change_shipping_address_option_browsertest.cc",
     "payment_handler_enable_delegations_browsertest.cc",

chrome/browser/payments/android/journey_logger_android.cc

@@ -184,6 +184,7 @@ static jlong JNI_JourneyLogger_InitJourneyLoggerAndroid(
     const JavaParamRef<jobject>& jweb_contents) {
   content::WebContents* web_contents =
       content::WebContents::FromJavaWebContents(jweb_contents);
+  DCHECK(web_contents);  // Verified in Java before invoking this function.
   return reinterpret_cast<jlong>(new JourneyLoggerAndroid(
       jis_incognito, ukm::GetSourceIdForWebContentsDocument(web_contents)));
 }

chrome/browser/payments/android/payment_app_service_bridge.cc

@@ -95,6 +95,9 @@ void JNI_PaymentAppServiceBridge_Create(
 
   auto* render_frame_host =
       content::RenderFrameHost::FromJavaRenderFrameHost(jrender_frame_host);
+  if (!render_frame_host)  // The frame is being unloaded.
+    return;
+
   std::string top_origin = ConvertJavaStringToUTF8(jtop_origin);
 
   scoped_refptr<payments::PaymentManifestWebDataService> web_data_service =

chrome/browser/payments/android/service_worker_payment_app_bridge.cc

@@ -124,7 +124,7 @@ static void JNI_ServiceWorkerPaymentAppBridge_OnClosingPaymentAppWindow(
     jint reason) {
   content::WebContents* web_contents =
       content::WebContents::FromJavaWebContents(jweb_contents);
-
+  DCHECK(web_contents);  // Verified in Java before invoking this function.
   content::PaymentAppProvider::GetInstance()->OnClosingOpenedWindow(
       web_contents,
       static_cast<payments::mojom::PaymentEventResponseType>(reason));

chrome/browser/payments/android/ssl_validity_checker_android.cc

@@ -17,6 +17,7 @@ JNI_SslValidityChecker_GetInvalidSslCertificateErrorMessage(
     const base::android::JavaParamRef<jobject>& jweb_contents) {
   content::WebContents* web_contents =
       content::WebContents::FromJavaWebContents(jweb_contents);
+  // SslValidityChecker checks for null `web_contents` parameter.
   return base::android::ConvertUTF8ToJavaString(
       env,
       SslValidityChecker::GetInvalidSslCertificateErrorMessage(web_contents));
@@ -26,6 +27,7 @@ JNI_SslValidityChecker_GetInvalidSslCertificateErrorMessage(
 jboolean JNI_SslValidityChecker_IsValidPageInPaymentHandlerWindow(
     JNIEnv* env,
     const base::android::JavaParamRef<jobject>& jweb_contents) {
+  // SslValidityChecker checks for null `web_contents` parameter.
   return SslValidityChecker::IsValidPageInPaymentHandlerWindow(
       content::WebContents::FromJavaWebContents(jweb_contents));
 }

chrome/browser/payments/load_and_remove_iframe_with_many_payment_requests_browsertest.cc

+// Copyright 2020 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/test/payments/payment_request_platform_browsertest_base.h"
+#include "content/public/test/browser_test.h"
+#include "content/public/test/browser_test_utils.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace payments {
+namespace {
+
+class LoadAndRemoveIframeWithManyPaymentRequestsTest
+    : public PaymentRequestPlatformBrowserTestBase {
+ public:
+  void RunTest(const std::string& iframe_hostname) {
+    NavigateTo("a.com", "/load_and_remove_iframe.html");
+
+    // EvalJs waits for JavaScript promise to resolve.
+    EXPECT_EQ("success",
+              content::EvalJs(GetActiveWebContents(),
+                              content::JsReplace(
+                                  "loadAndRemoveIframe($1, /*timeout=*/100);",
+                                  https_server()
+                                      ->GetURL(iframe_hostname,
+                                               "/create_many_requests.html")
+                                      .spec())));
+  }
+};
+
+IN_PROC_BROWSER_TEST_F(LoadAndRemoveIframeWithManyPaymentRequestsTest,
+                       CrossOriginNoCrash) {
+  RunTest(/*iframe_hostname=*/"b.com");
+}
+
+IN_PROC_BROWSER_TEST_F(LoadAndRemoveIframeWithManyPaymentRequestsTest,
+                       SameOriginNoCrash) {
+  RunTest(/*iframe_hostname=*/"a.com");
+}
+
+}  // namespace
+}  // namespace payments

components/payments/content/android/java/src/org/chromium/components/payments/ComponentPaymentRequestImpl.java

@@ -188,9 +188,9 @@ public class ComponentPaymentRequestImpl {
         assert onClosedListener != null;
 
         WebContents webContents = WebContentsStatics.fromRenderFrameHost(renderFrameHost);
-        if (webContents == null) {
-            abortBeforeInstantiation(client, /*journeyLogger=*/null, ErrorStrings.NO_WEB_CONTENTS,
-                    AbortReason.INVALID_DATA_FROM_RENDERER);
+        if (webContents == null || webContents.isDestroyed()) {
+            abortBeforeInstantiation(/*client=*/null, /*journeyLogger=*/null,
+                    ErrorStrings.NO_WEB_CONTENTS, AbortReason.INVALID_DATA_FROM_RENDERER);
             return null;
         }
 

components/payments/content/android/java/src/org/chromium/components/payments/JourneyLogger.java

@@ -20,7 +20,14 @@ public class JourneyLogger {
 
     private boolean mHasRecorded;
 
+    /**
+     * Creates the journey logger.
+     * @param isIncognito Whether the user profile is incognito.
+     * @param webContents The web contents where PaymentRequest API is invoked. Should not be null.
+     */
     public JourneyLogger(boolean isIncognito, WebContents webContents) {
+        assert webContents != null;
+        assert !webContents.isDestroyed();
         // Note that this pointer could leak the native object. The called must call destroy() to
         // ensure that the native object is destroyed.
         mJourneyLoggerAndroid = JourneyLoggerJni.get().initJourneyLoggerAndroid(

components/test/data/payments/create_many_requests.html

+<!DOCTYPE html>
+<!--
+Copyright 2020 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.
+-->
+<html lang="en">
+<head>
+<meta charset="utf-8">
+<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1">
+<title>Create Many Requests</title>
+</head>
+<body>
+<script src="create_many_requests.js"></script>
+</body>
+</html>

components/test/data/payments/create_many_requests.js

+/*
+ * Copyright 2020 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.
+ */
+
+const supportedInstruments = [{
+  supportedMethods: 'secure-payment-confirmation',
+  data: {
+    'credentialIds': [new ArrayBuffer(4)],
+    'fallbackUrl': 'localhost:8000',
+    'networkData': new ArrayBuffer(4),
+  },
+}];
+
+const details = {
+  total: {label: 'Total', amount: {currency: 'USD', value: '55.00'}},
+};
+
+for (let i = 0; i < 0x1000; i++) {
+  new PaymentRequest(supportedInstruments, details);
+}

components/test/data/payments/load_and_remove_iframe.html

+<!DOCTYPE html>
+<!--
+Copyright 2020 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.
+-->
+<html lang="en">
+<head>
+<meta charset="utf-8">
+<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, minimum-scale=1">
+<title>Remove Iframe</title>
+</head>
+<body>
+<iframe id="ifrm" allow="payment"></iframe>
+<script src="load_and_remove_iframe.js"></script>
+</body>
+</html>

components/test/data/payments/load_and_remove_iframe.js

+/*
+ * Copyright 2020 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.
+ */
+
+/**
+ * Loads the given `url` into an iframe and then removes it after the `timeout`.
+ * @param {string} url - The URL to load into the iframe.
+ * @param {int} timeout - The number of milliseconds to wait before removing the
+ * iframe.
+ * @return {Promise<string>} - The string "success".
+ */
+async function loadAndRemoveIframe(url, timeout) { // eslint-disable-line no-unused-vars, max-len
+  const frame = document.getElementById('ifrm');
+  frame.src = url;
+  return new Promise((resolve) => {
+    frame.onload = () => {
+      window.setTimeout(() => {
+        frame.parentNode.removeChild(frame);
+        resolve('success');
+      }, timeout);
+    };
+  });
+}