From f96629e877623801b3dcb24453aee1ce15032465 Mon Sep 17 00:00:00 2001
From: Dan Murphy <[email protected]>
Date: Wed, 16 Sep 2026 10:55:23 -0700
Subject: [PATCH] [JavaScript Dialogs] Fix Invalidate() potential UAF

When AppModalDialogManager::CancelDialogs() invalidates a queued dialog
(such as a beforeunload dialog), its DialogClosedCallback can
synchronously resume deferred operations (e.g. prerender activation) and
re-entrantly invoke CancelDialogs().

Closing the active dialog triggers ShowNextDialog() -> GetNextDialog(),
which drains and destroys all invalidated queued controllers from
AppModalDialogQueue, including the controller whose Invalidate() is
currently executing on the call stack. Accessing view_ after
CallDialogClosedCallback() then results in a potential use-after-free.
In addition, iterating AppModalDialogQueue directly in CancelDialogs()
causes circular_deque iterator invalidation when re-entrancy mutates the
queue.

Similarly, AppModalDialogQueue::InvalidateAndClearQueuedDialogs() popped
dialogs one by one, allowing re-entrant ShowNextDialog() calls to drain
and destroy queued controllers while the loop was still walking them. In
NotifyDelegate(), valid_ was set to false at the bottom of the method,
leaving a window where synchronous callbacks could re-enter
NotifyDelegate() or Invalidate().

Fix this by:

1. Adding WeakPtrFactory to AppModalDialogController, checking weak_this
   after CallDialogClosedCallback(), CompleteDialog(), and
   ShowModalDialog() before accessing member variables, and invalidating
   weak pointers at the start of ~AppModalDialogController().
2. Marking valid_ = false immediately at the top of NotifyDelegate() to
   close the re-entrancy window.
3. Snapshotting WeakPtrs into a local vector in CancelDialogs() before
   calling Invalidate(), and re-fetching the active dialog afterwards.
4. Moving queued dialogs into a local container before invalidating in
   AppModalDialogQueue::InvalidateAndClearQueuedDialogs(), with an outer
   while loop to catch dialogs enqueued during invalidation.
5. Adding regression tests in app_modal_dialog_queue_unittest.cc
   covering both CancelDialogs() and CancelAllDialogs() re-entrancy.

TAG=agy
CONV=b95c3950-7b1c-4e87-bf3c-e8b6423aeef8

Fixed: 560439699
Test: components_unittests --gtest_filter="AppModalDialogQueueTest.*"
Change-Id: Ic910991545fbb878b60fb3a0b5d8f48d1f98cfd0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8414918
Commit-Queue: Daniel Murphy <[email protected]>
Reviewed-by: Avi Drissman <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1699065}
---
diff --git a/components/javascript_dialogs/app_modal_dialog_controller.cc b/components/javascript_dialogs/app_modal_dialog_controller.cc
index 805778f..70b64deb 100644
--- a/components/javascript_dialogs/app_modal_dialog_controller.cc
+++ b/components/javascript_dialogs/app_modal_dialog_controller.cc
@@ -88,16 +88,25 @@
       callback_(std::move(callback)) {}
 
 AppModalDialogController::~AppModalDialogController() {
+  // `CompleteDialog()` below can synchronously run more dialog code (it shows
+  // the next dialog in the queue). Invalidate weak pointers first so that any
+  // such code, as well as any stack frame holding a weak pointer to `this`,
+  // sees this dialog as already gone instead of as a half-destroyed object.
+  weak_ptr_factory_.InvalidateWeakPtrs();
   CompleteDialog();
 }
 
 void AppModalDialogController::ShowModalDialog(
     std::unique_ptr<AppModalDialogController> controller) {
   CHECK_EQ(this, controller.get());
+  // Showing the dialog can synchronously close it again, which destroys the
+  // view and with it `this`.
+  base::WeakPtr<AppModalDialogController> weak_this =
+      weak_ptr_factory_.GetWeakPtr();
   view_ = AppModalDialogManager::GetInstance()->view_factory()->Run(
       std::move(controller));
   view_->ShowAppModalDialog();
-  if (app_modal_dialog_observer) {
+  if (weak_this && app_modal_dialog_observer) {
     app_modal_dialog_observer->Notify(this);
   }
 }
@@ -133,7 +142,20 @@
   }
 
   valid_ = false;
+
+  // The closed callback can synchronously re-enter the dialog machinery: for
+  // example, closing a beforeunload dialog can resume a deferred prerender
+  // activation, which cancels dialogs again. That eventually reaches
+  // `AppModalDialogQueue::GetNextDialog()`, which destroys every invalidated
+  // queued dialog - including `this`, whose `Invalidate()` is still on the
+  // stack. See https://crbug.com/560439699.
+  base::WeakPtr<AppModalDialogController> weak_this =
+      weak_ptr_factory_.GetWeakPtr();
   CallDialogClosedCallback(false, std::u16string());
+  if (!weak_this) {
+    return;
+  }
+
   if (view_) {
     CloseModalDialog();
   }
@@ -146,14 +168,28 @@
   // that were still open in the ModalDialogQueue, which would send activation
   // back to this one. The framework should be improved to handle this, so this
   // is a temporary workaround.
+  //
+  // `CompleteDialog()` shows the next dialog in the queue, which can run
+  // arbitrary code that destroys `this`.
+  base::WeakPtr<AppModalDialogController> weak_this =
+      weak_ptr_factory_.GetWeakPtr();
   CompleteDialog();
+  if (!weak_this) {
+    return;
+  }
 
   NotifyDelegate(/*success=*/false, std::u16string(), suppress_js_messages);
 }
 
 void AppModalDialogController::OnAccept(const std::u16string& prompt_text,
                                         bool suppress_js_messages) {
+  // See the comment in `OnCancel()`: `CompleteDialog()` can destroy `this`.
+  base::WeakPtr<AppModalDialogController> weak_this =
+      weak_ptr_factory_.GetWeakPtr();
   CompleteDialog();
+  if (!weak_this) {
+    return;
+  }
 
   NotifyDelegate(/*success=*/true, override_prompt_text_.value_or(prompt_text),
                  suppress_js_messages);
@@ -179,7 +215,20 @@
     return;
   }
 
+  // Mark invalid immediately so that any re-entrant calls (such as OnClose()
+  // from Views during window destruction, or Invalidate() from navigation)
+  // return early. On Views, we can also end up coming through this code path a
+  // second time asynchronously (see https://crbug.com/40085084).
+  valid_ = false;
+
+  // The close callback can destroy `this`, e.g. by closing the dialog and
+  // letting the dialog queue delete it. See https://crbug.com/560439699.
+  base::WeakPtr<AppModalDialogController> weak_this =
+      weak_ptr_factory_.GetWeakPtr();
   CallDialogClosedCallback(success, user_input);
+  if (!weak_this) {
+    return;
+  }
 
   // The close callback above may delete web_contents_, thus removing the extra
   // data from the map owned by ::AppModalDialogManager. Make sure
@@ -191,10 +240,6 @@
       extra_data->second.suppress_javascript_messages_ = suppress_js_messages;
     }
   }
-
-  // On Views, we can end up coming through this code path twice :(.
-  // See https://crbug.com/40085084.
-  valid_ = false;
 }
 
 void AppModalDialogController::CallDialogClosedCallback(
diff --git a/components/javascript_dialogs/app_modal_dialog_controller.h b/components/javascript_dialogs/app_modal_dialog_controller.h
index e907a2bb..b83c325 100644
--- a/components/javascript_dialogs/app_modal_dialog_controller.h
+++ b/components/javascript_dialogs/app_modal_dialog_controller.h
@@ -11,6 +11,7 @@
 #include "base/compiler_specific.h"
 #include "base/functional/callback.h"
 #include "base/memory/raw_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/javascript_dialog_manager.h"
 #include "content/public/browser/web_contents_observer.h"
 
@@ -72,6 +73,11 @@
 
   // Invalidates the dialog, therefore causing it to not be shown when its turn
   // to be shown comes around.
+  //
+  // WARNING: This runs the dialog's closed callback, which can synchronously
+  // re-enter the dialog queue and destroy `this` (see the implementation).
+  // Callers must not touch this object after calling this method unless they
+  // hold a weak pointer to it, see `GetWeakPtr()`.
   void Invalidate();
 
   // Callbacks from NativeDialog when the user accepts or cancels the dialog.
@@ -98,6 +104,12 @@
   bool is_before_unload_dialog() const { return is_before_unload_dialog_; }
   bool is_reload() const { return is_reload_; }
 
+  // Weak pointers are invalidated at the start of the destructor, before
+  // `CompleteDialog()` runs.
+  base::WeakPtr<AppModalDialogController> GetWeakPtr() {
+    return weak_ptr_factory_.GetWeakPtr();
+  }
+
   // content::WebContentsObserver overrides:
   void WebContentsDestroyed() final;
 
@@ -143,6 +155,8 @@
   // Used only for testing. Specifies alternative prompt text that should be
   // used when notifying the delegate.
   std::optional<std::u16string> override_prompt_text_;
+
+  base::WeakPtrFactory<AppModalDialogController> weak_ptr_factory_{this};
 };
 
 // An interface to observe that a modal dialog is shown.
diff --git a/components/javascript_dialogs/app_modal_dialog_manager.cc b/components/javascript_dialogs/app_modal_dialog_manager.cc
index 65763a2..6a809675 100644
--- a/components/javascript_dialogs/app_modal_dialog_manager.cc
+++ b/components/javascript_dialogs/app_modal_dialog_manager.cc
@@ -7,8 +7,10 @@
 #include <algorithm>
 #include <memory>
 #include <utility>
+#include <vector>
 
 #include "base/functional/bind.h"
+#include "base/memory/weak_ptr.h"
 #include "base/no_destructor.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
@@ -233,16 +235,40 @@
 void AppModalDialogManager::CancelDialogs(content::WebContents* web_contents,
                                           bool reset_state) {
   AppModalDialogQueue* queue = AppModalDialogQueue::GetInstance();
+
+  // `AppModalDialogController::Invalidate()` runs the dialog's closed callback,
+  // which can synchronously re-enter this method and the dialog queue. That
+  // re-entrancy can both mutate `queue` (invalidating the iterators used here)
+  // and destroy the queued dialogs, since
+  // `AppModalDialogQueue::GetNextDialog()` deletes invalidated dialogs as it
+  // drains the queue. So collect weak pointers to the dialogs to cancel up
+  // front, and re-check each one before using it.
+  // See https://crbug.com/560439699.
+  std::vector<base::WeakPtr<AppModalDialogController>> dialogs_to_cancel;
   for (auto& dialog : *queue) {
     if (dialog->web_contents() == web_contents) {
+      dialogs_to_cancel.push_back(dialog->GetWeakPtr());
+    }
+  }
+  for (base::WeakPtr<AppModalDialogController>& dialog : dialogs_to_cancel) {
+    // `dialog` is null if it was already destroyed by the re-entrant cancelling
+    // triggered by an earlier dialog in this loop.
+    if (dialog) {
       dialog->Invalidate();
     }
   }
+
+  // The active dialog is deliberately looked up after the loop above: closing
+  // the queued dialogs can have shown a different dialog in the meantime.
+  // `Invalidate()` may destroy `active_dialog` (it closes its view, which owns
+  // it), so nothing may be done with the pointer afterwards.
   AppModalDialogController* active_dialog = queue->active_dialog();
   if (active_dialog && active_dialog->web_contents() == web_contents) {
     active_dialog->Invalidate();
   }
 
+  // Note: no weak pointer is needed for `this`: `AppModalDialogManager` is a
+  // `base::NoDestructor` singleton, so it outlives any re-entrant call above.
   if (reset_state) {
     javascript_dialog_extra_data_.erase(web_contents);
   }
diff --git a/components/javascript_dialogs/app_modal_dialog_queue.cc b/components/javascript_dialogs/app_modal_dialog_queue.cc
index 96b6cc8..23e8171 100644
--- a/components/javascript_dialogs/app_modal_dialog_queue.cc
+++ b/components/javascript_dialogs/app_modal_dialog_queue.cc
@@ -4,6 +4,9 @@
 
 #include "components/javascript_dialogs/app_modal_dialog_queue.h"
 
+#include <memory>
+#include <utility>
+
 #include "base/no_destructor.h"
 #include "components/javascript_dialogs/app_modal_dialog_controller.h"
 
@@ -27,9 +30,25 @@
 }
 
 void AppModalDialogQueue::InvalidateAndClearQueuedDialogs() {
+  // `AppModalDialogController::Invalidate()` runs the dialog's closed callback,
+  // which can synchronously re-enter this queue: it can come back into this
+  // method (e.g. via `ShowNextDialog()` during shutdown), and it can enqueue
+  // new dialogs. Take ownership of the queued dialogs before invalidating any
+  // of them, so that a re-entrant call sees an empty queue and cannot pop and
+  // destroy a dialog whose `Invalidate()` is still on the stack.
+  // See https://crbug.com/560439699.
+  //
+  // The outer loop handles dialogs that were enqueued by the re-entrant calls
+  // made while invalidating; those dialogs must be invalidated as well.
   while (!app_modal_dialog_queue_.empty()) {
-    app_modal_dialog_queue_.front()->Invalidate();
-    app_modal_dialog_queue_.pop_front();
+    DialogQueue dialogs = std::move(app_modal_dialog_queue_);
+    // The state of a moved-from container is unspecified, so explicitly empty
+    // the member queue for any re-entrant caller.
+    app_modal_dialog_queue_.clear();
+    for (std::unique_ptr<AppModalDialogController>& dialog : dialogs) {
+      dialog->Invalidate();
+    }
+    // `dialogs` goes out of scope here, destroying the invalidated dialogs.
   }
 }
 
diff --git a/components/javascript_dialogs/app_modal_dialog_queue_unittest.cc b/components/javascript_dialogs/app_modal_dialog_queue_unittest.cc
index 31b1e70..1efde0e 100644
--- a/components/javascript_dialogs/app_modal_dialog_queue_unittest.cc
+++ b/components/javascript_dialogs/app_modal_dialog_queue_unittest.cc
@@ -5,9 +5,12 @@
 #include "components/javascript_dialogs/app_modal_dialog_queue.h"
 
 #include <memory>
+#include <utility>
 
+#include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
 #include "components/javascript_dialogs/app_modal_dialog_controller.h"
+#include "components/javascript_dialogs/app_modal_dialog_manager.h"
 #include "components/javascript_dialogs/app_modal_dialog_view.h"
 #include "content/public/browser/javascript_dialog_manager.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -24,7 +27,14 @@
 
   void ShowAppModalDialog() override {}
   void ActivateAppModalDialog() override {}
-  void CloseAppModalDialog() override {}
+  // Real views notify their controller that the dialog was dismissed when they
+  // are closed. Note that, unlike the real views, this one keeps `controller_`
+  // alive so that tests control its lifetime.
+  void CloseAppModalDialog() override {
+    if (controller_) {
+      controller_->OnCancel(/*suppress_js_messages=*/false);
+    }
+  }
   void AcceptAppModalDialog() override {}
   void CancelAppModalDialog() override {}
   bool IsShowing() const override { return true; }
@@ -35,7 +45,10 @@
 
 class TestDialogControllerBase : public AppModalDialogController {
  public:
-  explicit TestDialogControllerBase(bool* was_destroyed)
+  explicit TestDialogControllerBase(
+      bool* was_destroyed,
+      content::JavaScriptDialogManager::DialogClosedCallback callback =
+          base::DoNothing())
       : AppModalDialogController(nullptr,
                                  &extra_data_,
                                  u"test",
@@ -45,7 +58,7 @@
                                  false,
                                  false,
                                  false,
-                                 base::DoNothing()),
+                                 std::move(callback)),
         was_destroyed_(was_destroyed) {}
 
   ~TestDialogControllerBase() override {
@@ -179,5 +192,92 @@
   dialog1_ptr->Invalidate();
 }
 
+// Returns a DialogClosedCallback that re-entrantly cancels all dialogs for the
+// test WebContents (nullptr), and records that it ran.
+content::JavaScriptDialogManager::DialogClosedCallback
+MakeReentrantCancelCallback(bool* callback_ran) {
+  return base::BindOnce(
+      [](bool* callback_ran, bool /*success*/,
+         const std::u16string& /*user_input*/) {
+        *callback_ran = true;
+        // This invalidates the active dialog, whose view then dispatches
+        // OnCancel -> CompleteDialog -> ShowNextDialog -> GetNextDialog, which
+        // drains and destroys the invalidated dialogs still in the queue - one
+        // of which is the dialog whose Invalidate() is running this callback.
+        AppModalDialogManager::GetInstance()->CancelDialogs(
+            /*web_contents=*/nullptr, /*reset_state=*/false);
+      },
+      callback_ran);
+}
+
+// Regression test for https://crbug.com/560439699: a queued dialog's closed
+// callback re-entrantly cancels dialogs, which destroys the queued dialogs
+// while AppModalDialogController::Invalidate() and the CancelDialogs() loop
+// iterating the queue are still on the stack.
+TEST_F(AppModalDialogQueueTest, ReentrantCancelDialogsDuringInvalidate) {
+  auto* queue = AppModalDialogQueue::GetInstance();
+  auto* manager = AppModalDialogManager::GetInstance();
+
+  bool dialog1_shown = false;
+  bool dialog1_destroyed = false;
+  bool dialog2_destroyed = false;
+  bool dialog3_destroyed = false;
+  bool callback_ran = false;
+  std::unique_ptr<FakeView> dialog1_view;
... (file diff truncated)

diff --git a/components/javascript_dialogs/app_modal_dialog_queue_unittest.cc b/components/javascript_dialogs/app_modal_dialog_queue_unittest.cc
index 31b1e70..1efde0e 100644
--- a/components/javascript_dialogs/app_modal_dialog_queue_unittest.cc
+++ b/components/javascript_dialogs/app_modal_dialog_queue_unittest.cc
@@ -5,9 +5,12 @@
 #include "components/javascript_dialogs/app_modal_dialog_queue.h"
 
 #include <memory>
+#include <utility>
 
+#include "base/functional/bind.h"
 #include "base/functional/callback_helpers.h"
 #include "components/javascript_dialogs/app_modal_dialog_controller.h"
+#include "components/javascript_dialogs/app_modal_dialog_manager.h"
 #include "components/javascript_dialogs/app_modal_dialog_view.h"
 #include "content/public/browser/javascript_dialog_manager.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -24,7 +27,14 @@
 
   void ShowAppModalDialog() override {}
   void ActivateAppModalDialog() override {}
-  void CloseAppModalDialog() override {}
+  // Real views notify their controller that the dialog was dismissed when they
+  // are closed. Note that, unlike the real views, this one keeps `controller_`
+  // alive so that tests control its lifetime.
+  void CloseAppModalDialog() override {
+    if (controller_) {
+      controller_->OnCancel(/*suppress_js_messages=*/false);
+    }
+  }
   void AcceptAppModalDialog() override {}
   void CancelAppModalDialog() override {}
   bool IsShowing() const override { return true; }
@@ -35,7 +45,10 @@
 
 class TestDialogControllerBase : public AppModalDialogController {
  public:
-  explicit TestDialogControllerBase(bool* was_destroyed)
+  explicit TestDialogControllerBase(
+      bool* was_destroyed,
+      content::JavaScriptDialogManager::DialogClosedCallback callback =
+          base::DoNothing())
       : AppModalDialogController(nullptr,
                                  &extra_data_,
                                  u"test",
@@ -45,7 +58,7 @@
                                  false,
                                  false,
                                  false,
-                                 base::DoNothing()),
+                                 std::move(callback)),
         was_destroyed_(was_destroyed) {}
 
   ~TestDialogControllerBase() override {
@@ -179,5 +192,92 @@
   dialog1_ptr->Invalidate();
 }
 
+// Returns a DialogClosedCallback that re-entrantly cancels all dialogs for the
+// test WebContents (nullptr), and records that it ran.
+content::JavaScriptDialogManager::DialogClosedCallback
+MakeReentrantCancelCallback(bool* callback_ran) {
+  return base::BindOnce(
+      [](bool* callback_ran, bool /*success*/,
+         const std::u16string& /*user_input*/) {
+        *callback_ran = true;
+        // This invalidates the active dialog, whose view then dispatches
+        // OnCancel -> CompleteDialog -> ShowNextDialog -> GetNextDialog, which
+        // drains and destroys the invalidated dialogs still in the queue - one
+        // of which is the dialog whose Invalidate() is running this callback.
+        AppModalDialogManager::GetInstance()->CancelDialogs(
+            /*web_contents=*/nullptr, /*reset_state=*/false);
+      },
+      callback_ran);
+}
+
+// Regression test for https://crbug.com/560439699: a queued dialog's closed
+// callback re-entrantly cancels dialogs, which destroys the queued dialogs
+// while AppModalDialogController::Invalidate() and the CancelDialogs() loop
+// iterating the queue are still on the stack.
+TEST_F(AppModalDialogQueueTest, ReentrantCancelDialogsDuringInvalidate) {
+  auto* queue = AppModalDialogQueue::GetInstance();
+  auto* manager = AppModalDialogManager::GetInstance();
+
+  bool dialog1_shown = false;
+  bool dialog1_destroyed = false;
+  bool dialog2_destroyed = false;
+  bool dialog3_destroyed = false;
+  bool callback_ran = false;
+  std::unique_ptr<FakeView> dialog1_view;
+
+  queue->AddDialog(std::make_unique<TestDialogController>(
+      &dialog1_shown, &dialog1_destroyed, &dialog1_view));
+  ASSERT_TRUE(dialog1_shown);
+  ASSERT_TRUE(queue->HasActiveDialog());
+
+  queue->AddDialog(std::make_unique<TestDialogControllerBase>(
+      &dialog2_destroyed, MakeReentrantCancelCallback(&callback_ran)));
+  queue->AddDialog(
+      std::make_unique<TestDialogControllerBase>(&dialog3_destroyed));
+
+  manager->CancelDialogs(/*web_contents=*/nullptr, /*reset_state=*/false);
+
+  EXPECT_TRUE(callback_ran);
+  EXPECT_TRUE(dialog2_destroyed);
+  EXPECT_TRUE(dialog3_destroyed);
+  EXPECT_FALSE(queue->HasActiveDialog());
+  // The active dialog is owned by its view, so it outlives the cancellation.
+  EXPECT_FALSE(dialog1_destroyed);
+}
+
+// Regression test for https://crbug.com/560439699: the same re-entrancy, but
+// entered through AppModalDialogQueue::InvalidateAndClearQueuedDialogs(). The
+// re-entrant ShowNextDialog() must not drain (and destroy) the dialogs that the
+// outer invalidation loop is still walking over.
+TEST_F(AppModalDialogQueueTest, ReentrantCancelDuringCancelAllDialogs) {
+  auto* queue = AppModalDialogQueue::GetInstance();
+
+  bool dialog1_shown = false;
+  bool dialog1_destroyed = false;
+  bool dialog2_destroyed = false;
+  bool dialog3_destroyed = false;
+  bool callback_ran = false;
+  std::unique_ptr<FakeView> dialog1_view;
+
+  queue->AddDialog(std::make_unique<TestDialogController>(
+      &dialog1_shown, &dialog1_destroyed, &dialog1_view));
+  ASSERT_TRUE(dialog1_shown);
+  ASSERT_TRUE(queue->HasActiveDialog());
+
+  queue->AddDialog(std::make_unique<TestDialogControllerBase>(
+      &dialog2_destroyed, MakeReentrantCancelCallback(&callback_ran)));
+  queue->AddDialog(
+      std::make_unique<TestDialogControllerBase>(&dialog3_destroyed));
+
+  queue->CancelAllDialogs();
+
+  EXPECT_TRUE(callback_ran);
+  EXPECT_TRUE(dialog2_destroyed);
+  EXPECT_TRUE(dialog3_destroyed);
+  EXPECT_FALSE(queue->HasActiveDialog());
+  // The active dialog is owned by its view, so it outlives the cancellation.
+  EXPECT_FALSE(dialog1_destroyed);
+}
+
 }  // namespace
 }  // namespace javascript_dialogs