Re-enable listening on private runloop modes, for a whitelist.

r470769 started observing private AppKit run loop modes to keep
UI and web pages responsive while menus were fading out, but it
interacted badly with app-modal dialogs.

This re-enables r470769, but only in 3 places:
 - Web page context menus
 - Web page <select> menus
 - menus run from toolkit-views UI.

      pause briefly when either the <select> menu or the right-
      click context menu are fading out. (The main menubar and the
      Chrome app/wrench menu still cause a brief pause when dismissed).

Bug: 726200, 602914, 640466, 652007
Test: https://jsfiddle.net/kvmy8q9d/5/ - the animation shouldn't
Change-Id: Idf769c1f830ccb3ddc67205acb9e86aac2ec0460
Reviewed-on: https://chromium-review.googlesource.com/647836Reviewed-by: Mark Mentovai <mark@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503112}

base/message_loop/message_pump_mac.h

@@ -91,9 +91,10 @@ class BASE_EXPORT MessagePumpCFRunLoopBase : public MessagePump {
   friend class MessagePumpScopedAutoreleasePool;
   friend class TestMessagePumpCFRunLoopBase;
 
-  // Tasks will be pumped in the run loop modes described by |mode_mask|, which
-  // maps bits to the index of an internal array of run loop mode identifiers.
-  explicit MessagePumpCFRunLoopBase(int mode_mask);
+  // Tasks will be pumped in the run loop modes described by
+  // |initial_mode_mask|, which maps bits to the index of an internal array of
+  // run loop mode identifiers.
+  explicit MessagePumpCFRunLoopBase(int initial_mode_mask);
   ~MessagePumpCFRunLoopBase() override;
 
   // Subclasses should implement the work they need to do in MessagePump::Run
@@ -117,12 +118,18 @@ class BASE_EXPORT MessagePumpCFRunLoopBase : public MessagePump {
   // objects autoreleased by work to fall into the current autorelease pool.
   virtual AutoreleasePoolType* CreateAutoreleasePool();
 
-  // Invokes function(run_loop_, arg, mode) for all the modes in |mode_mask_|.
-  template <typename Argument>
-  void InvokeForEnabledModes(void function(CFRunLoopRef, Argument, CFStringRef),
-                             Argument argument);
+  // Enable and disable entries in |enabled_modes_| to match |mode_mask|.
+  void SetModeMask(int mode_mask);
+
+  // Get the current mode mask from |enabled_modes_|.
+  int GetModeMask() const;
 
  private:
+  class ScopedModeEnabler;
+
+  // The maximum number of run loop modes that can be monitored.
+  static constexpr int kNumModes = 4;
+
   // Marking timers as invalid at the right time helps significantly reduce
   // power use (see the comment in RunDelayedWorkTimer()), however there is no
   // public API for doing so. CFRuntime.h states that CFRuntimeBase, upon which
@@ -198,8 +205,8 @@ class BASE_EXPORT MessagePumpCFRunLoopBase : public MessagePump {
   // The thread's run loop.
   CFRunLoopRef run_loop_;
 
-  // Bitmask controlling the run loop modes in which posted tasks may run.
-  const int mode_mask_;
+  // The enabled modes. Posted tasks may run in any non-null entry.
+  std::unique_ptr<ScopedModeEnabler> enabled_modes_[kNumModes];
 
   // The timer, sources, and observers are described above alongside their
   // callbacks.
@@ -308,6 +315,17 @@ class MessagePumpUIApplication : public MessagePumpCFRunLoopBase {
 
 #else
 
+// While in scope, permits posted tasks to be run in private AppKit run loop
+// modes that would otherwise make the UI unresponsive. E.g., menu fade out.
+class BASE_EXPORT ScopedPumpMessagesInPrivateModes {
+ public:
+  ScopedPumpMessagesInPrivateModes();
+  ~ScopedPumpMessagesInPrivateModes();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ScopedPumpMessagesInPrivateModes);
+};
+
 class MessagePumpNSApplication : public MessagePumpCFRunLoopBase {
  public:
   MessagePumpNSApplication();
@@ -317,6 +335,8 @@ class MessagePumpNSApplication : public MessagePumpCFRunLoopBase {
   void Quit() override;
 
  private:
+  friend class ScopedPumpMessagesInPrivateModes;
+
   // False after Quit is called.
   bool keep_running_;
 

base/message_loop/message_pump_mac.mm

@@ -12,6 +12,7 @@
 #include "base/mac/call_with_eh_frame.h"
 #include "base/mac/scoped_cftyperef.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
 #include "base/message_loop/timer_slack.h"
 #include "base/run_loop.h"
 #include "base/time/time.h"
@@ -45,17 +46,17 @@ const CFStringRef kAllModes[] = {
 };
 
 // Mask that determines which modes in |kAllModes| to use.
-enum { kCommonModeMask = 0x1, kAllModesMask = ~0 };
+enum { kCommonModeMask = 0x1, kAllModesMask = 0xf };
 
-// Modes to use for MessagePumpNSApplication. Currently just common and
-// exclusive modes. TODO(tapted): Use kAllModesMask once http://crbug.com/640466
-// blockers are fixed.
-enum { kNSApplicationModeMask = 0x3 };
+// Modes to use for MessagePumpNSApplication that are considered "safe".
+// Currently just common and exclusive modes. Ideally, messages would be pumped
+// in all modes, but that interacts badly with app modal dialogs (e.g. NSAlert).
+enum { kNSApplicationModalSafeModeMask = 0x3 };
 
 void NoOp(void* info) {
 }
 
-const CFTimeInterval kCFTimeIntervalMax =
+constexpr CFTimeInterval kCFTimeIntervalMax =
     std::numeric_limits<CFTimeInterval>::max();
 
 #if !defined(OS_IOS)
@@ -63,6 +64,9 @@ const CFTimeInterval kCFTimeIntervalMax =
 // initialized.  Only accessed from the main thread.
 bool g_not_using_cr_app = false;
 
+// The MessagePump controlling [NSApp run].
+MessagePumpNSApplication* g_app_pump;
+
 // Various CoreFoundation definitions.
 typedef struct __CFRuntimeBase {
   uintptr_t _cfisa;
@@ -128,6 +132,40 @@ class MessagePumpScopedAutoreleasePool {
   DISALLOW_COPY_AND_ASSIGN(MessagePumpScopedAutoreleasePool);
 };
 
+class MessagePumpCFRunLoopBase::ScopedModeEnabler {
+ public:
+  ScopedModeEnabler(MessagePumpCFRunLoopBase* owner, int mode_index)
+      : owner_(owner), mode_index_(mode_index) {
+    CFRunLoopRef loop = owner_->run_loop_;
+    CFRunLoopAddTimer(loop, owner_->delayed_work_timer_, mode());
+    CFRunLoopAddSource(loop, owner_->work_source_, mode());
+    CFRunLoopAddSource(loop, owner_->idle_work_source_, mode());
+    CFRunLoopAddSource(loop, owner_->nesting_deferred_work_source_, mode());
+    CFRunLoopAddObserver(loop, owner_->pre_wait_observer_, mode());
+    CFRunLoopAddObserver(loop, owner_->pre_source_observer_, mode());
+    CFRunLoopAddObserver(loop, owner_->enter_exit_observer_, mode());
+  }
+
+  ~ScopedModeEnabler() {
+    CFRunLoopRef loop = owner_->run_loop_;
+    CFRunLoopRemoveObserver(loop, owner_->enter_exit_observer_, mode());
+    CFRunLoopRemoveObserver(loop, owner_->pre_source_observer_, mode());
+    CFRunLoopRemoveObserver(loop, owner_->pre_wait_observer_, mode());
+    CFRunLoopRemoveSource(loop, owner_->nesting_deferred_work_source_, mode());
+    CFRunLoopRemoveSource(loop, owner_->idle_work_source_, mode());
+    CFRunLoopRemoveSource(loop, owner_->work_source_, mode());
+    CFRunLoopRemoveTimer(loop, owner_->delayed_work_timer_, mode());
+  }
+
+  const CFStringRef& mode() const { return kAllModes[mode_index_]; }
+
+ private:
+  MessagePumpCFRunLoopBase* const owner_;  // Weak. Owns this.
+  const int mode_index_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedModeEnabler);
+};
+
 // Must be called on the run loop thread.
 void MessagePumpCFRunLoopBase::Run(Delegate* delegate) {
   // nesting_level_ will be incremented in EnterExitRunLoop, so set
@@ -180,9 +218,8 @@ void MessagePumpCFRunLoopBase::SetTimerSlack(TimerSlack timer_slack) {
 }
 
 // Must be called on the run loop thread.
-MessagePumpCFRunLoopBase::MessagePumpCFRunLoopBase(int mode_mask)
-    : mode_mask_(mode_mask),
-      delegate_(NULL),
+MessagePumpCFRunLoopBase::MessagePumpCFRunLoopBase(int initial_mode_mask)
+    : delegate_(NULL),
       delayed_work_fire_time_(kCFTimeIntervalMax),
       timer_slack_(base::TIMER_SLACK_NONE),
       nesting_level_(0),
@@ -205,7 +242,6 @@ MessagePumpCFRunLoopBase::MessagePumpCFRunLoopBase(int mode_mask)
                                              0,                   // priority
                                              RunDelayedWorkTimer,
                                              &timer_context);
-  InvokeForEnabledModes(&CFRunLoopAddTimer, delayed_work_timer_);
 
   CFRunLoopSourceContext source_context = CFRunLoopSourceContext();
   source_context.info = this;
@@ -213,19 +249,14 @@ MessagePumpCFRunLoopBase::MessagePumpCFRunLoopBase(int mode_mask)
   work_source_ = CFRunLoopSourceCreate(NULL,  // allocator
                                        1,     // priority
                                        &source_context);
-  InvokeForEnabledModes(&CFRunLoopAddSource, work_source_);
-
   source_context.perform = RunIdleWorkSource;
   idle_work_source_ = CFRunLoopSourceCreate(NULL,  // allocator
                                             2,     // priority
                                             &source_context);
-  InvokeForEnabledModes(&CFRunLoopAddSource, idle_work_source_);
-
   source_context.perform = RunNestingDeferredWorkSource;
   nesting_deferred_work_source_ = CFRunLoopSourceCreate(NULL,  // allocator
                                                         0,     // priority
                                                         &source_context);
-  InvokeForEnabledModes(&CFRunLoopAddSource, nesting_deferred_work_source_);
 
   CFRunLoopObserverContext observer_context = CFRunLoopObserverContext();
   observer_context.info = this;
@@ -235,16 +266,12 @@ MessagePumpCFRunLoopBase::MessagePumpCFRunLoopBase(int mode_mask)
                                                0,     // priority
                                                PreWaitObserver,
                                                &observer_context);
-  InvokeForEnabledModes(&CFRunLoopAddObserver, pre_wait_observer_);
-
   pre_source_observer_ = CFRunLoopObserverCreate(NULL,  // allocator
                                                  kCFRunLoopBeforeSources,
                                                  true,  // repeat
                                                  0,     // priority
                                                  PreSourceObserver,
                                                  &observer_context);
-  InvokeForEnabledModes(&CFRunLoopAddObserver, pre_source_observer_);
-
   enter_exit_observer_ = CFRunLoopObserverCreate(NULL,  // allocator
                                                  kCFRunLoopEntry |
                                                      kCFRunLoopExit,
@@ -252,24 +279,20 @@ MessagePumpCFRunLoopBase::MessagePumpCFRunLoopBase(int mode_mask)
                                                  0,     // priority
                                                  EnterExitObserver,
                                                  &observer_context);
-  InvokeForEnabledModes(&CFRunLoopAddObserver, enter_exit_observer_);
+  SetModeMask(initial_mode_mask);
 }
 
 // Ideally called on the run loop thread.  If other run loops were running
 // lower on the run loop thread's stack when this object was created, the
 // same number of run loops must be running when this object is destroyed.
 MessagePumpCFRunLoopBase::~MessagePumpCFRunLoopBase() {
-  for (const CFRunLoopObserverRef& observer :
-       {enter_exit_observer_, pre_source_observer_, pre_wait_observer_}) {
-    InvokeForEnabledModes(&CFRunLoopRemoveObserver, observer);
-    CFRelease(observer);
-  }
-  for (const CFRunLoopSourceRef& source :
-       {nesting_deferred_work_source_, idle_work_source_, work_source_}) {
-    InvokeForEnabledModes(&CFRunLoopRemoveSource, source);
-    CFRelease(source);
-  }
-  InvokeForEnabledModes(&CFRunLoopRemoveTimer, delayed_work_timer_);
+  SetModeMask(0);
+  CFRelease(enter_exit_observer_);
+  CFRelease(pre_source_observer_);
+  CFRelease(pre_wait_observer_);
+  CFRelease(nesting_deferred_work_source_);
+  CFRelease(idle_work_source_);
+  CFRelease(work_source_);
   CFRelease(delayed_work_timer_);
   CFRelease(run_loop_);
 }
@@ -297,17 +320,25 @@ AutoreleasePoolType* MessagePumpCFRunLoopBase::CreateAutoreleasePool() {
   return [[NSAutoreleasePool alloc] init];
 }
 
-template <typename Argument>
-void MessagePumpCFRunLoopBase::InvokeForEnabledModes(void method(CFRunLoopRef,
-                                                                 Argument,
-                                                                 CFStringRef),
-                                                     Argument argument) {
+void MessagePumpCFRunLoopBase::SetModeMask(int mode_mask) {
+  static_assert(arraysize(enabled_modes_) == arraysize(kAllModes),
+                "mode size mismatch");
   for (size_t i = 0; i < arraysize(kAllModes); ++i) {
-    if (mode_mask_ & (0x1 << i))
-      method(run_loop_, argument, kAllModes[i]);
+    bool enable = mode_mask & (0x1 << i);
+    if (enable == !enabled_modes_[i]) {
+      enabled_modes_[i] =
+          enable ? base::MakeUnique<ScopedModeEnabler>(this, i) : nullptr;
+    }
   }
 }
 
+int MessagePumpCFRunLoopBase::GetModeMask() const {
+  int mask = 0;
+  for (size_t i = 0; i < arraysize(enabled_modes_); ++i)
+    mask |= enabled_modes_[i] ? (0x1 << i) : 0;
+  return mask;
+}
+
 #if !defined(OS_IOS)
 // This function uses private API to modify a test timer's valid state and
 // uses public API to confirm that the private API changed the correct bit.
@@ -679,11 +710,11 @@ MessagePumpNSRunLoop::MessagePumpNSRunLoop()
   quit_source_ = CFRunLoopSourceCreate(NULL,  // allocator
                                        0,     // priority
                                        &source_context);
-  InvokeForEnabledModes(&CFRunLoopAddSource, quit_source_);
+  CFRunLoopAddSource(run_loop(), quit_source_, kCFRunLoopCommonModes);
 }
 
 MessagePumpNSRunLoop::~MessagePumpNSRunLoop() {
-  InvokeForEnabledModes(&CFRunLoopRemoveSource, quit_source_);
+  CFRunLoopRemoveSource(run_loop(), quit_source_, kCFRunLoopCommonModes);
   CFRelease(quit_source_);
 }
 
@@ -726,12 +757,33 @@ void MessagePumpUIApplication::Attach(Delegate* delegate) {
 
 #else
 
+ScopedPumpMessagesInPrivateModes::ScopedPumpMessagesInPrivateModes() {
+  // Pumping events in private runloop modes is known to interact badly with
+  // app modal windows like NSAlert.
+  CHECK(![NSApp modalWindow]);
+  DCHECK(g_app_pump);
+  DCHECK_EQ(kNSApplicationModalSafeModeMask, g_app_pump->GetModeMask());
+  g_app_pump->SetModeMask(kAllModesMask);
+}
+
+ScopedPumpMessagesInPrivateModes::~ScopedPumpMessagesInPrivateModes() {
+  DCHECK(g_app_pump);
+  DCHECK_EQ(kAllModesMask, g_app_pump->GetModeMask());
+  g_app_pump->SetModeMask(kNSApplicationModalSafeModeMask);
+}
+
 MessagePumpNSApplication::MessagePumpNSApplication()
-    : MessagePumpCFRunLoopBase(kNSApplicationModeMask),
+    : MessagePumpCFRunLoopBase(kNSApplicationModalSafeModeMask),
       keep_running_(true),
-      running_own_loop_(false) {}
+      running_own_loop_(false) {
+  DCHECK_EQ(nullptr, g_app_pump);
+  g_app_pump = this;
+}
 
-MessagePumpNSApplication::~MessagePumpNSApplication() {}
+MessagePumpNSApplication::~MessagePumpNSApplication() {
+  DCHECK_EQ(this, g_app_pump);
+  g_app_pump = nullptr;
+}
 
 void MessagePumpNSApplication::DoRun(Delegate* delegate) {
   bool last_running_own_loop_ = running_own_loop_;

base/message_loop/message_pump_mac_unittest.cc

@@ -6,6 +6,8 @@
 
 #include "base/mac/scoped_cftyperef.h"
 #include "base/macros.h"
+#include "base/message_loop/message_loop.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
@@ -105,4 +107,67 @@ TEST(MessagePumpMacTest, TestInvalidatedTimerReuse) {
   CFRunLoopRemoveTimer(CFRunLoopGetCurrent(), test_timer,
                        kMessageLoopExclusiveRunLoopMode);
 }
+
+namespace {
+
+// PostedTasks are only executed while the message pump has a delegate. That is,
+// when a base::RunLoop is running, so in order to test whether posted tasks
+// are run by CFRunLoopRunInMode and *not* by the regular RunLoop, we need to
+// be inside a task that is also calling CFRunLoopRunInMode. This task runs the
+// given |mode| after posting a task to increment a counter, then checks whether
+// the counter incremented after emptying that run loop mode.
+void IncrementInModeAndExpect(CFRunLoopMode mode, int result) {
+  // Since this task is "ours" rather than a system task, allow nesting.
+  MessageLoop::ScopedNestableTaskAllower allow(MessageLoop::current());
+  int counter = 0;
+  auto increment = BindRepeating([](int* i) { ++*i; }, &counter);
+  ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, increment);
+  while (CFRunLoopRunInMode(mode, 0, true) == kCFRunLoopRunHandledSource)
+    ;
+  ASSERT_EQ(result, counter);
+}
+
+}  // namespace
+
+// Tests the correct behavior of ScopedPumpMessagesInPrivateModes.
+TEST(MessagePumpMacTest, ScopedPumpMessagesInPrivateModes) {
+  MessageLoopForUI message_loop;
+
+  CFRunLoopMode kRegular = kCFRunLoopDefaultMode;
+  CFRunLoopMode kPrivate = CFSTR("NSUnhighlightMenuRunLoopMode");
+
+  // Work is seen when running in the default mode.
+  ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, BindOnce(&IncrementInModeAndExpect, kRegular, 1));
+  EXPECT_NO_FATAL_FAILURE(RunLoop().RunUntilIdle());
+
+  // But not seen when running in a private mode.
+  ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, BindOnce(&IncrementInModeAndExpect, kPrivate, 0));
+  EXPECT_NO_FATAL_FAILURE(RunLoop().RunUntilIdle());
+
+  {
+    ScopedPumpMessagesInPrivateModes allow_private;
+    // Now the work should be seen.
+    ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, BindOnce(&IncrementInModeAndExpect, kPrivate, 1));
+    EXPECT_NO_FATAL_FAILURE(RunLoop().RunUntilIdle());
+
+    // The regular mode should also work the same.
+    ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, BindOnce(&IncrementInModeAndExpect, kRegular, 1));
+    EXPECT_NO_FATAL_FAILURE(RunLoop().RunUntilIdle());
+  }
+
+  // And now the scoper is out of scope, private modes should no longer see it.
+  ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, BindOnce(&IncrementInModeAndExpect, kPrivate, 0));
+  EXPECT_NO_FATAL_FAILURE(RunLoop().RunUntilIdle());
+
+  // Only regular modes see it.
+  ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, BindOnce(&IncrementInModeAndExpect, kRegular, 1));
+  EXPECT_NO_FATAL_FAILURE(RunLoop().RunUntilIdle());
+}
+
 }  // namespace base

chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm

@@ -12,6 +12,7 @@
 #import "base/mac/scoped_sending_event.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
+#import "base/message_loop/message_pump_mac.h"
 #include "base/strings/sys_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #import "chrome/browser/mac/nsprocessinfo_additions.h"
@@ -224,6 +225,9 @@ void RenderViewContextMenuMac::Show() {
     base::MessageLoop::ScopedNestableTaskAllower allow(
         base::MessageLoop::current());
 
+    // Ensure the UI can update while the menu is fading out.
+    base::ScopedPumpMessagesInPrivateModes pump_private;
+
     // One of the events that could be pumped is |window.close()|.
     // User-initiated event-tracking loops protect against this by
     // setting flags in -[CrApplication sendEvent:], but since

content/browser/renderer_host/webmenurunner_mac.mm

@@ -6,6 +6,7 @@
 
 #include <stddef.h>
 
+#import "base/message_loop/message_pump_mac.h"
 #include "base/strings/sys_string_conversions.h"
 
 @interface WebMenuRunner (PrivateAPI)
@@ -155,7 +156,11 @@
 
   // Display the menu, and set a flag if a menu item was chosen.
   [cell attachPopUpWithFrame:[dummyView bounds] inView:dummyView];
-  [cell performClickWithFrame:[dummyView bounds] inView:dummyView];
+  {
+    // Ensure the UI can update while the menu is fading out.
+    base::ScopedPumpMessagesInPrivateModes pump_private;
+    [cell performClickWithFrame:[dummyView bounds] inView:dummyView];
+  }
 
   [dummyView removeFromSuperview];
 

ui/views/controls/menu/menu_runner_impl_cocoa.mm

@@ -5,6 +5,7 @@
 #import "ui/views/controls/menu/menu_runner_impl_cocoa.h"
 
 #include "base/mac/sdk_forward_declarations.h"
+#import "base/message_loop/message_pump_mac.h"
 #import "ui/base/cocoa/cocoa_base_utils.h"
 #import "ui/base/cocoa/menu_controller.h"
 #include "ui/base/models/menu_model.h"
@@ -164,6 +165,9 @@ void MenuRunnerImplCocoa::RunMenuAt(Widget* parent,
   closing_event_time_ = base::TimeTicks();
   running_ = true;
 
+  // Ensure the UI can update while the menu is fading out.
+  base::ScopedPumpMessagesInPrivateModes pump_private;
+
   NSWindow* window = parent->GetNativeWindow();
   if (run_types & MenuRunner::CONTEXT_MENU) {
     [NSMenu popUpContextMenu:[menu_controller_ menu]