Reland Fix MenuController Heap-use-after-free

ui/views/controls/menu/menu_controller_unittest.cc

Index: ui/views/controls/menu/menu_controller_unittest.cc
diff --git a/ui/views/controls/menu/menu_controller_unittest.cc b/ui/views/controls/menu/menu_controller_unittest.cc
index 804313f33866f67399fb207302ab963a515b39eb..70aded93ba9e3e141f363e8160e635bdf72062be 100644
--- a/ui/views/controls/menu/menu_controller_unittest.cc
+++ b/ui/views/controls/menu/menu_controller_unittest.cc
@@ -29,6 +29,7 @@
 #include "ui/views/controls/menu/menu_scroll_view_container.h"
 #include "ui/views/controls/menu/submenu_view.h"
 #include "ui/views/test/menu_test_utils.h"
+#include "ui/views/test/test_views_delegate.h"
 #include "ui/views/test/views_test_base.h"
 
 #if defined(USE_AURA)
@@ -237,6 +238,31 @@ bool TestDragDropClient::IsDragDropInProgress() {
 
 #endif  // defined(USE_AURA)
 
+// Test implementation of TestViewsDelegate which overrides ReleaseRef in order
+// to test destruction order. This simulates Chrome shutting down upon the
+// release of the ref. Associated tests should not crash.
+class DestructingTestViewsDelegate : public TestViewsDelegate {
+ public:
+  DestructingTestViewsDelegate() {}
+  ~DestructingTestViewsDelegate() override {}
+
+  void set_release_ref_callback(const base::Closure& release_ref_callback) {
+    release_ref_callback_ = release_ref_callback;
+  }
+
+  // TestViewsDelegate:
+  void ReleaseRef() override;
+
+ private:
+  base::Closure release_ref_callback_;
+  DISALLOW_COPY_AND_ASSIGN(DestructingTestViewsDelegate);
+};
+
+void DestructingTestViewsDelegate::ReleaseRef() {
+  if (!release_ref_callback_.is_null())
+    release_ref_callback_.Run();
+}
+
 }  // namespace
 
 class TestMenuItemViewShown : public MenuItemView {
@@ -263,6 +289,11 @@ class MenuControllerTest : public ViewsTestBase {
 
   // ViewsTestBase:
   void SetUp() override {
+    std::unique_ptr<DestructingTestViewsDelegate> views_delegate(
+        new DestructingTestViewsDelegate());
+    test_views_delegate_ = views_delegate.get();
+    // ViewsTestBase takes ownership, destroying during Teardown.
+    set_views_delegate(std::move(views_delegate));
     ViewsTestBase::SetUp();
     Init();
     ASSERT_TRUE(base::MessageLoopForUI::IsCurrent());
@@ -402,6 +433,16 @@ class MenuControllerTest : public ViewsTestBase {
     EXPECT_EQ(0, menu_controller_delegate_->on_menu_closed_called());
   }
 
+  // Tests that destroying the menu during ViewsDelegate::ReleaseRef does not
+  // cause a crash.
+  void TestDestroyedDuringViewsRelease() {
+    // |test_views_delegate_| is owned by views::ViewsTestBase and not deleted
+    // until TearDown. MenuControllerTest outlives it.
+    test_views_delegate_->set_release_ref_callback(base::Bind(
+        &MenuControllerTest::DestroyMenuController, base::Unretained(this)));
+    menu_controller_->ExitAsyncRun();
+  }
+
  protected:
   void SetPendingStateItem(MenuItemView* item) {
     menu_controller_->pending_state_.item = item;
@@ -613,6 +654,9 @@ class MenuControllerTest : public ViewsTestBase {
     menu_item_->SetController(menu_controller_);
   }
 
+  // Not owned.
+  DestructingTestViewsDelegate* test_views_delegate_;
+
   std::unique_ptr<Widget> owner_;
   std::unique_ptr<ui::test::EventGenerator> event_generator_;
   std::unique_ptr<TestMenuItemViewShown> menu_item_;
@@ -1458,6 +1502,21 @@ TEST_F(MenuControllerTest, CancelAllDuringDrag) {
   StartDrag();
 }
 
+// Tests that when releasing the ref on ViewsDelegate and MenuController is
+// deleted, that shutdown occurs without crashing.
+TEST_F(MenuControllerTest, DestroyedDuringViewsRelease) {
+  ExitMenuRun();
+  MenuController* controller = menu_controller();
+  controller->SetAsyncRun(true);
+
+  int mouse_event_flags = 0;
+  MenuItemView* run_result =
+      controller->Run(owner(), nullptr, menu_item(), gfx::Rect(),
+                      MENU_ANCHOR_TOPLEFT, false, false, &mouse_event_flags);
+  EXPECT_EQ(run_result, nullptr);
+  TestDestroyedDuringViewsRelease();
+}
+
 #endif  // defined(USE_AURA)
 
 }  // namespace test