From ae3143abb013464882b25c5ad5b5a569c21e596c Mon Sep 17 00:00:00 2001
From: Luis Antunes <luisantunes@google.com>
Date: Tue, 28 Jul 2026 11:38:43 -0700
Subject: [PATCH] [Payments] Fix UAF in PaymentRequest::ShowErrorMessageAndAbortPayment

`delegate_->ShowErrorMessage()` can trigger synchronous teardown of the
hosting `WebContents`. This destroys the `PaymentRequest` object while
it is still executing `ShowErrorMessageAndAbortPayment()`, causing a UAF
when subsequently notifying observers. This CL prevents the UAF by
saving `observer_for_testing_` to a local variable before closing the
dialog so that it can be safely invoked even if `this` is destroyed.

Fixed: 533053621
Change-Id: Idecfb82f56695ea4b07cb372ea38bad3f97cc6b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8144841
Reviewed-by: Darwin Yang <darwinyang@chromium.org>
Commit-Queue: Luis Antunes <luisantunes@google.com>
Cr-Commit-Position: refs/heads/main@{#1669631}
---

diff --git a/components/payments/content/payment_request.cc b/components/payments/content/payment_request.cc
index 5b270bc..2b1b2821 100644
--- a/components/payments/content/payment_request.cc
+++ b/components/payments/content/payment_request.cc
@@ -1290,9 +1290,14 @@
   if (display_handle_ && display_handle_->was_shown()) {
     // Will invoke OnUserCancelled() asynchronously when the user closes the
     // error message UI.
+    // ShowErrorMessage() can synchronously close the dialog and destroy `this`.
+    // We save `observer_for_testing_` locally so it can be safely invoked
+    // even during teardown. Do not access `this` below this point.
+    base::WeakPtr<ObserverForTest> observer = observer_for_testing_;
     delegate_->ShowErrorMessage();
-    if (observer_for_testing_)
-      observer_for_testing_->OnErrorDisplayed();
+    if (observer) {
+      observer->OnErrorDisplayed();
+    }
   } else {
     // Only app store billing apps do not display any browser payment UI.
     DCHECK(spec_->IsAppStoreBillingAlsoRequested());
diff --git a/components/payments/content/payment_request_unittest.cc b/components/payments/content/payment_request_unittest.cc
index 4465d72..21d8a88 100644
--- a/components/payments/content/payment_request_unittest.cc
+++ b/components/payments/content/payment_request_unittest.cc
@@ -267,6 +267,71 @@
   EXPECT_FALSE(bad_message_observer.got_bad_message());
 }
 
+class MockObserverForTest : public PaymentRequest::ObserverForTest {
+ public:
+  MOCK_METHOD(void, OnErrorDisplayed, (), (override));
+
+  void OnCanMakePaymentCalled() override {}
+  void OnCanMakePaymentReturned() override {}
+  void OnHasEnrolledInstrumentCalled() override {}
+  void OnHasEnrolledInstrumentReturned() override {}
+  void OnNotSupportedError() override {}
+  void OnConnectionTerminated() override {}
+  void OnPayCalled() override {}
+  void OnAbortCalled() override {}
+  void OnInternalError() override {}
+
+  base::WeakPtr<MockObserverForTest> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
+ private:
+  base::WeakPtrFactory<MockObserverForTest> weak_ptr_factory_{this};
+};
+
+// Tests that PaymentRequest handles the case where showing the error message
+// synchronously destroys the hosting WebContents (and therefore the
+// PaymentRequest itself), which can happen when closing the dialog widget
+// shifts activation to an observer that closes the payment window.
+TEST_P(PaymentRequestValidationTest,
+       CompleteFailWithSynchronousWebContentsDestruction) {
+  auto delegate = CreateMockDelegate();
+  auto* delegate_ptr = delegate.get();
+
+  mojo::Remote<mojom::PaymentRequest> payment_request;
+  PaymentRequest* request = new PaymentRequest(
+      std::move(delegate), payment_request.BindNewPipeAndPassReceiver());
+  base::WeakPtr<PaymentRequest> weak_request = request->GetWeakPtr();
+
+  MockObserverForTest mock_observer;
+  request->set_observer_for_test(mock_observer.GetWeakPtr());
+
+  mojo::PendingRemote<mojom::PaymentRequestClient> client_remote;
+  auto dummy_receiver = client_remote.InitWithNewPipeAndPassReceiver();
+
+  std::vector<mojom::PaymentMethodDataPtr> method_data;
+  method_data.push_back(CreateSpcMethodData());
+
+  payment_request->Init(std::move(client_remote), std::move(method_data),
+                        CreateDummyDetails(), mojom::PaymentOptions::New());
+  payment_request->Show(/*wait_for_updated_details=*/false,
+                        /*had_user_activation=*/true);
+
+  // Due to the observer local copy trick, the observer still gets notified
+  // even if the PaymentRequest is synchronously destroyed.
+  EXPECT_CALL(mock_observer, OnErrorDisplayed()).Times(1);
+
+  EXPECT_CALL(*delegate_ptr, ShowErrorMessage()).WillOnce([this] {
+    DeleteContents();
+  });
+
+  payment_request->Complete(mojom::PaymentComplete::FAIL);
+  payment_request.FlushForTesting();
+
+  // The PaymentRequest was safely destroyed without triggering the UAF.
+  EXPECT_FALSE(weak_request);
+}
+
 INSTANTIATE_TEST_SUITE_P(All, PaymentRequestValidationTest, testing::Bool());
 
 }  // namespace
diff --git a/components/payments/core/payment_request_delegate.h b/components/payments/core/payment_request_delegate.h
index 95cfa66d..5173302 100644
--- a/components/payments/core/payment_request_delegate.h
+++ b/components/payments/core/payment_request_delegate.h
@@ -34,7 +34,9 @@
   virtual void CloseDialog() = 0;
 
   // Disables the dialog and shows an error message that the transaction has
-  // failed.
+  // failed. Warning: This method may result in the synchronous teardown of
+  // the PaymentRequest object. Callers should not access their members after
+  // calling this method.
   virtual void ShowErrorMessage() = 0;
 
   // Disables user interaction by showing a spinner.

diff --git a/components/payments/content/payment_request_unittest.cc b/components/payments/content/payment_request_unittest.cc
index 4465d72..21d8a88 100644
--- a/components/payments/content/payment_request_unittest.cc
+++ b/components/payments/content/payment_request_unittest.cc
@@ -267,6 +267,71 @@
   EXPECT_FALSE(bad_message_observer.got_bad_message());
 }
 
+class MockObserverForTest : public PaymentRequest::ObserverForTest {
+ public:
+  MOCK_METHOD(void, OnErrorDisplayed, (), (override));
+
+  void OnCanMakePaymentCalled() override {}
+  void OnCanMakePaymentReturned() override {}
+  void OnHasEnrolledInstrumentCalled() override {}
+  void OnHasEnrolledInstrumentReturned() override {}
+  void OnNotSupportedError() override {}
+  void OnConnectionTerminated() override {}
+  void OnPayCalled() override {}
+  void OnAbortCalled() override {}
+  void OnInternalError() override {}
+
+  base::WeakPtr<MockObserverForTest> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
+ private:
+  base::WeakPtrFactory<MockObserverForTest> weak_ptr_factory_{this};
+};
+
+// Tests that PaymentRequest handles the case where showing the error message
+// synchronously destroys the hosting WebContents (and therefore the
+// PaymentRequest itself), which can happen when closing the dialog widget
+// shifts activation to an observer that closes the payment window.
+TEST_P(PaymentRequestValidationTest,
+       CompleteFailWithSynchronousWebContentsDestruction) {
+  auto delegate = CreateMockDelegate();
+  auto* delegate_ptr = delegate.get();
+
+  mojo::Remote<mojom::PaymentRequest> payment_request;
+  PaymentRequest* request = new PaymentRequest(
+      std::move(delegate), payment_request.BindNewPipeAndPassReceiver());
+  base::WeakPtr<PaymentRequest> weak_request = request->GetWeakPtr();
+
+  MockObserverForTest mock_observer;
+  request->set_observer_for_test(mock_observer.GetWeakPtr());
+
+  mojo::PendingRemote<mojom::PaymentRequestClient> client_remote;
+  auto dummy_receiver = client_remote.InitWithNewPipeAndPassReceiver();
+
+  std::vector<mojom::PaymentMethodDataPtr> method_data;
+  method_data.push_back(CreateSpcMethodData());
+
+  payment_request->Init(std::move(client_remote), std::move(method_data),
+                        CreateDummyDetails(), mojom::PaymentOptions::New());
+  payment_request->Show(/*wait_for_updated_details=*/false,
+                        /*had_user_activation=*/true);
+
+  // Due to the observer local copy trick, the observer still gets notified
+  // even if the PaymentRequest is synchronously destroyed.
+  EXPECT_CALL(mock_observer, OnErrorDisplayed()).Times(1);
+
+  EXPECT_CALL(*delegate_ptr, ShowErrorMessage()).WillOnce([this] {
+    DeleteContents();
+  });
+
+  payment_request->Complete(mojom::PaymentComplete::FAIL);
+  payment_request.FlushForTesting();
+
+  // The PaymentRequest was safely destroyed without triggering the UAF.
+  EXPECT_FALSE(weak_request);
+}
+
 INSTANTIATE_TEST_SUITE_P(All, PaymentRequestValidationTest, testing::Bool());
 
 }  // namespace