[Web Contents] Delete authenticator before render frame.

Before this patch, it was possible to delete the render frame host
before the secure payment confirmation app, which owned an instance of
an authenticator that must outlive the render frame host. This caused a
crash.

This patch makes secure confirmation app observe the render frame host
deletion, so the app can delete the authenticator that it owns before
the associated render frame host is deleted.

The android implementation (currently behind
chrome://flags/#enable-web-platform-experimental-features flag) has been
changed to match the desktop implementation (behind origin trial in
M-86) to create the authenticator on the main frame of the web page, so
cross-origin web pages can exercise the payment credential, too.

After this patch, the authenticator is deleted before the render frame
host, which avoids the crash.

Bug: 1125614
Change-Id: I83502b1b6bf23b838211687497a6498d5c4777ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2410348Reviewed-by: Danyao Wang <danyao@chromium.org>
Reviewed-by: Tommy Martino <tmartino@chromium.org>
Commit-Queue: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#808390}

chrome/browser/autofill/android/internal_authenticator_android.cc

@@ -107,6 +107,10 @@ void InternalAuthenticatorAndroid::Cancel() {
   Java_AuthenticatorImpl_cancel(env, obj);
 }
 
+content::RenderFrameHost* InternalAuthenticatorAndroid::GetRenderFrameHost() {
+  return render_frame_host_;
+}
+
 void InternalAuthenticatorAndroid::InvokeMakeCredentialResponse(
     JNIEnv* env,
     jint status,

chrome/browser/autofill/android/internal_authenticator_android.h

@@ -47,6 +47,7 @@ class InternalAuthenticatorAndroid : public autofill::InternalAuthenticator {
           IsUserVerifyingPlatformAuthenticatorAvailableCallback callback)
       override;
   void Cancel() override;
+  content::RenderFrameHost* GetRenderFrameHost() override;
 
   void InvokeMakeCredentialResponse(
       JNIEnv* env,

chrome/browser/payments/android/payment_app_service_bridge.cc

@@ -252,7 +252,13 @@ PaymentAppServiceBridge::GetMethodData() const {
 
 std::unique_ptr<autofill::InternalAuthenticator>
 PaymentAppServiceBridge::CreateInternalAuthenticator() const {
-  return std::make_unique<InternalAuthenticatorAndroid>(render_frame_host_);
+  // This authenticator can be used in a cross-origin iframe only if the
+  // top-level frame allowed it with Feature Policy, e.g., with allow="payment"
+  // iframe attribute. The secure payment confirmation dialog displays the
+  // top-level origin in its UI before the user can click on the [Verify] button
+  // to invoke this authenticator.
+  return std::make_unique<InternalAuthenticatorAndroid>(
+      web_contents_->GetMainFrame());
 }
 
 scoped_refptr<PaymentManifestWebDataService>

components/autofill/content/browser/webauthn/internal_authenticator_impl.cc

@@ -67,6 +67,10 @@ void InternalAuthenticatorImpl::Cancel() {
   authenticator_common_->Cancel();
 }
 
+content::RenderFrameHost* InternalAuthenticatorImpl::GetRenderFrameHost() {
+  return render_frame_host_;
+}
+
 void InternalAuthenticatorImpl::DidFinishNavigation(
     NavigationHandle* navigation_handle) {
   // If the RenderFrameHost itself is navigated then this function will cause

components/autofill/content/browser/webauthn/internal_authenticator_impl.h

@@ -50,6 +50,7 @@ class InternalAuthenticatorImpl : public autofill::InternalAuthenticator,
           IsUserVerifyingPlatformAuthenticatorAvailableCallback callback)
       override;
   void Cancel() override;
+  content::RenderFrameHost* GetRenderFrameHost() override;
 
   // WebContentsObserver:
   void DidFinishNavigation(NavigationHandle* navigation_handle) override;

components/autofill/core/browser/payments/internal_authenticator.h

@@ -8,6 +8,10 @@
 #include "third_party/blink/public/mojom/webauthn/authenticator.mojom.h"
 #include "url/origin.h"
 
+namespace content {
+class RenderFrameHost;
+}  // namespace content
+
 namespace autofill {
 
 // Interface similar to blink::mojom::Authenticator meant only for internal
@@ -47,6 +51,10 @@ class InternalAuthenticator {
   // Only one MakeCredential or GetAssertion call at a time is allowed,
   // any future calls are cancelled.
   virtual void Cancel() = 0;
+
+  // Returns the non-owned render frame host associated with this authenticator.
+  // Can be used for observing the host's deletion.
+  virtual content::RenderFrameHost* GetRenderFrameHost() = 0;
 };
 
 }  // namespace autofill

components/autofill/core/browser/payments/test_internal_authenticator.cc

@@ -12,4 +12,8 @@ void TestInternalAuthenticator::IsUserVerifyingPlatformAuthenticatorAvailable(
   std::move(callback).Run(false);
 }
 
+content::RenderFrameHost* TestInternalAuthenticator::GetRenderFrameHost() {
+  return nullptr;
+}
+
 }  // namespace autofill

components/autofill/core/browser/payments/test_internal_authenticator.h

@@ -30,6 +30,7 @@ class TestInternalAuthenticator : public InternalAuthenticator {
           IsUserVerifyingPlatformAuthenticatorAvailableCallback callback)
       override;
   void Cancel() override {}
+  content::RenderFrameHost* GetRenderFrameHost() override;
 };
 
 }  // namespace autofill

components/payments/content/secure_payment_confirmation_app.cc

@@ -20,6 +20,7 @@
 #include "components/autofill/core/browser/payments/internal_authenticator.h"
 #include "components/payments/core/method_strings.h"
 #include "components/payments/core/payer_data.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/common/content_features.h"
 #include "crypto/sha2.h"
 #include "device/fido/fido_transport_protocol.h"
@@ -79,6 +80,7 @@ std::vector<uint8_t> GetSecurePaymentConfirmationChallenge(
 }  // namespace
 
 SecurePaymentConfirmationApp::SecurePaymentConfirmationApp(
+    content::WebContents* web_contents_to_observe,
     const std::string& effective_relying_party_identity,
     std::unique_ptr<SkBitmap> icon,
     const base::string16& label,
@@ -88,6 +90,9 @@ SecurePaymentConfirmationApp::SecurePaymentConfirmationApp(
     mojom::SecurePaymentConfirmationRequestPtr request,
     std::unique_ptr<autofill::InternalAuthenticator> authenticator)
     : PaymentApp(/*icon_resource_id=*/0, PaymentApp::Type::INTERNAL),
+      content::WebContentsObserver(web_contents_to_observe),
+      authenticator_render_frame_host_pointer_do_not_dereference_(
+          authenticator->GetRenderFrameHost()),
       effective_relying_party_identity_(effective_relying_party_identity),
       icon_(std::move(icon)),
       label_(label),
@@ -97,6 +102,8 @@ SecurePaymentConfirmationApp::SecurePaymentConfirmationApp(
       total_(total.Clone()),
       request_(std::move(request)),
       authenticator_(std::move(authenticator)) {
+  DCHECK_EQ(web_contents_to_observe->GetMainFrame(),
+            authenticator_render_frame_host_pointer_do_not_dereference_);
   DCHECK(!credential_id_.empty());
 
   app_method_names_.insert(methods::kSecurePaymentConfirmation);
@@ -105,6 +112,9 @@ SecurePaymentConfirmationApp::SecurePaymentConfirmationApp(
 SecurePaymentConfirmationApp::~SecurePaymentConfirmationApp() = default;
 
 void SecurePaymentConfirmationApp::InvokePaymentApp(Delegate* delegate) {
+  if (!authenticator_)
+    return;
+
   auto options = blink::mojom::PublicKeyCredentialRequestOptions::New();
   options->relying_party_id = effective_relying_party_identity_;
   options->timeout = request_->timeout.has_value()
@@ -241,6 +251,15 @@ void SecurePaymentConfirmationApp::AbortPaymentApp(
   std::move(abort_callback).Run(/*abort_success=*/false);
 }
 
+void SecurePaymentConfirmationApp::RenderFrameDeleted(
+    content::RenderFrameHost* render_frame_host) {
+  if (authenticator_render_frame_host_pointer_do_not_dereference_ ==
+      render_frame_host) {
+    // The authenticator requires to be deleted before the render frame.
+    authenticator_.reset();
+  }
+}
+
 void SecurePaymentConfirmationApp::OnGetAssertion(
     Delegate* delegate,
     blink::mojom::AuthenticatorStatus status,

components/payments/content/secure_payment_confirmation_app.h

@@ -15,6 +15,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
 #include "components/payments/content/secure_payment_confirmation_controller.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "third_party/blink/public/mojom/payments/payment_request.mojom.h"
 #include "third_party/blink/public/mojom/webauthn/authenticator.mojom.h"
 #include "url/origin.h"
@@ -25,13 +26,20 @@ namespace autofill {
 class InternalAuthenticator;
 }  // namespace autofill
 
+namespace content {
+class RenderFrameHost;
+class WebContents;
+}  // namespace content
+
 namespace payments {
 
-class SecurePaymentConfirmationApp : public PaymentApp {
+class SecurePaymentConfirmationApp : public PaymentApp,
+                                     public content::WebContentsObserver {
  public:
   // Please use `std::move()` for the `credential_id` parameter to avoid extra
   // copies.
   SecurePaymentConfirmationApp(
+      content::WebContents* web_contents_to_observe,
       const std::string& effective_relying_party_identity,
       std::unique_ptr<SkBitmap> icon,
       const base::string16& label,
@@ -75,12 +83,20 @@ class SecurePaymentConfirmationApp : public PaymentApp {
   void OnPaymentDetailsNotUpdated() override;
   void AbortPaymentApp(base::OnceCallback<void(bool)> abort_callback) override;
 
+  // WebContentsObserver implementation.
+  void RenderFrameDeleted(content::RenderFrameHost* render_frame_host) override;
+
  private:
   void OnGetAssertion(
       Delegate* delegate,
       blink::mojom::AuthenticatorStatus status,
       blink::mojom::GetAssertionAuthenticatorResponsePtr response);
 
+  // Used only for comparison with the RenderFrameHost pointer in
+  // RenderFrameDeleted() method.
+  const content::RenderFrameHost* const
+      authenticator_render_frame_host_pointer_do_not_dereference_;
+
   const std::string effective_relying_party_identity_;
   const std::unique_ptr<SkBitmap> icon_;
   const base::string16 label_;
@@ -89,7 +105,7 @@ class SecurePaymentConfirmationApp : public PaymentApp {
   const url::Origin merchant_origin_;
   const mojom::PaymentCurrencyAmountPtr total_;
   const mojom::SecurePaymentConfirmationRequestPtr request_;
-  const std::unique_ptr<autofill::InternalAuthenticator> authenticator_;
+  std::unique_ptr<autofill::InternalAuthenticator> authenticator_;
 
   base::WeakPtrFactory<SecurePaymentConfirmationApp> weak_ptr_factory_{this};
 };

components/payments/content/secure_payment_confirmation_app_factory.cc

@@ -24,6 +24,8 @@
 #include "components/payments/core/secure_payment_confirmation_instrument.h"
 #include "components/webdata/common/web_data_results.h"
 #include "components/webdata/common/web_data_service_base.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/content_features.h"
 #include "services/data_decoder/public/cpp/decode_image.h"
 #include "third_party/blink/public/mojom/payments/payment_request.mojom.h"
@@ -140,15 +142,17 @@ void SecurePaymentConfirmationAppFactory::Create(
   delegate->OnDoneCreatingPaymentApps();
 }
 
-struct SecurePaymentConfirmationAppFactory::Request {
+struct SecurePaymentConfirmationAppFactory::Request
+    : public content::WebContentsObserver {
   Request(base::WeakPtr<PaymentAppFactory::Delegate> delegate,
           mojom::SecurePaymentConfirmationRequestPtr mojo_request,
           std::unique_ptr<autofill::InternalAuthenticator> authenticator)
-      : delegate(delegate),
+      : content::WebContentsObserver(delegate->GetWebContents()),
+        delegate(delegate),
         mojo_request(std::move(mojo_request)),
         authenticator(std::move(authenticator)) {}
 
-  ~Request() = default;
+  ~Request() override = default;
 
   Request(const Request& other) = delete;
   Request& operator=(const Request& other) = delete;
@@ -168,7 +172,7 @@ void SecurePaymentConfirmationAppFactory::OnWebDataServiceRequestDone(
   std::unique_ptr<Request> request = std::move(iterator->second);
   requests_.erase(iterator);
   DCHECK(request.get());
-  if (!request->delegate)
+  if (!request->delegate || !request->web_contents())
     return;
 
   if (!result || result->GetType() != SECURE_PAYMENT_CONFIRMATION) {
@@ -206,12 +210,22 @@ void SecurePaymentConfirmationAppFactory::OnAppIconDecoded(
     std::unique_ptr<SecurePaymentConfirmationInstrument> instrument,
     std::unique_ptr<Request> request,
     const SkBitmap& decoded_icon) {
+  if (!request->delegate || !request->web_contents())
+    return;
+
+  if (request->authenticator->GetRenderFrameHost() !=
+      request->web_contents()->GetMainFrame()) {
+    request->delegate->OnDoneCreatingPaymentApps();
+    return;
+  }
+
   DCHECK(!decoded_icon.drawsNothing());
   auto icon = std::make_unique<SkBitmap>(decoded_icon);
 
   request->delegate->OnPaymentAppCreated(
       std::make_unique<SecurePaymentConfirmationApp>(
-          instrument->relying_party_id, std::move(icon), instrument->label,
+          request->web_contents(), instrument->relying_party_id,
+          std::move(icon), instrument->label,
           std::move(instrument->credential_id),
           url::Origin::Create(request->delegate->GetTopOrigin()),
           request->delegate->GetSpec()->details().total->amount,

components/payments/content/secure_payment_confirmation_app_unittest.cc

@@ -4,10 +4,16 @@
 
 #include "components/payments/content/secure_payment_confirmation_app.h"
 
+#include <memory>
+
 #include "base/base64.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/autofill/core/browser/payments/internal_authenticator.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_task_environment.h"
+#include "content/public/test/test_browser_context.h"
+#include "content/public/test/test_web_contents_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/mojom/webauthn/authenticator.mojom.h"
@@ -24,6 +30,10 @@ static constexpr char kCredentialIdBase64[] = "cccc";
 
 class MockAuthenticator : public autofill::InternalAuthenticator {
  public:
+  MockAuthenticator()
+      : web_contents_(web_contents_factory_.CreateWebContents(&context_)) {}
+  ~MockAuthenticator() override = default;
+
   MOCK_METHOD1(SetEffectiveOrigin, void(const url::Origin&));
   MOCK_METHOD2(
       MakeCredential,
@@ -35,6 +45,10 @@ class MockAuthenticator : public autofill::InternalAuthenticator {
   MOCK_METHOD0(Cancel, void());
   MOCK_METHOD1(VerifyChallenge, void(const std::vector<uint8_t>&));
 
+  content::RenderFrameHost* GetRenderFrameHost() override {
+    return web_contents_->GetMainFrame();
+  }
+
   // Implements an autofill::InternalAuthenticator method to delegate fields of
   // |options| to gmock methods for easier verification.
   void GetAssertion(
@@ -42,6 +56,14 @@ class MockAuthenticator : public autofill::InternalAuthenticator {
       blink::mojom::Authenticator::GetAssertionCallback callback) override {
     VerifyChallenge(options->challenge);
   }
+
+  content::WebContents* web_contents() { return web_contents_; }
+
+ private:
+  content::BrowserTaskEnvironment task_environment_;
+  content::TestBrowserContext context_;
+  content::TestWebContentsFactory web_contents_factory_;
+  content::WebContents* web_contents_;  // Owned by `web_contents_factory_`.
 };
 
 class SecurePaymentConfirmationAppTest : public testing::Test {
@@ -77,9 +99,10 @@ TEST_F(SecurePaymentConfirmationAppTest, Smoke) {
 
   auto authenticator = std::make_unique<MockAuthenticator>();
   MockAuthenticator* mock_authenticator = authenticator.get();
+  content::WebContents* web_contents = authenticator->web_contents();
 
   SecurePaymentConfirmationApp app(
-      "effective_rp.example",
+      web_contents, "effective_rp.example",
       /*icon=*/std::make_unique<SkBitmap>(), label_, std::move(credential_id),
       url::Origin::Create(GURL("https://merchant.example")), total_,
       MakeRequest(), std::move(authenticator));