views: Terminate the nested message-loop correctly when closing the menu.

It is possible for a non-menu source to start a nested message loop
(e.g. for dragging a menu-item). So attempting to terminate the current
iteration of the message-loop when the menu is closed can be problematic.
So instead, terminate the message-loop through the DispatcherClient.

BUG=367850, 372208
R=sky@chromium.org

Review URL: https://codereview.chromium.org/279073002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@269819 0039d316-1c4b-4281-b951-d872f2087c98

ui/views/controls/menu/menu_controller.cc

@@ -2378,17 +2378,19 @@ void MenuController::SetExitType(ExitType type) {
   // the current loop.
   bool quit_now = ShouldQuitNow() && exit_type_ != EXIT_NONE &&
       message_loop_depth_;
+  if (quit_now)
+    TerminateNestedMessageLoop();
+}
 
-  if (quit_now) {
-    if (owner_) {
-      aura::Window* root = owner_->GetNativeWindow()->GetRootWindow();
-      aura::client::GetDispatcherClient(root)->QuitNestedMessageLoop();
-    } else {
-      base::MessageLoop::current()->QuitNow();
-    }
-    // Restore the previous dispatcher.
-    nested_dispatcher_.reset();
+void MenuController::TerminateNestedMessageLoop() {
+  if (owner_) {
+    aura::Window* root = owner_->GetNativeWindow()->GetRootWindow();
+    aura::client::GetDispatcherClient(root)->QuitNestedMessageLoop();
+  } else {
+    base::MessageLoop::current()->QuitNow();
   }
+  // Restore the previous dispatcher.
+  nested_dispatcher_.reset();
 }
 
 bool MenuController::ShouldQuitNow() const {

ui/views/controls/menu/menu_controller.h

@@ -150,6 +150,7 @@ class VIEWS_EXPORT MenuController : public WidgetObserver {
   friend class internal::MenuEventDispatcher;
   friend class internal::MenuMessagePumpDispatcher;
   friend class internal::MenuRunnerImpl;
+  friend class MenuControllerTest;
   friend class MenuHostRootView;
   friend class MenuItemView;
   friend class SubmenuView;
@@ -471,9 +472,12 @@ class VIEWS_EXPORT MenuController : public WidgetObserver {
   void SetActiveMouseView(View* view);
   View* GetActiveMouseView();
 
-  // Sets exit type.
+  // Sets exit type. Calling this can terminate the active nested message-loop.
   void SetExitType(ExitType type);
 
+  // Terminates the current nested message-loop.
+  void TerminateNestedMessageLoop();
+
   // Returns true if SetExitType() should quit the message loop.
   bool ShouldQuitNow() const;
 

ui/views/controls/menu/menu_controller_unittest.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ui/views/controls/menu/menu_controller.h"
+
+#include "base/run_loop.h"
+#include "ui/aura/window.h"
+#include "ui/events/platform/platform_event_source.h"
+#include "ui/views/test/views_test_base.h"
+#include "ui/wm/public/dispatcher_client.h"
+
+#if defined(OS_WIN)
+#include "base/message_loop/message_pump_dispatcher.h"
+#elif defined(USE_X11)
+#include <X11/Xlib.h>
+#undef Bool
+#undef None
+#endif
+
+namespace views {
+
+namespace {
+
+class TestPlatformEventSource : public ui::PlatformEventSource {
+ public:
+  TestPlatformEventSource() {}
+  virtual ~TestPlatformEventSource() {}
+
+  uint32_t Dispatch(const ui::PlatformEvent& event) {
+    return DispatchEvent(event);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestPlatformEventSource);
+};
+
+class TestDispatcherClient : public aura::client::DispatcherClient {
+ public:
+  TestDispatcherClient() : dispatcher_(NULL) {}
+  virtual ~TestDispatcherClient() {}
+
+  base::MessagePumpDispatcher* dispatcher() {
+    return dispatcher_;
+  }
+
+  // aura::client::DispatcherClient:
+  virtual void RunWithDispatcher(
+      base::MessagePumpDispatcher* dispatcher) OVERRIDE {
+    base::AutoReset<base::MessagePumpDispatcher*> reset_dispatcher(&dispatcher_,
+                                                                   dispatcher);
+    base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
+    base::MessageLoop::ScopedNestableTaskAllower allow(loop);
+    base::RunLoop run_loop;
+    quit_callback_ = run_loop.QuitClosure();
+    run_loop.Run();
+  }
+
+  virtual void QuitNestedMessageLoop() OVERRIDE { quit_callback_.Run(); }
+
+ private:
+  base::MessagePumpDispatcher* dispatcher_;
+  base::Closure quit_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestDispatcherClient);
+};
+
+}  // namespace
+
+class MenuControllerTest : public ViewsTestBase {
+ public:
+  MenuControllerTest() : controller_(NULL) {}
+  virtual ~MenuControllerTest() {}
+
+  // Dispatches |count| number of items, each in a separate iteration of the
+  // message-loop, by posting a task.
+  void Step3_DispatchEvents(int count) {
+    base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
+    base::MessageLoop::ScopedNestableTaskAllower allow(loop);
+    controller_->exit_type_ = MenuController::EXIT_ALL;
+
+#if defined(USE_X11)
+    XEvent xevent;
+    memset(&xevent, 0, sizeof(xevent));
+    event_source_.Dispatch(&xevent);
+#else
+    MSG msg;
+    memset(&msg, 0, sizeof(MSG));
+    dispatcher_client_.dispatcher()->Dispatch(msg);
+#endif
+
+    if (count) {
+      base::MessageLoop::current()->PostTask(
+          FROM_HERE,
+          base::Bind(&MenuControllerTest::Step3_DispatchEvents,
+                     base::Unretained(this),
+                     count - 1));
+    } else {
+      EXPECT_TRUE(run_loop_->running());
+      run_loop_->Quit();
+    }
+  }
+
+  // Runs a nested message-loop that does not involve the menu itself.
+  void Step2_RunNestedLoop() {
+    base::MessageLoopForUI* loop = base::MessageLoopForUI::current();
+    base::MessageLoop::ScopedNestableTaskAllower allow(loop);
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&MenuControllerTest::Step3_DispatchEvents,
+                   base::Unretained(this),
+                   3));
+    run_loop_.reset(new base::RunLoop());
+    run_loop_->Run();
+  }
+
+  void Step1_RunMenu() {
+    Widget widget;
+    Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
+    params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+    widget.Init(params);
+    widget.Show();
+
+    aura::client::SetDispatcherClient(widget.GetNativeWindow()->GetRootWindow(),
+                                      &dispatcher_client_);
+
+    controller_ = new MenuController(NULL, true, NULL);
+    controller_->owner_ = &widget;
+    base::MessageLoop::current()->PostTask(
+        FROM_HERE,
+        base::Bind(&MenuControllerTest::Step2_RunNestedLoop,
+                   base::Unretained(this)));
+    controller_->RunMessageLoop(false);
+  }
+
+ private:
+  MenuController* controller_;
+  scoped_ptr<base::RunLoop> run_loop_;
+  TestPlatformEventSource event_source_;
+  TestDispatcherClient dispatcher_client_;
+
+  DISALLOW_COPY_AND_ASSIGN(MenuControllerTest);
+};
+
+TEST_F(MenuControllerTest, Basic) {
+  base::MessageLoop::ScopedNestableTaskAllower allow_nested(
+      base::MessageLoop::current());
+  message_loop()->PostTask(
+      FROM_HERE,
+      base::Bind(&MenuControllerTest::Step1_RunMenu, base::Unretained(this)));
+}
+
+}  // namespace views

ui/views/controls/menu/menu_event_dispatcher_linux.cc

@@ -22,35 +22,49 @@ bool MenuEventDispatcher::CanDispatchEvent(const ui::PlatformEvent& event) {
 }
 
 uint32_t MenuEventDispatcher::DispatchEvent(const ui::PlatformEvent& event) {
+  bool should_quit = false;
+  bool should_perform_default = true;
   if (menu_controller_->exit_type() == MenuController::EXIT_ALL ||
-      menu_controller_->exit_type() == MenuController::EXIT_DESTROYED)
-    return (ui::POST_DISPATCH_QUIT_LOOP | ui::POST_DISPATCH_PERFORM_DEFAULT);
-
-  switch (ui::EventTypeFromNative(event)) {
-    case ui::ET_KEY_PRESSED: {
-      if (!menu_controller_->OnKeyDown(ui::KeyboardCodeFromNative(event)))
-        return ui::POST_DISPATCH_QUIT_LOOP;
-
-      // Do not check mnemonics if the Alt or Ctrl modifiers are pressed.
-      int flags = ui::EventFlagsFromNative(event);
-      if ((flags & (ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN)) == 0) {
-        char c = ui::GetCharacterFromKeyCode(ui::KeyboardCodeFromNative(event),
-                                             flags);
-        if (menu_controller_->SelectByChar(c))
-          return ui::POST_DISPATCH_QUIT_LOOP;
+      menu_controller_->exit_type() == MenuController::EXIT_DESTROYED) {
+    should_quit = true;
+  } else {
+    switch (ui::EventTypeFromNative(event)) {
+      case ui::ET_KEY_PRESSED: {
+        if (!menu_controller_->OnKeyDown(ui::KeyboardCodeFromNative(event))) {
+          should_quit = true;
+          should_perform_default = false;
+          break;
+        }
+
+        // Do not check mnemonics if the Alt or Ctrl modifiers are pressed.
+        int flags = ui::EventFlagsFromNative(event);
+        if ((flags & (ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN)) == 0) {
+          char c = ui::GetCharacterFromKeyCode(
+              ui::KeyboardCodeFromNative(event), flags);
+          if (menu_controller_->SelectByChar(c)) {
+            should_quit = true;
+            should_perform_default = false;
+            break;
+          }
+        }
+        should_quit = false;
+        should_perform_default = false;
+        break;
       }
-      return ui::POST_DISPATCH_NONE;
+      case ui::ET_KEY_RELEASED:
+        should_quit = false;
+        should_perform_default = false;
+        break;
+      default:
+        break;
     }
-    case ui::ET_KEY_RELEASED:
-      return ui::POST_DISPATCH_NONE;
-    default:
-      break;
   }
 
-  return ui::POST_DISPATCH_PERFORM_DEFAULT |
-         (menu_controller_->exit_type() == MenuController::EXIT_NONE
-              ? ui::POST_DISPATCH_NONE
-              : ui::POST_DISPATCH_QUIT_LOOP);
+  if (should_quit || menu_controller_->exit_type() != MenuController::EXIT_NONE)
+    menu_controller_->TerminateNestedMessageLoop();
+
+  return should_perform_default ? ui::POST_DISPATCH_PERFORM_DEFAULT
+                                : ui::POST_DISPATCH_NONE;
 }
 
 }  // namespace internal

ui/views/controls/menu/menu_message_pump_dispatcher_win.cc

@@ -23,61 +23,72 @@ MenuMessagePumpDispatcher::~MenuMessagePumpDispatcher() {}
 uint32_t MenuMessagePumpDispatcher::Dispatch(const MSG& msg) {
   DCHECK(menu_controller_->IsBlockingRun());
 
+  bool should_quit = false;
+  bool should_perform_default = true;
   if (menu_controller_->exit_type() == MenuController::EXIT_ALL ||
-      menu_controller_->exit_type() == MenuController::EXIT_DESTROYED)
-    return (POST_DISPATCH_QUIT_LOOP | POST_DISPATCH_PERFORM_DEFAULT);
-
-  // NOTE: we don't get WM_ACTIVATE or anything else interesting in here.
-  switch (msg.message) {
-    case WM_CONTEXTMENU: {
-      MenuItemView* item = menu_controller_->pending_state_.item;
-      if (item && item->GetRootMenuItem() != item) {
-        gfx::Point screen_loc(0, item->height());
-        View::ConvertPointToScreen(item, &screen_loc);
-        ui::MenuSourceType source_type = ui::MENU_SOURCE_MOUSE;
-        if (GET_X_LPARAM(msg.lParam) == -1 && GET_Y_LPARAM(msg.lParam) == -1)
-          source_type = ui::MENU_SOURCE_KEYBOARD;
-        item->GetDelegate()->ShowContextMenu(
-            item, item->GetCommand(), screen_loc, source_type);
+      menu_controller_->exit_type() == MenuController::EXIT_DESTROYED) {
+    should_quit = true;
+  } else {
+    // NOTE: we don't get WM_ACTIVATE or anything else interesting in here.
+    switch (msg.message) {
+      case WM_CONTEXTMENU: {
+        MenuItemView* item = menu_controller_->pending_state_.item;
+        if (item && item->GetRootMenuItem() != item) {
+          gfx::Point screen_loc(0, item->height());
+          View::ConvertPointToScreen(item, &screen_loc);
+          ui::MenuSourceType source_type = ui::MENU_SOURCE_MOUSE;
+          if (GET_X_LPARAM(msg.lParam) == -1 && GET_Y_LPARAM(msg.lParam) == -1)
+            source_type = ui::MENU_SOURCE_KEYBOARD;
+          item->GetDelegate()->ShowContextMenu(
+              item, item->GetCommand(), screen_loc, source_type);
+        }
+        should_quit = false;
+        should_perform_default = false;
+        break;
       }
-      return POST_DISPATCH_NONE;
-    }
-
-    // NOTE: focus wasn't changed when the menu was shown. As such, don't
-    // dispatch key events otherwise the focused window will get the events.
-    case WM_KEYDOWN: {
-      bool result =
-          menu_controller_->OnKeyDown(ui::KeyboardCodeFromNative(msg));
-      TranslateMessage(&msg);
-      return result ? POST_DISPATCH_NONE : POST_DISPATCH_QUIT_LOOP;
-    }
-    case WM_CHAR: {
-      bool should_exit =
-          menu_controller_->SelectByChar(static_cast<base::char16>(msg.wParam));
-      return should_exit ? POST_DISPATCH_QUIT_LOOP : POST_DISPATCH_NONE;
-    }
-    case WM_KEYUP:
-      return POST_DISPATCH_NONE;
 
-    case WM_SYSKEYUP:
-      // We may have been shown on a system key, as such don't do anything
-      // here. If another system key is pushed we'll get a WM_SYSKEYDOWN and
-      // close the menu.
-      return POST_DISPATCH_NONE;
+      // NOTE: focus wasn't changed when the menu was shown. As such, don't
+      // dispatch key events otherwise the focused window will get the events.
+      case WM_KEYDOWN: {
+        bool result =
+            menu_controller_->OnKeyDown(ui::KeyboardCodeFromNative(msg));
+        TranslateMessage(&msg);
+        should_perform_default = false;
+        should_quit = !result;
+        break;
+      }
+      case WM_CHAR: {
+        should_quit = menu_controller_->SelectByChar(
+            static_cast<base::char16>(msg.wParam));
+        should_perform_default = false;
+        break;
+      }
+      case WM_KEYUP:
+      case WM_SYSKEYUP:
+        // We may have been shown on a system key, as such don't do anything
+        // here. If another system key is pushed we'll get a WM_SYSKEYDOWN and
+        // close the menu.
+        should_quit = false;
+        should_perform_default = false;
+        break;
 
-    case WM_CANCELMODE:
-    case WM_SYSKEYDOWN:
-      // Exit immediately on system keys.
-      menu_controller_->Cancel(MenuController::EXIT_ALL);
-      return POST_DISPATCH_QUIT_LOOP;
+      case WM_CANCELMODE:
+      case WM_SYSKEYDOWN:
+        // Exit immediately on system keys.
+        menu_controller_->Cancel(MenuController::EXIT_ALL);
+        should_quit = true;
+        should_perform_default = false;
+        break;
 
-    default:
-      break;
+      default:
+        break;
+    }
   }
-  return POST_DISPATCH_PERFORM_DEFAULT |
-         (menu_controller_->exit_type() == MenuController::EXIT_NONE
-              ? POST_DISPATCH_NONE
-              : POST_DISPATCH_QUIT_LOOP);
+
+  if (should_quit || menu_controller_->exit_type() != MenuController::EXIT_NONE)
+    menu_controller_->TerminateNestedMessageLoop();
+  return should_perform_default ? POST_DISPATCH_PERFORM_DEFAULT
+                                : POST_DISPATCH_NONE;
 }
 
 }  // namespace internal

ui/views/views.gyp

@@ -622,6 +622,7 @@
         'controls/combobox/combobox_unittest.cc',
         'controls/label_unittest.cc',
         'controls/menu/menu_model_adapter_unittest.cc',
+        'controls/menu/menu_controller_unittest.cc',
         'controls/native/native_view_host_aura_unittest.cc',
         'controls/native/native_view_host_unittest.cc',
         'controls/prefix_selector_unittest.cc',