From 0609c16f1f026d8c5ec0519671cfc8f943a64bcf Mon Sep 17 00:00:00 2001
From: Stephen McGruer <[email protected]>
Date: Fri, 16 Oct 2026 08:20:12 -0700
Subject: [PATCH] [Payment Handler] Avoid UAF when synchronously failing window size check

When PaymentRequest.show() is called inside a Document
Picture-in-Picture window, creating and showing the
PaymentRequestDialogView child widget causes
PipChildDialogObserverHelper to synchronously enlarge the PiP window
bounds before ShowDialog() returns. That synchronous bounds change fires
PaymentRequestDialogView::OnWidgetBoundsChanged(), which calls
CheckIfDialogFitsInBrowserWindow().

CheckIfDialogFitsInBrowserWindow will then fail for a PiP window (which
is another bug - crbug.com/573056666) as the auto-resize doesn't include
our 5% safety margin. This failing then synchronously invokes
OnInternalError(), which calls ResetAndDeleteThis(). The PaymentRequest
object is thus destroyed while PaymentRequest::Show() is still on the
call stack.

This CL moves the asynchronous PostTask into
PaymentRequest::OnInternalError() so that all callers avoid synchronous
self-deletion. It also removes the obsolete TODO(crbug.com/483395708)
comment, as there is no point in it now that OnInternalError has to be
async for other callers too.

Fixed: 572422506
Test: browser_tests --gtest_filter="PaymentRequestWindowSizeTest.*"
Change-Id: I25afd5bb2bbbdacf42d449002f8510ab2b503b73
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8562498
Commit-Queue: Xuehui Chen <[email protected]>
Reviewed-by: Slobodan Pejic <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1720717}
---
diff --git a/chrome/browser/ui/views/payments/payment_request_dialog_view.cc b/chrome/browser/ui/views/payments/payment_request_dialog_view.cc
index 78496c8..a7bd77af 100644
--- a/chrome/browser/ui/views/payments/payment_request_dialog_view.cc
+++ b/chrome/browser/ui/views/payments/payment_request_dialog_view.cc
@@ -158,15 +158,7 @@
     VLOG(2) << "ShowDialog: Rejected because dialog does not fit";
     request_->SetWindowSizeCheckRejectionReason(
         JourneyLogger::WindowSizeCheckRejectionReason::kRejectedAtShow);
-
-    // To avoid tearing down the PaymentRequest class in the middle of showing
-    // the dialog, we post this call asynchronously.
-    //
-    // TODO(crbug.com/483395708): Refactor PaymentRequest::Show so that it can
-    // handle this case synchronously, e.g. via a bool return parameter.
-    base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
-        FROM_HERE, base::BindOnce(&PaymentRequest::OnInternalError, request_,
-                                  errors::kBrowserWindowTooSmall));
+    request_->OnInternalError(errors::kBrowserWindowTooSmall);
 
     // Because we never handed over ownership of this object to the widget in
     // ShowWebModalDialogViews, we need to manually delete ourselves.
diff --git a/components/payments/content/payment_request.cc b/components/payments/content/payment_request.cc
index 90e8339..45f07b78 100644
--- a/components/payments/content/payment_request.cc
+++ b/components/payments/content/payment_request.cc
@@ -14,6 +14,7 @@
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/strings/string_util.h"
+#include "base/task/sequenced_task_runner.h"
 #include "components/payments/content/content_payment_request_delegate.h"
 #include "components/payments/content/has_enrolled_instrument_query_factory.h"
 #include "components/payments/content/payment_app.h"
@@ -1004,21 +1005,13 @@
 }
 
 void PaymentRequest::OnInternalError(const std::string& error_message) {
-  // If |client_| is not bound, then the object is already being destroyed as
-  // a result of a renderer event.
-  if (!client_.is_bound()) {
-    return;
-  }
-
-  RecordFirstAbortReason(JourneyLogger::ABORT_REASON_INTERNAL_ERROR);
-
-  client_->OnError(mojom::PaymentErrorReason::NOT_SUPPORTED, error_message);
-
-  if (observer_for_testing_) {
-    observer_for_testing_->OnInternalError();
-  }
-
-  ResetAndDeleteThis();
+  // To avoid tearing down the PaymentRequest class while it or its delegate
+  // may still be on the call stack (e.g., during ShowDialog() or if showing
+  // the dialog triggers a synchronous browser window resize), post the error
+  // handling and self-deletion asynchronously.
+  base::SequencedTaskRunner::GetCurrentDefault()->PostTask(
+      FROM_HERE, base::BindOnce(&PaymentRequest::OnInternalErrorImpl,
+                                weak_ptr_factory_.GetWeakPtr(), error_message));
 }
 
 void PaymentRequest::SetWindowSizeCheckRejectionReason(
@@ -1281,6 +1274,24 @@
   }
 }
 
+void PaymentRequest::OnInternalErrorImpl(const std::string& error_message) {
+  // If |client_| is not bound, then the object is already being destroyed as
+  // a result of a renderer event.
+  if (!client_.is_bound()) {
+    return;
+  }
+
+  RecordFirstAbortReason(JourneyLogger::ABORT_REASON_INTERNAL_ERROR);
+
+  client_->OnError(mojom::PaymentErrorReason::NOT_SUPPORTED, error_message);
+
+  if (observer_for_testing_) {
+    observer_for_testing_->OnInternalError();
+  }
+
+  ResetAndDeleteThis();
+}
+
 void PaymentRequest::ShowErrorMessageAndAbortPayment() {
   // Note that both branches of the if-else will invoke the OnUserCancelled()
   // method.
diff --git a/components/payments/content/payment_request.h b/components/payments/content/payment_request.h
index a7e61e7f..a0e7b91 100644
--- a/components/payments/content/payment_request.h
+++ b/components/payments/content/payment_request.h
@@ -122,7 +122,8 @@
   void OnPayerInfoSelected(mojom::PayerDetailPtr payer_info) override;
 
   // Called when the Payment Request is being aborted due to an internal error,
-  // such as the browser window being too small to show the UX.
+  // such as the browser window being too small to show the UX. Posts a task to
+  // notify the client and destroy this object asynchronously.
   void OnInternalError(const std::string& error_message);
 
   // Called when the user wants to authenticate in a different way. This is
@@ -240,6 +241,10 @@
 
   void OnAbortResult(bool aborted);
 
+  // Notifies the renderer client of an internal error and destroys `this`.
+  // Posted as a task by `OnInternalError()`.
+  void OnInternalErrorImpl(const std::string& error_message);
+
   // Show an error message in the UI (if available) and abort payment.
   void ShowErrorMessageAndAbortPayment();
 
diff --git a/chrome/browser/ui/views/payments/payment_request_window_size_browsertest.cc b/chrome/browser/ui/views/payments/payment_request_window_size_browsertest.cc
index c936798..620ebec 100644
--- a/chrome/browser/ui/views/payments/payment_request_window_size_browsertest.cc
+++ b/chrome/browser/ui/views/payments/payment_request_window_size_browsertest.cc
@@ -5,15 +5,18 @@
 #include "base/numerics/safe_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "build/build_config.h"
+#include "chrome/browser/picture_in_picture/picture_in_picture_window_manager.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/browser_window/public/browser_window_interface.h"
 #include "chrome/browser/ui/views/payments/payment_request_browsertest_base.h"
+#include "chrome/browser/ui/views/payments/payment_request_views_util.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/payments/core/features.h"
 #include "components/payments/core/journey_logger.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
 #include "ui/gfx/geometry/rect.h"
+#include "ui/views/test/widget_test_api.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
@@ -95,7 +98,9 @@
   EXPECT_NE(nullptr, dialog_view());
 
   // Resize the window to be too small.
-  ResetEventWaiter(DialogEvent::INTERNAL_ERROR);
+  ResetEventWaiterForSequence(
+      {DialogEvent::DIALOG_SIZE_CHECK_AFTER_BROWSER_RESIZE,
+       DialogEvent::INTERNAL_ERROR});
   ui_test_utils::SetAndWaitForBounds(*browser(), gfx::Rect(0, 0, 100, 100));
 
   // We need to wait for the throttle timer (100ms).
@@ -178,4 +183,74 @@
       JourneyLogger::WindowSizeCheckRejectionReason::kRejectedAtResize, 0);
 }
 
+// Regression test for crbug.com/572422506 where synchronous Picture-in-Picture
+// window auto-enlarging caused a UAF via CheckIfDialogFitsInBrowserWindow.
+IN_PROC_BROWSER_TEST_F(PaymentRequestWindowSizeTest,
+                       RejectInAutoEnlargedPictureInPictureWindow) {
+  std::string method_name;
+  InstallPaymentApp("a.com", "/payment_request_success_responder.js",
+                    &method_name);
+  NavigateTo("/payment_request_no_shipping_test.html");
+
+  // A newly created Document Picture-in-Picture window has a committed URL of
+  // about:blank, which UrlUtil::IsOriginAllowedToUseWebPaymentApis() rejects.
+  // Using document.open() followed by history.replaceState() updates the PiP
+  // main frame's committed URL to the opener's HTTPS URL via a same-document
+  // navigation without closing the PiP window.
+  //
+  // TODO(crbug.com/573056666): Either disallow Payment Request in PiP entirely,
+  // or fix it so that this hack is not needed.
+  ASSERT_EQ(true, content::EvalJs(
+                      GetActiveWebContents(),
+                      "documentPictureInPicture.requestWindow().then(w => { "
+                      "w.document.open(); "
+                      "w.document.close(); "
+                      "w.history.replaceState(null, '', location.href); "
+                      "return true; })"));
+
+  content::WebContents* pip_web_contents =
+      PictureInPictureWindowManager::GetInstance()->GetChildWebContents();
+  ASSERT_NE(nullptr, pip_web_contents);
+
+  views::Widget* pip_widget = views::Widget::GetTopLevelWidgetForNativeView(
+      pip_web_contents->GetNativeView());
+  ASSERT_NE(nullptr, pip_widget);
+
+  // Size the PiP window to the minimum pre-widget size required by ShowDialog()
+  // (1.05 * kDialogMinWidth x kDialogHeight, with a 0 top offset). When the
+  // child dialog widget is shown, PipChildDialogObserverHelper synchronously
+  // enlarges the PiP window to fit the unscaled dialog plus title bar padding
+  // (without the 5% safety buffer), which then fails the post-widget size check
+  // in CheckIfDialogFitsInBrowserWindow().
+  const int width = base::ClampRound(kDialogMinWidth * 1.05f);
+  const int height = base::ClampRound(kDialogHeight * 1.05f);
+  views::AsyncWidgetRequestWaiter bounds_waiter(*pip_widget);
+  pip_widget->SetBounds(gfx::Rect(0, 0, width, height));
+  bounds_waiter.Wait();
+
+  base::HistogramTester histograms;
+
+  ResetEventWaiterForSequence(
+      {DialogEvent::PROCESSING_SPINNER_SHOWN,
+       DialogEvent::DIALOG_SIZE_CHECK_AFTER_BROWSER_RESIZE,
+       DialogEvent::INTERNAL_ERROR});
+
+  EXPECT_EQ(
+      "NotSupportedError: Browser window is too small for Payment Request UI.",
+      content::EvalJs(
+          pip_web_contents,
+          content::JsReplace(
+              "const request = new PaymentRequest([{supportedMethods: $1}], "
+              "{total: {label: 'Total', amount: {currency: 'USD', value: "
+              "'1.00'}}}); "
+              "request.show().catch(e => e.toString());",
+              method_name)));
+
+  ASSERT_TRUE(WaitForObservedEvent());
+
+  histograms.ExpectUniqueSample(
+      "PaymentRequest.WindowSizeCheckRejectionReason",
+      JourneyLogger::WindowSizeCheckRejectionReason::kRejectedAtResize, 1);
+}
+
 }  // namespace payments

diff --git a/chrome/browser/ui/views/payments/payment_request_window_size_browsertest.cc b/chrome/browser/ui/views/payments/payment_request_window_size_browsertest.cc
index c936798..620ebec 100644
--- a/chrome/browser/ui/views/payments/payment_request_window_size_browsertest.cc
+++ b/chrome/browser/ui/views/payments/payment_request_window_size_browsertest.cc
@@ -5,15 +5,18 @@
 #include "base/numerics/safe_conversions.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "build/build_config.h"
+#include "chrome/browser/picture_in_picture/picture_in_picture_window_manager.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/browser_window/public/browser_window_interface.h"
 #include "chrome/browser/ui/views/payments/payment_request_browsertest_base.h"
+#include "chrome/browser/ui/views/payments/payment_request_views_util.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/payments/core/features.h"
 #include "components/payments/core/journey_logger.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
 #include "ui/gfx/geometry/rect.h"
+#include "ui/views/test/widget_test_api.h"
 #include "ui/views/view.h"
 #include "ui/views/widget/widget.h"
 
@@ -95,7 +98,9 @@
   EXPECT_NE(nullptr, dialog_view());
 
   // Resize the window to be too small.
-  ResetEventWaiter(DialogEvent::INTERNAL_ERROR);
+  ResetEventWaiterForSequence(
+      {DialogEvent::DIALOG_SIZE_CHECK_AFTER_BROWSER_RESIZE,
+       DialogEvent::INTERNAL_ERROR});
   ui_test_utils::SetAndWaitForBounds(*browser(), gfx::Rect(0, 0, 100, 100));
 
   // We need to wait for the throttle timer (100ms).
@@ -178,4 +183,74 @@
       JourneyLogger::WindowSizeCheckRejectionReason::kRejectedAtResize, 0);
 }
 
+// Regression test for crbug.com/572422506 where synchronous Picture-in-Picture
+// window auto-enlarging caused a UAF via CheckIfDialogFitsInBrowserWindow.
+IN_PROC_BROWSER_TEST_F(PaymentRequestWindowSizeTest,
+                       RejectInAutoEnlargedPictureInPictureWindow) {
+  std::string method_name;
+  InstallPaymentApp("a.com", "/payment_request_success_responder.js",
+                    &method_name);
+  NavigateTo("/payment_request_no_shipping_test.html");
+
+  // A newly created Document Picture-in-Picture window has a committed URL of
+  // about:blank, which UrlUtil::IsOriginAllowedToUseWebPaymentApis() rejects.
+  // Using document.open() followed by history.replaceState() updates the PiP
+  // main frame's committed URL to the opener's HTTPS URL via a same-document
+  // navigation without closing the PiP window.
+  //
+  // TODO(crbug.com/573056666): Either disallow Payment Request in PiP entirely,
+  // or fix it so that this hack is not needed.
+  ASSERT_EQ(true, content::EvalJs(
+                      GetActiveWebContents(),
+                      "documentPictureInPicture.requestWindow().then(w => { "
+                      "w.document.open(); "
+                      "w.document.close(); "
+                      "w.history.replaceState(null, '', location.href); "
+                      "return true; })"));
+
+  content::WebContents* pip_web_contents =
+      PictureInPictureWindowManager::GetInstance()->GetChildWebContents();
+  ASSERT_NE(nullptr, pip_web_contents);
+
+  views::Widget* pip_widget = views::Widget::GetTopLevelWidgetForNativeView(
+      pip_web_contents->GetNativeView());
+  ASSERT_NE(nullptr, pip_widget);
+
+  // Size the PiP window to the minimum pre-widget size required by ShowDialog()
+  // (1.05 * kDialogMinWidth x kDialogHeight, with a 0 top offset). When the
+  // child dialog widget is shown, PipChildDialogObserverHelper synchronously
+  // enlarges the PiP window to fit the unscaled dialog plus title bar padding
+  // (without the 5% safety buffer), which then fails the post-widget size check
+  // in CheckIfDialogFitsInBrowserWindow().
+  const int width = base::ClampRound(kDialogMinWidth * 1.05f);
+  const int height = base::ClampRound(kDialogHeight * 1.05f);
+  views::AsyncWidgetRequestWaiter bounds_waiter(*pip_widget);
+  pip_widget->SetBounds(gfx::Rect(0, 0, width, height));
+  bounds_waiter.Wait();
+
+  base::HistogramTester histograms;
+
+  ResetEventWaiterForSequence(
+      {DialogEvent::PROCESSING_SPINNER_SHOWN,
+       DialogEvent::DIALOG_SIZE_CHECK_AFTER_BROWSER_RESIZE,
+       DialogEvent::INTERNAL_ERROR});
+
+  EXPECT_EQ(
+      "NotSupportedError: Browser window is too small for Payment Request UI.",
+      content::EvalJs(
+          pip_web_contents,
+          content::JsReplace(
+              "const request = new PaymentRequest([{supportedMethods: $1}], "
+              "{total: {label: 'Total', amount: {currency: 'USD', value: "
+              "'1.00'}}}); "
+              "request.show().catch(e => e.toString());",
+              method_name)));
+
+  ASSERT_TRUE(WaitForObservedEvent());
+
+  histograms.ExpectUniqueSample(
+      "PaymentRequest.WindowSizeCheckRejectionReason",
+      JourneyLogger::WindowSizeCheckRejectionReason::kRejectedAtResize, 1);
+}
+
 }  // namespace payments