From 932e957b2cd9e2a172e779ef44e6aa6ab6442bee Mon Sep 17 00:00:00 2001
From: Mitsuru Oshima <oshima@chromium.org>
Date: Tue, 01 Sep 2026 23:23:04 -0700
Subject: [PATCH] views: Fix UAF in MenuController and simplify teardown

This CL fixes a Use-After-Free (UAF) in MenuController::OpenMenuImpl
by deferring MenuController and MenuRunnerImpl destruction when the
execution stack is active (tracked via stack_depth_ > 0).

To support this:
- Fix UAF in MenuItemView by clearing parent pointers during
  destruction.
- Fix gesture transfer issue in MenuHost by returning early in
  HideMenuHost if we are destroying and it is already hidden.
- Fix dangling pointers in ClipboardHistory and BirchBar menus by
  clearing delegates/history pointers during menu teardown.

Bug: 516996291
AX-Relnotes: n/a.
TAG=agy
CONV=b4c86621-d1cd-478c-ae64-25a090f21efa

Change-Id: I4d4d4cf14c95bab5745653f1d0d59845dc3b9024
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8246240
Reviewed-by: Allen Bauer <kylixrd@chromium.org>
Reviewed-by: Keren Zhu <kerenzhu@chromium.org>
Commit-Queue: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1690483}
---

diff --git a/ui/views/controls/menu/menu_controller.cc b/ui/views/controls/menu/menu_controller.cc
index 6a8ca32..c0ac1441 100644
--- a/ui/views/controls/menu/menu_controller.cc
+++ b/ui/views/controls/menu/menu_controller.cc
@@ -11,7 +11,6 @@
 #include "base/callback_list.h"
 #include "base/check.h"
 #include "base/containers/flat_set.h"
-#include "base/debug/dump_without_crashing.h"
 #include "base/functional/bind.h"
 #include "base/i18n/case_conversion.h"
 #include "base/i18n/rtl.h"
@@ -47,6 +46,7 @@
 #include "ui/views/controls/menu/menu_host_root_view.h"
 #include "ui/views/controls/menu/menu_item_view.h"
 #include "ui/views/controls/menu/menu_pre_target_handler.h"
+#include "ui/views/controls/menu/menu_runner_impl.h"
 #include "ui/views/controls/menu/menu_scroll_view_container.h"
 #include "ui/views/controls/menu/menu_types.h"
 #include "ui/views/controls/menu/submenu_view.h"
@@ -685,6 +685,9 @@
                          MenuType menu_type,
                          bool is_nested_drag,
                          gfx::NativeView native_view_for_gestures) {
+  // Track stack depth to defer controller destruction during initial menu
+  // display.
+  ScopedDeletionGuard guard(AsWeakPtr());
   exit_type_ = ExitType::kNone;
   possible_drag_ = false;
   drag_in_progress_ = false;
@@ -816,6 +819,9 @@
 }
 
 void MenuController::Cancel(ExitType type, bool disable_animation) {
+  // Track stack depth to defer controller destruction during menu
+  // cancellation.
+  ScopedDeletionGuard guard(AsWeakPtr());
 #if BUILDFLAG(IS_MAC)
   menu_closure_animation_.reset();
 #endif
@@ -918,12 +924,12 @@
 
 void MenuController::SelectItemAndOpenSubmenu(MenuItemView* item) {
   DCHECK(item);
-  auto this_ref = AsWeakPtr();
+  ScopedDeletionGuard guard(AsWeakPtr());
   SetSelection(item, SELECTION_OPEN_SUBMENU | SELECTION_UPDATE_IMMEDIATELY);
 
   // Accessibility events fired as a result of the selection changing may have
-  // closed the menu and deleted `this`. Guard against that.
-  if (!this_ref) {
+  // closed the menu and destroyed `this`. Guard against that.
+  if (destroy_pending_) {
     return;
   }
 
@@ -937,6 +943,7 @@
 
 bool MenuController::OnMousePressed(SubmenuView* source,
                                     const ui::MouseEvent& event) {
+  ScopedDeletionGuard guard(AsWeakPtr());
   // We should either have no current_mouse_event_target_, or should have a
   // pressed state stored.
   DCHECK(!current_mouse_event_target_ || current_mouse_pressed_state_);
@@ -968,12 +975,11 @@
     View* view =
         forward_to_root->GetEventHandlerForPoint(event_for_root.location());
     Button* button = Button::AsButton(view);
-    auto this_ref = AsWeakPtr();
     if (hot_button_ != button) {
       SetHotTrackedButton(button);
     }
 
-    if (!this_ref) {
+    if (destroy_pending_) {
       return true;
     }
 
@@ -982,7 +988,7 @@
       bool processed = forward_to_root->ProcessMousePressed(event_for_root);
       // This object may be destroyed as a result of a mouse press event (some
       // item may close the menu).
-      if (!this_ref) {
+      if (destroy_pending_) {
         return true;
       }
 
@@ -1006,6 +1012,7 @@
 
 bool MenuController::OnMouseDragged(SubmenuView* source,
                                     const ui::MouseEvent& event) {
+  ScopedDeletionGuard guard(AsWeakPtr());
   if (current_mouse_event_target_) {
     return current_mouse_event_target_->ProcessMouseDragged(
         ConvertLocatedEventForRootView(*source, *current_mouse_event_target_,
@@ -1025,9 +1032,6 @@
     }
     return true;
   }
-  // Changing the selection or showing a sibling menu can cause `this` to be
-  // deleted as a side effect of accessibility notifications.
-  auto this_ref = AsWeakPtr();
   MenuItemView* mouse_menu = nullptr;
   if (part.type == MenuPartType::kMenuItem) {
     // If there is no menu target, but a submenu target, then we are interacting
@@ -1055,7 +1059,9 @@
       }
     }
   }
-  if (!this_ref) {
+  // Changing the selection or showing a sibling menu can cause `this` to be
+  // destroyed as a side effect of accessibility notifications.
+  if (destroy_pending_) {
     return false;
   }
   UpdateActiveMouseView(source, event, mouse_menu);
@@ -1065,10 +1071,9 @@
 
 void MenuController::OnMouseReleased(SubmenuView* source,
                                      const ui::MouseEvent& event) {
+  ScopedDeletionGuard guard(AsWeakPtr());
   current_mouse_pressed_state_ &= ~event.changed_button_flags();
 
-  auto this_ref = AsWeakPtr();
-
   if (current_mouse_event_target_) {
     // If this was the final mouse button, then remove the forwarding target.
     // We need to do this *before* dispatching the event to the root view
@@ -1147,7 +1152,7 @@
     // On the rare, off chance that an accessibility event is fired as a result
     // of the selection changing *and* the Accessibility tool causes the menu to
     // be closed, `this` will be otherwise dangling. Guard against that.
-    if (!this_ref) {
+    if (destroy_pending_) {
       return;
     }
   }
@@ -1157,6 +1162,7 @@
 
 void MenuController::OnMouseMoved(SubmenuView* source,
                                   const ui::MouseEvent& event) {
+  ScopedDeletionGuard guard(AsWeakPtr());
   if (current_mouse_event_target_) {
     current_mouse_event_target_->ProcessMouseMoved(
         ConvertLocatedEventForRootView(*source, *current_mouse_event_target_,
@@ -1190,9 +1196,8 @@
 
   // `HandleMouseLocation()` may change the selection, which can cause `this` to
   // be deleted as a side effect of accessibility notifications.
-  auto this_ref = AsWeakPtr();
   HandleMouseLocation(source, event.location());
-  if (!this_ref) {
+  if (destroy_pending_) {
     return;
   }
 
@@ -1213,6 +1218,7 @@
 
 bool MenuController::OnMouseWheel(SubmenuView* source,
                                   const ui::MouseWheelEvent& event) {
+  ScopedDeletionGuard guard(AsWeakPtr());
   // Stop scrolling via scroll button to prevent flickering.
   StopScrollingViaButton();
 
@@ -1220,12 +1226,18 @@
 
   SetSelection(part.menu ? part.menu.get() : state_.item.get(),
                SELECTION_OPEN_SUBMENU | SELECTION_UPDATE_IMMEDIATELY);
+  if (destroy_pending_) {
+    return false;

diff --git a/ui/views/controls/menu/menu_controller_unittest.cc b/ui/views/controls/menu/menu_controller_unittest.cc
index 8c13124..b0db67a 100644
--- a/ui/views/controls/menu/menu_controller_unittest.cc
+++ b/ui/views/controls/menu/menu_controller_unittest.cc
@@ -1063,13 +1063,8 @@
     return;
   }
 
-  if (!owner_->IsClosed()) {
-    owner_->RemoveObserver(menu_controller_);
-  }
-
-  menu_controller_->showing_ = false;
-  menu_controller_->owner_ = nullptr;
-  delete menu_controller_.ExtractAsDangling();
+  menu_controller_->ClearOwner();
+  menu_controller_.ExtractAsDangling()->Destroy();
 }
 
 // static
diff --git a/ui/views/controls/menu/menu_runner_unittest.cc b/ui/views/controls/menu/menu_runner_unittest.cc
index cde50741..9ba9dd83 100644
--- a/ui/views/controls/menu/menu_runner_unittest.cc
+++ b/ui/views/controls/menu/menu_runner_unittest.cc
@@ -13,6 +13,7 @@
 #include "base/memory/raw_ptr.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind.h"
+#include "base/test/gtest_util.h"
 #include "base/test/metrics/histogram_tester.h"
 #include "base/test/run_until.h"
 #include "base/test/simple_test_tick_clock.h"
@@ -37,6 +38,7 @@
 #include "ui/views/test/menu_test_utils.h"
 #include "ui/views/test/test_views.h"
 #include "ui/views/test/views_test_base.h"
+#include "ui/views/test/widget_test.h"
 #include "ui/views/widget/any_widget_observer.h"
 #include "ui/views/widget/native_widget_private.h"
 #include "ui/views/widget/widget.h"
@@ -680,6 +682,32 @@
   ~MenuRunnerImplTest() override = default;
 };
 
+// Tests that when nested menu runners are destroyed out of order, that
+// MenuController is not accessed after it has been destroyed. This should not
+// crash on ASAN bots.
+TEST_F(MenuRunnerImplTest, NestedMenuRunnersDestroyedOutOfOrder) {
+  internal::MenuRunnerImpl* menu_runner =
+      new internal::MenuRunnerImpl(CreateMenuItemView());
+  menu_runner->RunMenuAt(owner(), nullptr, gfx::Rect(),
+                         MenuAnchorPosition::kTopLeft);
+
+  auto menu_delegate2 = std::make_unique<TestMenuDelegate>();
+  auto menu_item_view2 = std::make_unique<MenuItemView>(menu_delegate2.get());
+  menu_item_view2->AppendMenuItem(1, u"One");
+
+  internal::MenuRunnerImpl* menu_runner2 =
+      new internal::MenuRunnerImpl(std::move(menu_item_view2));
+  menu_runner2->RunMenuAt(
+      owner(), nullptr, gfx::Rect(), MenuAnchorPosition::kTopLeft,
+      ui::mojom::MenuSourceType::kNone, MenuRunner::IS_NESTED);
+
+  menu_runner->Cancel();
+
+  // This should not access the destroyed MenuController
+  menu_runner2->Release();
+  ResetMenuItemView();
+  menu_runner->Release();
+}
 
 // Regression test demonstrating that the host_-exists branch of
 // SubmenuView::ShowAt lacks a WeakPtr liveness guard after ShowMenuHost.
@@ -752,6 +780,351 @@
   EXPECT_TRUE(fired);
 }
 
+// Regression test demonstrating that MenuItemView destruction during ShowAt
+// is deferred and completes safely without crash.
+TEST_F(MenuRunnerImplTest, NestedRunnerReleasedDuringShowAtSafe) {
+  // Build a root menu containing one submenu item.
+  auto root = std::make_unique<TestMenuItemView>(menu_delegate());
+  MenuItemView* sub_item = root->AppendSubMenu(100, u"Sub");
+  sub_item->AppendMenuItem(101, u"Leaf");
+
+  internal::MenuRunnerImpl* menu_runner =
+      new internal::MenuRunnerImpl(std::move(root));
+  menu_runner->RunMenuAt(owner(), nullptr, gfx::Rect(gfx::Size(200, 200)),
+                         MenuAnchorPosition::kTopLeft);
+
+  MenuController* controller = MenuController::GetForOwnerWidget(owner());
+  ASSERT_TRUE(controller);
+
+  // We need to create a nested runner to simulate the nested menu.
+  auto nested_root = std::make_unique<TestMenuItemView>(menu_delegate());
+  MenuItemView* nested_sub_item = nested_root->AppendSubMenu(200, u"NestedSub");
+  nested_sub_item->AppendMenuItem(201, u"NestedLeaf");
+
+  internal::MenuRunnerImpl* nested_runner =
+      new internal::MenuRunnerImpl(std::move(nested_root));
+  nested_runner->RunMenuAt(owner(), nullptr, gfx::Rect(gfx::Size(200, 200)),
+                           MenuAnchorPosition::kTopLeft,
+                           ui::mojom::MenuSourceType::kNone,
+                           MenuRunner::IS_NESTED);
+
+  bool fired = false;
+  base::WeakPtr<Widget> host_widget;
+  AnyWidgetObserver observer(views::test::AnyWidgetTestPasskey{});
+  observer.set_shown_callback(
+      base::BindLambdaForTesting([&](views::Widget* widget) {
+        if (fired || widget->GetName() != "MenuHost") {
+          return;
+        }
+        fired = true;
+        host_widget = widget->GetWeakPtr();
+        // Release the runner. This should NOT crash, but defer deletion.
+        nested_runner->Release();
+
+        // The widget should still be alive because deletion is deferred.
+        ASSERT_TRUE(host_widget);
+      }));
+
+  nested_sub_item->GetMenuController()->SelectItemAndOpenSubmenu(
+      nested_sub_item);
+
+  EXPECT_TRUE(fired);
+
+  // After ShowSubmenuImmediately returns, the stack depth returns to 0.
+  // But since we didn't cancel the menu, the widget should still be alive.
+  EXPECT_TRUE(host_widget);
+
+  // Now cancel the menu. This should trigger OnMenuClosed, which will
+  // see delete_after_run_ is true, and delete the runner.
+  controller->Cancel(MenuController::ExitType::kAll);
+
+  // Wait for the runner to be deleted and destroy the widget.
+  if (host_widget) {
+    views::test::WidgetDestroyedWaiter(host_widget.get()).Wait();
+  }
+  EXPECT_FALSE(host_widget);
+
+  // Clean up parent.
+  menu_runner->Release();
+}
+
+// Regression test demonstrating that MenuController destruction during ShowAt
+// is blocked and results in a CHECK failure.
+using MenuRunnerImplDeathTest = MenuRunnerImplTest;
+TEST_F(MenuRunnerImplDeathTest, MenuControllerDeletedDuringShowAtCHECK) {
+  // Build a root menu containing one submenu item.
+  auto root = std::make_unique<TestMenuItemView>(menu_delegate());
+  MenuItemView* root_ptr = root.get();
+  MenuItemView* sub_item = root->AppendSubMenu(100, u"Sub");
+  sub_item->AppendMenuItem(101, u"Leaf");
+
+  internal::MenuRunnerImpl* menu_runner =
+      new internal::MenuRunnerImpl(std::move(root));
+  menu_runner->RunMenuAt(owner(), nullptr, gfx::Rect(gfx::Size(200, 200)),
+                         MenuAnchorPosition::kTopLeft);
+
+  MenuController* controller = MenuController::GetForOwnerWidget(owner());
+  ASSERT_TRUE(controller);
+  Widget* root_widget = root_ptr->GetSubmenu()->GetWidget();
+  ASSERT_TRUE(root_widget);
+
+  // We need to create a nested runner to simulate the nested menu.
+  auto nested_root = std::make_unique<TestMenuItemView>(menu_delegate());
+  MenuItemView* nested_root_ptr = nested_root.get();
+  MenuItemView* nested_sub_item = nested_root->AppendSubMenu(200, u"NestedSub");
+  nested_sub_item->AppendMenuItem(201, u"NestedLeaf");
+
+  internal::MenuRunnerImpl* nested_runner =
+      new internal::MenuRunnerImpl(std::move(nested_root));
+  nested_runner->RunMenuAt(owner(), nullptr, gfx::Rect(gfx::Size(200, 200)),
+                           MenuAnchorPosition::kTopLeft,
+                           ui::mojom::MenuSourceType::kNone,
+                           MenuRunner::IS_NESTED);
+  Widget* nested_widget = nested_root_ptr->GetSubmenu()->GetWidget();
+  ASSERT_TRUE(nested_widget);
+
+  // We expect the deletion to crash the process.
+  EXPECT_DEATH(
+      {
+        AnyWidgetObserver observer(views::test::AnyWidgetTestPasskey{});
+        bool fired = false;
+        observer.set_shown_callback(
+            base::BindLambdaForTesting([&](views::Widget* widget) {
+              if (fired || widget->GetName() != "MenuHost") {
+                return;
+              }
+              fired = true;
+              // Fake scenario to delete the instance while blocking the
+              // deletion.
+              MenuController::DeleteForTesting(
+                  MenuController::GetActiveInstance());
+            }));
+        MenuController::GetActiveInstance()->SelectItemAndOpenSubmenu(
+            nested_sub_item);
+      },
+      "Check failed: stack_depth_ == 0");
+
+  // Clean up in the parent process.
+  base::WeakPtr<Widget> root_weak = root_widget->GetWeakPtr();
+  base::WeakPtr<Widget> nested_weak = nested_widget->GetWeakPtr();
+
+  nested_runner->Release();
+  menu_runner->Release();
+
+  if (nested_weak) {
+    views::test::WidgetDestroyedWaiter(nested_weak.get()).Wait();
+  }
+  if (root_weak) {
+    views::test::WidgetDestroyedWaiter(root_weak.get()).Wait();
+  }
+}
+
+// Regression test demonstrating that MenuHost destruction during ShowAt
+// is deferred and completed after ShowAt returns.
+TEST_F(MenuRunnerImplTest, MenuHostDeferredDuringShowAt) {
+  // Build a root menu containing one submenu item.
+  auto root = std::make_unique<TestMenuItemView>(menu_delegate());
+  MenuItemView* sub_item = root->AppendSubMenu(100, u"Sub");
+  sub_item->AppendMenuItem(101, u"Leaf");
+
+  internal::MenuRunnerImpl* menu_runner =
+      new internal::MenuRunnerImpl(std::move(root));
+  menu_runner->RunMenuAt(owner(), nullptr, gfx::Rect(gfx::Size(200, 200)),
+                         MenuAnchorPosition::kTopLeft);
+
+  MenuController* controller = MenuController::GetForOwnerWidget(owner());
+  ASSERT_TRUE(controller);
+
+  // We need to create a nested runner to simulate the nested menu.
+  auto nested_root = std::make_unique<TestMenuItemView>(menu_delegate());
+  MenuItemView* nested_sub_item = nested_root->AppendSubMenu(200, u"NestedSub");
+  nested_sub_item->AppendMenuItem(201, u"NestedLeaf");
+
+  internal::MenuRunnerImpl* nested_runner =
+      new internal::MenuRunnerImpl(std::move(nested_root));
+  nested_runner->RunMenuAt(owner(), nullptr, gfx::Rect(gfx::Size(200, 200)),
+                           MenuAnchorPosition::kTopLeft,
+                           ui::mojom::MenuSourceType::kNone,
+                           MenuRunner::IS_NESTED);
+
+  bool fired = false;
+  base::WeakPtr<Widget> host_widget;
+  AnyWidgetObserver observer(views::test::AnyWidgetTestPasskey{});
+  observer.set_shown_callback(
+      base::BindLambdaForTesting([&](views::Widget* widget) {
+        if (fired || widget->GetName() != "MenuHost") {
+          return;
+        }
+        fired = true;
+        host_widget = widget->GetWeakPtr();
+
+        // Cancel the menu during ShowAt. This calls DestroyMenuHost on
+        // MenuHost. Since we are in ShowAt, the destruction should be deferred.
+        controller->Cancel(MenuController::ExitType::kAll);
+
+        // The widget should still be alive (destruction deferred).
+        ASSERT_TRUE(host_widget);
+        EXPECT_TRUE(static_cast<MenuHost*>(host_widget.get())
+                        ->destroying_for_testing());
+      }));
+
+  nested_sub_item->GetMenuController()->SelectItemAndOpenSubmenu(
+      nested_sub_item);
+
+  EXPECT_TRUE(fired);
+  // Wait for the deferred Widget::Close() to complete.
+  if (host_widget) {
+    views::test::WidgetDestroyedWaiter(host_widget.get()).Wait();
+  }
+  // After ShowSubmenuImmediately returns and waiter finishes, the deferred
+  // close should have run, destroying the widget.
+  EXPECT_FALSE(host_widget);
+
+  // Clean up
+  nested_runner->Release();
+  menu_runner->Release();
+}
+
+// Regression test demonstrating that MenuController destruction during ShowAt
+// is deferred and completed after ShowAt returns.
+TEST_F(MenuRunnerImplTest, MenuControllerDeferredDestructionDuringShowAt) {
+  // Build a root menu containing one submenu item.
+  auto root = std::make_unique<TestMenuItemView>(menu_delegate());
+  MenuItemView* sub_item = root->AppendSubMenu(100, u"Sub");
+  sub_item->AppendMenuItem(101, u"Leaf");
+
+  internal::MenuRunnerImpl* menu_runner =
+      new internal::MenuRunnerImpl(std::move(root));
+  menu_runner->RunMenuAt(owner(), nullptr, gfx::Rect(gfx::Size(200, 200)),
+                         MenuAnchorPosition::kTopLeft);
+
+  MenuController* controller = MenuController::GetForOwnerWidget(owner());
+  ASSERT_TRUE(controller);
+  base::WeakPtr<MenuController> controller_weak = controller->AsWeakPtr();
+
+  // We need to create a nested runner to simulate the nested menu.
... (truncated)