[Web Payments] Add canMakePayment tests for Secure Payment Confirmation.

Both canMakePayment() and hasEnrolledInstrument() are expected to return
true if a device supports Secure Payment Confirmation: i.e. has a
compatible authenticator, regardless of the presence of payment
credentials. These new tests verify these behaviors.

Also fixed a crash that happens when PaymentRequest is destroyed after
the associated RenderFrameHost. This is new to Secure Payment
Confirmation because PaymentRequest now holds an InternalAuthenticator,
which must be out-lived by the RenderFrameHost. This patch fixes the
bug by observing WebContentsObserver::RenderFrameDeleted() and perform
the necessary clean up before the RenderFrameHost is actually destroyed.

This crash was detected by the new tests, which triggers PaymentRequest
destructor during browser shutdown.

Bug: 1121168
Change-Id: I1e7ab94c3c6fac23ef5e3104beccc1ab1da2f3e5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2375887
Commit-Queue: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#801738}

chrome/browser/payments/secure_payment_confirmation_browsertest.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/command_line.h"
+#include "base/strings/stringprintf.h"
 #include "build/build_config.h"
 #include "chrome/test/payments/payment_request_platform_browsertest_base.h"
 #include "content/public/common/content_switches.h"
@@ -13,8 +14,8 @@
 namespace payments {
 namespace {
 
-static constexpr char kInvokePaymentRequest[] =
-    "getStatusForMethodData([{"
+static constexpr char kTestMethodData[] =
+    "[{ "
     "  supportedMethods: 'secure-payment-confirmation',"
     "  data: {"
     "    action: 'authenticate',"
@@ -22,7 +23,11 @@ static constexpr char kInvokePaymentRequest[] =
     "    networkData: Uint8Array.from('x', c => c.charCodeAt(0)),"
     "    timeout: 60000,"
     "    fallbackUrl: 'https://fallback.example/url'"
-    "}}])";
+    "}}]";
+
+std::string getInvokePaymentRequestSnippet() {
+  return base::StringPrintf("getStatusForMethodData(%s)", kTestMethodData);
+}
 
 class SecurePaymentConfirmationTest
     : public PaymentRequestPlatformBrowserTestBase {
@@ -41,7 +46,8 @@ IN_PROC_BROWSER_TEST_F(SecurePaymentConfirmationTest, NoAuthenticator) {
   // EvalJs waits for JavaScript promise to resolve.
   EXPECT_EQ(
       "The payment method \"secure-payment-confirmation\" is not supported.",
-      content::EvalJs(GetActiveWebContents(), kInvokePaymentRequest));
+      content::EvalJs(GetActiveWebContents(),
+                      getInvokePaymentRequestSnippet()));
 }
 
 #if defined(OS_ANDROID)
@@ -59,7 +65,8 @@ IN_PROC_BROWSER_TEST_F(SecurePaymentConfirmationTest,
 
   // ExecJs starts executing JavaScript and immediately returns, not waiting for
   // any promise to return.
-  EXPECT_TRUE(content::ExecJs(GetActiveWebContents(), kInvokePaymentRequest));
+  EXPECT_TRUE(content::ExecJs(GetActiveWebContents(),
+                              getInvokePaymentRequestSnippet()));
 
   WaitForObservedEvent();
   ASSERT_FALSE(test_controller()->app_descriptions().empty());
@@ -67,6 +74,53 @@ IN_PROC_BROWSER_TEST_F(SecurePaymentConfirmationTest,
   EXPECT_EQ("Stub label", test_controller()->app_descriptions().front().label);
 }
 
+// canMakePayment() and hasEnrolledInstrument() should return false on platforms
+// without a compatible authenticator.
+IN_PROC_BROWSER_TEST_F(SecurePaymentConfirmationTest,
+                       CanMakePayment_NoAuthenticator) {
+  test_controller()->SetHasAuthenticator(false);
+  NavigateTo("a.com", "/can_make_payment_checker.html");
+
+  {
+    std::string snippet =
+        base::StringPrintf("canMakePaymentForMethodData(%s)", kTestMethodData);
+    EXPECT_EQ("false", content::EvalJs(GetActiveWebContents(), snippet));
+  }
+  {
+    std::string snippet = base::StringPrintf(
+        "hasEnrolledInstrumentForMethodData(%s)", kTestMethodData);
+    EXPECT_EQ("false", content::EvalJs(GetActiveWebContents(), snippet));
+  }
+}
+
+// canMakePayment() and hasEnrolledInstrument() should return true on platforms
+// with a compatible authenticator regardless of the presence of payment
+// credentials.
+#if defined(OS_ANDROID)
+// TODO(https://crbug.com/1110320): Implement SetHasAuthenticator() for Android,
+// so this behavior can be tested on Android as well.
+#define MAYBE_CanMakePayment_HasAuthenticator \
+  DISABLED_CanMakePayment_HasAuthenticator
+#else
+#define MAYBE_CanMakePayment_HasAuthenticator CanMakePayment_HasAuthenticator
+#endif  // OS_ANDROID
+IN_PROC_BROWSER_TEST_F(SecurePaymentConfirmationTest,
+                       MAYBE_CanMakePayment_HasAuthenticator) {
+  test_controller()->SetHasAuthenticator(true);
+  NavigateTo("a.com", "/can_make_payment_checker.html");
+
+  {
+    std::string snippet =
+        base::StringPrintf("canMakePaymentForMethodData(%s)", kTestMethodData);
+    EXPECT_EQ("true", content::EvalJs(GetActiveWebContents(), snippet));
+  }
+  {
+    std::string snippet = base::StringPrintf(
+        "hasEnrolledInstrumentForMethodData(%s)", kTestMethodData);
+    EXPECT_EQ("true", content::EvalJs(GetActiveWebContents(), snippet));
+  }
+}
+
 // Intentionally do not enable the "SecurePaymentConfirmation" Blink runtime
 // feature.
 class SecurePaymentConfirmationDisabledTest
@@ -80,7 +134,25 @@ IN_PROC_BROWSER_TEST_F(SecurePaymentConfirmationDisabledTest,
   // EvalJs waits for JavaScript promise to resolve.
   EXPECT_EQ(
       "The payment method \"secure-payment-confirmation\" is not supported.",
-      content::EvalJs(GetActiveWebContents(), kInvokePaymentRequest));
+      content::EvalJs(GetActiveWebContents(),
+                      getInvokePaymentRequestSnippet()));
+}
+
+IN_PROC_BROWSER_TEST_F(SecurePaymentConfirmationDisabledTest,
+                       CannotMakePayment) {
+  test_controller()->SetHasAuthenticator(true);
+  NavigateTo("a.com", "/can_make_payment_checker.html");
+
+  {
+    std::string snippet =
+        base::StringPrintf("canMakePaymentForMethodData(%s)", kTestMethodData);
+    EXPECT_EQ("false", content::EvalJs(GetActiveWebContents(), snippet));
+  }
+  {
+    std::string snippet = base::StringPrintf(
+        "hasEnrolledInstrumentForMethodData(%s)", kTestMethodData);
+    EXPECT_EQ("false", content::EvalJs(GetActiveWebContents(), snippet));
+  }
 }
 
 }  // namespace

components/payments/content/payment_request.cc

@@ -754,6 +754,21 @@ void PaymentRequest::DidStartMainFrameNavigationToDifferentDocument(
                              : JourneyLogger::ABORT_REASON_MERCHANT_NAVIGATION);
 }
 
+void PaymentRequest::RenderFrameDeleted(
+    content::RenderFrameHost* render_frame_host) {
+  DCHECK(render_frame_host ==
+         content::RenderFrameHost::FromID(initiator_frame_routing_id_));
+  // RenderFrameHost is usually deleted explicitly before PaymentRequest
+  // destruction if the user closes the tab or browser window without closing
+  // the payment request dialog.
+  RecordFirstAbortReason(JourneyLogger::ABORT_REASON_ABORTED_BY_USER);
+  // But don't bother sending errors to |client_| because the mojo pipe will be
+  // torn down anyways when RenderFrameHost is destroyed. It's not safe to call
+  // UserCancelled() here because it is not re-entrant.
+  // TODO(crbug.com/1121841) Make UserCancelled re-entrant.
+  OnConnectionTerminated();
+}
+
 void PaymentRequest::OnConnectionTerminated() {
   // We are here because of a browser-side error, or likely as a result of the
   // disconnect_handler on |receiver_|, which can mean that the renderer

components/payments/content/payment_request.h

@@ -111,6 +111,12 @@ class PaymentRequest : public mojom::PaymentRequest,
   // another document, but before the PaymentRequest is destroyed.
   void DidStartMainFrameNavigationToDifferentDocument(bool is_user_initiated);
 
+  // Called when the frame attached to this PaymentRequest is about to be
+  // destroyed. This is used to clean up before the RenderFrameHost is actually
+  // destroyed because some objects held by the PaymentRequest (e.g.
+  // InternalAuthenticator) must be out-lived by the RenderFrameHost.
+  void RenderFrameDeleted(content::RenderFrameHost* render_frame_host);
+
   // As a result of a browser-side error or renderer-initiated mojo channel
   // closure (e.g. there was an error on the renderer side, or payment was
   // successful), this method is called. It is responsible for cleaning up,
@@ -130,6 +136,10 @@ class PaymentRequest : public mojom::PaymentRequest,
 
   content::WebContents* web_contents() { return web_contents_; }
 
+  const content::GlobalFrameRoutingId& initiator_frame_routing_id() {
+    return initiator_frame_routing_id_;
+  }
+
   bool skipped_payment_request_ui() { return skipped_payment_request_ui_; }
   bool is_show_user_gesture() const { return is_show_user_gesture_; }
 

components/payments/content/payment_request_web_contents_manager.cc

@@ -66,6 +66,22 @@ void PaymentRequestWebContentsManager::DidStartNavigation(
   }
 }
 
+void PaymentRequestWebContentsManager::RenderFrameDeleted(
+    content::RenderFrameHost* render_frame_host) {
+  // Two passes to avoid modifying the |payment_requests_| map while iterating
+  // over it.
+  std::vector<PaymentRequest*> obsolete;
+  for (auto& it : payment_requests_) {
+    if (content::RenderFrameHost::FromID(
+            it.second->initiator_frame_routing_id()) == render_frame_host) {
+      obsolete.push_back(it.first);
+    }
+  }
+  for (auto* request : obsolete) {
+    request->RenderFrameDeleted(render_frame_host);
+  }
+}
+
 void PaymentRequestWebContentsManager::DestroyRequest(PaymentRequest* request) {
   request->HideIfNecessary();
   payment_requests_.erase(request);

components/payments/content/payment_request_web_contents_manager.h

@@ -59,6 +59,7 @@ class PaymentRequestWebContentsManager
   // WebContentsObserver::
   void DidStartNavigation(
       content::NavigationHandle* navigation_handle) override;
+  void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
 
  private:
   explicit PaymentRequestWebContentsManager(content::WebContents* web_contents);

components/test/data/payments/can_make_payment_checker.js

@@ -4,6 +4,10 @@
  * found in the LICENSE file.
  */
 
+const kDetails = {
+  total: {label: 'TEST', amount: {currency: 'USD', value: '0.01'}},
+};
+
 /**
  * Checks whether the given payment method can make payments.
  * @param {string} method - The payment method identifier to check.
@@ -11,12 +15,40 @@
  */
 async function canMakePayment(method) { // eslint-disable-line no-unused-vars, max-len
   try {
-    const request = new PaymentRequest(
-        [{supportedMethods: method}],
-        {total: {label: 'TEST', amount: {currency: 'USD', value: '0.01'}}});
+    const request = new PaymentRequest([{supportedMethods: method}], kDetails);
+    const result = await request.canMakePayment();
+    return result ? 'true' : 'false';
+  } catch (e) {
+    return e.message;
+  }
+}
+
+/**
+ * Creates a PaymentRequest with |methodData| and checks canMakePayment.
+ * @param {object} methodData - The payment method data to build the request.
+ * @return {string} - 'true', 'false', or error message on failure.
+ */
+async function canMakePaymentForMethodData(methodData) { // eslint-disable-line no-unused-vars, max-len
+  try {
+    const request = new PaymentRequest(methodData, kDetails);
     const result = await request.canMakePayment();
     return result ? 'true' : 'false';
   } catch (e) {
     return e.message;
   }
 }
+
+/**
+ * Creates a PaymentRequest with |methodData| and checks hasEnrolledInstrument.
+ * @param {object} methodData - The payment method data to build the request.
+ * @return {string} - 'true', 'false', or error message on failure.
+ */
+async function hasEnrolledInstrumentForMethodData(methodData) { // eslint-disable-line no-unused-vars, max-len
+  try {
+    const request = new PaymentRequest(methodData, kDetails);
+    const result = await request.hasEnrolledInstrument();
+    return result ? 'true' : 'false';
+  } catch (e) {
+    return e.message;
+  }
+}