base: Don't do delayed work after quitting in Mac message pump

This patch fixes a problem in the Mac message pump where we kept calling
Delegate::DoDelayedWork even if a previous call to Delegate::DoWork caused the
runloop to quit. This matches the behavior outlined in message_pump.h[1] as
well as that of the other platform-specific pumps.

We also introduce a message pump unit test that verifies behavior across all
message pump types.

Bug: 891670

[1] https://cs.chromium.org/chromium/src/base/message_loop/message_pump.h?rcl=a28a6097182e739c4a8c600b538fcc07681fc7a3&l=58

Change-Id: I00ccb5c55ac8551259a2febd343ec64fdd55630d
Reviewed-on: https://chromium-review.googlesource.com/c/1370367Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615528}

base/BUILD.gn

@@ -2368,6 +2368,7 @@ test("base_unittests") {
     "message_loop/message_pump_glib_unittest.cc",
     "message_loop/message_pump_io_ios_unittest.cc",
     "message_loop/message_pump_mac_unittest.mm",
+    "message_loop/message_pump_unittest.cc",
     "metrics/bucket_ranges_unittest.cc",
     "metrics/field_trial_memory_mac_unittest.cc",
     "metrics/field_trial_params_unittest.cc",

base/message_loop/message_pump_mac.h

@@ -11,7 +11,7 @@
 // MessagePump interface called CFRunLoopBase.  CFRunLoopBase contains all
 // of the machinery necessary to dispatch events to a delegate, but does not
 // implement the specific run loop.  Concrete subclasses must provide their
-// own DoRun and Quit implementations.
+// own DoRun and DoQuit implementations.
 //
 // A concrete subclass that just runs a CFRunLoop loop is provided in
 // MessagePumpCFRunLoop.  For an NSRunLoop, the similar MessagePumpNSRunLoop
@@ -83,6 +83,7 @@ class BASE_EXPORT MessagePumpCFRunLoopBase : public MessagePump {
  public:
   // MessagePump:
   void Run(Delegate* delegate) override;
+  void Quit() override;
   void ScheduleWork() override;
   void ScheduleDelayedWork(const TimeTicks& delayed_work_time) override;
   void SetTimerSlack(TimerSlack timer_slack) override;
@@ -104,10 +105,20 @@ class BASE_EXPORT MessagePumpCFRunLoopBase : public MessagePump {
   // up and tear down things before and after the "meat" of DoRun.
   virtual void DoRun(Delegate* delegate) = 0;
 
+  // Similar to DoRun, this allows subclasses to perform custom handling when
+  // quitting a run loop. Return true if the quit took effect immediately;
+  // otherwise call OnDidQuit() when the quit is actually applied (e.g., a
+  // nested native runloop exited).
+  virtual bool DoQuit() = 0;
+
+  // Should be called by subclasses to signal when a deferred quit takes place.
+  void OnDidQuit();
+
   // Accessors for private data members to be used by subclasses.
   CFRunLoopRef run_loop() const { return run_loop_; }
   int nesting_level() const { return nesting_level_; }
   int run_nesting_level() const { return run_nesting_level_; }
+  bool keep_running() const { return keep_running_; }
 
   // Sets this pump's delegate.  Signals the appropriate sources if
   // |delegateless_work_| is true.  |delegate| can be NULL.
@@ -251,6 +262,10 @@ class BASE_EXPORT MessagePumpCFRunLoopBase : public MessagePump {
   // most recent attempt to run nesting-deferred work.
   int deepest_nesting_level_;
 
+  // Whether we should continue running application tasks. Set to false when
+  // Quit() is called for the innermost run loop.
+  bool keep_running_;
+
   // "Delegateless" work flags are set when work is ready to be performed but
   // must wait until a delegate is available to process it.  This can happen
   // when a MessagePumpCFRunLoopBase is instantiated and work arrives without
@@ -276,7 +291,7 @@ class BASE_EXPORT MessagePumpCFRunLoop : public MessagePumpCFRunLoopBase {
   ~MessagePumpCFRunLoop() override;
 
   void DoRun(Delegate* delegate) override;
-  void Quit() override;
+  bool DoQuit() override;
 
  private:
   void EnterExitRunLoop(CFRunLoopActivity activity) override;
@@ -295,7 +310,7 @@ class BASE_EXPORT MessagePumpNSRunLoop : public MessagePumpCFRunLoopBase {
   ~MessagePumpNSRunLoop() override;
 
   void DoRun(Delegate* delegate) override;
-  void Quit() override;
+  bool DoQuit() override;
 
  private:
   // A source that doesn't do anything but provide something signalable
@@ -303,9 +318,6 @@ class BASE_EXPORT MessagePumpNSRunLoop : public MessagePumpCFRunLoopBase {
   // is called, to cause the loop to wake up so that it can stop.
   CFRunLoopSourceRef quit_source_;
 
-  // False after Quit is called.
-  bool keep_running_;
-
   DISALLOW_COPY_AND_ASSIGN(MessagePumpNSRunLoop);
 };
 
@@ -318,7 +330,7 @@ class MessagePumpUIApplication : public MessagePumpCFRunLoopBase {
   MessagePumpUIApplication();
   ~MessagePumpUIApplication() override;
   void DoRun(Delegate* delegate) override;
-  void Quit() override;
+  bool DoQuit() override;
 
   // This message pump can not spin the main message loop directly.  Instead,
   // call |Attach()| to set up a delegate.  It is an error to call |Run()|.
@@ -351,14 +363,11 @@ class MessagePumpNSApplication : public MessagePumpCFRunLoopBase {
   ~MessagePumpNSApplication() override;
 
   void DoRun(Delegate* delegate) override;
-  void Quit() override;
+  bool DoQuit() override;
 
  private:
   friend class ScopedPumpMessagesInPrivateModes;
 
-  // False after Quit is called.
-  bool keep_running_;
-
   // True if DoRun is managing its own run loop as opposed to letting
   // -[NSApplication run] handle it.  The outermost run loop in the application
   // is managed by -[NSApplication run], inner run loops are handled by a loop

base/message_loop/message_pump_mac.mm

@@ -173,6 +173,7 @@ class MessagePumpCFRunLoopBase::ScopedModeEnabler {
 
 // Must be called on the run loop thread.
 void MessagePumpCFRunLoopBase::Run(Delegate* delegate) {
+  AutoReset<bool> auto_reset_keep_running(&keep_running_, true);
   // nesting_level_ will be incremented in EnterExitRunLoop, so set
   // run_nesting_level_ accordingly.
   int last_run_nesting_level = run_nesting_level_;
@@ -188,6 +189,15 @@ void MessagePumpCFRunLoopBase::Run(Delegate* delegate) {
   run_nesting_level_ = last_run_nesting_level;
 }
 
+void MessagePumpCFRunLoopBase::Quit() {
+  if (DoQuit())
+    OnDidQuit();
+}
+
+void MessagePumpCFRunLoopBase::OnDidQuit() {
+  keep_running_ = false;
+}
+
 // May be called on any thread.
 void MessagePumpCFRunLoopBase::ScheduleWork() {
   CFRunLoopSourceSignal(work_source_);
@@ -230,6 +240,7 @@ MessagePumpCFRunLoopBase::MessagePumpCFRunLoopBase(int initial_mode_mask)
       nesting_level_(0),
       run_nesting_level_(0),
       deepest_nesting_level_(0),
+      keep_running_(true),
       delegateless_work_(false),
       delegateless_idle_work_(false),
       allow_timer_invalidation_(true) {
@@ -475,6 +486,7 @@ bool MessagePumpCFRunLoopBase::RunWork() {
   bool resignal_work_source = did_work;
 
   TimeTicks next_time;
+  if (keep_running())
     delegate_->DoDelayedWork(&next_time);
   if (!did_work) {
     // Determine whether there's more delayed work, and if so, if it needs to
@@ -522,6 +534,8 @@ bool MessagePumpCFRunLoopBase::RunIdleWork() {
     delegateless_idle_work_ = true;
     return false;
   }
+  if (!keep_running())
+    return false;
 
   // The NSApplication-based run loop only drains the autorelease pool at each
   // UI event (NSEvent).  The autorelease pool is not drained for each
@@ -696,11 +710,12 @@ void MessagePumpCFRunLoop::DoRun(Delegate* delegate) {
 }
 
 // Must be called on the run loop thread.
-void MessagePumpCFRunLoop::Quit() {
+bool MessagePumpCFRunLoop::DoQuit() {
   // Stop the innermost run loop managed by this MessagePumpCFRunLoop object.
   if (nesting_level() == run_nesting_level()) {
     // This object is running the innermost loop, just stop it.
     CFRunLoopStop(run_loop());
+    return true;
   } else {
     // There's another loop running inside the loop managed by this object.
     // In other words, someone else called CFRunLoopRunInMode on the same
@@ -708,6 +723,7 @@ void MessagePumpCFRunLoop::Quit() {
     // other run loops, just mark this object to quit the innermost Run as
     // soon as the other inner loops not managed by Run are done.
     quit_pending_ = true;
+    return false;
   }
 }
 
@@ -722,11 +738,12 @@ void MessagePumpCFRunLoop::EnterExitRunLoop(CFRunLoopActivity activity) {
     // just inside Run.
     CFRunLoopStop(run_loop());
     quit_pending_ = false;
+    OnDidQuit();
   }
 }
 
 MessagePumpNSRunLoop::MessagePumpNSRunLoop()
-    : MessagePumpCFRunLoopBase(kCommonModeMask), keep_running_(true) {
+    : MessagePumpCFRunLoopBase(kCommonModeMask) {
   CFRunLoopSourceContext source_context = CFRunLoopSourceContext();
   source_context.perform = NoOp;
   quit_source_ = CFRunLoopSourceCreate(NULL,  // allocator
@@ -741,19 +758,17 @@ MessagePumpNSRunLoop::~MessagePumpNSRunLoop() {
 }
 
 void MessagePumpNSRunLoop::DoRun(Delegate* delegate) {
-  AutoReset<bool> auto_reset_keep_running(&keep_running_, true);
-
-  while (keep_running_) {
+  while (keep_running()) {
     // NSRunLoop manages autorelease pools itself.
     [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode
                              beforeDate:[NSDate distantFuture]];
   }
 }
 
-void MessagePumpNSRunLoop::Quit() {
-  keep_running_ = false;
+bool MessagePumpNSRunLoop::DoQuit() {
   CFRunLoopSourceSignal(quit_source_);
   CFRunLoopWakeUp(run_loop());
+  return true;
 }
 
 #if defined(OS_IOS)
@@ -766,8 +781,9 @@ void MessagePumpUIApplication::DoRun(Delegate* delegate) {
   NOTREACHED();
 }
 
-void MessagePumpUIApplication::Quit() {
+bool MessagePumpUIApplication::DoQuit() {
   NOTREACHED();
+  return false;
 }
 
 void MessagePumpUIApplication::Attach(Delegate* delegate) {
@@ -803,7 +819,6 @@ int ScopedPumpMessagesInPrivateModes::GetModeMaskForTest() {
 
 MessagePumpNSApplication::MessagePumpNSApplication()
     : MessagePumpCFRunLoopBase(kNSApplicationModalSafeModeMask),
-      keep_running_(true),
       running_own_loop_(false) {
   DCHECK_EQ(nullptr, g_app_pump);
   g_app_pump = this;
@@ -815,7 +830,6 @@ MessagePumpNSApplication::~MessagePumpNSApplication() {
 }
 
 void MessagePumpNSApplication::DoRun(Delegate* delegate) {
-  AutoReset<bool> auto_reset_keep_running(&keep_running_, true);
   bool last_running_own_loop_ = running_own_loop_;
 
   // NSApp must be initialized by calling:
@@ -832,7 +846,7 @@ void MessagePumpNSApplication::DoRun(Delegate* delegate) {
   } else {
     running_own_loop_ = true;
     NSDate* distant_future = [NSDate distantFuture];
-    while (keep_running_) {
+    while (keep_running()) {
       MessagePumpScopedAutoreleasePool autorelease_pool(this);
       NSEvent* event = [NSApp nextEventMatchingMask:NSAnyEventMask
                                           untilDate:distant_future
@@ -847,11 +861,9 @@ void MessagePumpNSApplication::DoRun(Delegate* delegate) {
   running_own_loop_ = last_running_own_loop_;
 }
 
-void MessagePumpNSApplication::Quit() {
+bool MessagePumpNSApplication::DoQuit() {
   if (!running_own_loop_) {
     [[NSApplication sharedApplication] stop:nil];
-  } else {
-    keep_running_ = false;
   }
 
   // Send a fake event to wake the loop up.
@@ -865,6 +877,7 @@ void MessagePumpNSApplication::Quit() {
                                          data1:0
                                          data2:0]
            atStart:NO];
+  return true;
 }
 
 MessagePumpCrApplication::MessagePumpCrApplication() {

base/message_loop/message_pump_mac_unittest.mm

@@ -270,12 +270,18 @@ TEST(MessagePumpMacTest, DontInvalidateTimerInNativeRunLoop) {
 
   // Post another task to close the menu. The 100ms delay was determined
   // experimentally on a 2013 Mac Pro.
+  RunLoop run_loop;
   ThreadTaskRunnerHandle::Get()->PostDelayedTask(
       FROM_HERE,
-      base::BindOnce([](NSMenu* menu) { [menu cancelTracking]; }, menu),
+      base::BindOnce(
+          [](RunLoop* run_loop, NSMenu* menu) {
+            [menu cancelTracking];
+            run_loop->Quit();
+          },
+          &run_loop, menu),
       base::TimeDelta::FromMilliseconds(100));
 
-  EXPECT_NO_FATAL_FAILURE(RunLoop().RunUntilIdle());
+  EXPECT_NO_FATAL_FAILURE(run_loop.Run());
 }
 
 }  // namespace base

base/message_loop/message_pump_unittest.cc

+// Copyright 2018 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 "base/message_loop/message_pump.h"
+#include "base/message_loop/message_loop.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using ::testing::_;
+using ::testing::Invoke;
+
+namespace base {
+namespace {
+
+class MockMessagePumpDelegate : public MessagePump::Delegate {
+ public:
+  MockMessagePumpDelegate() = default;
+
+  // MessagePump::Delegate:
+  MOCK_METHOD0(DoWork, bool());
+  MOCK_METHOD1(DoDelayedWork, bool(TimeTicks*));
+  MOCK_METHOD0(DoIdleWork, bool());
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockMessagePumpDelegate);
+};
+
+class MessagePumpTest : public ::testing::TestWithParam<MessageLoopBase::Type> {
+ public:
+  MessagePumpTest()
+      : message_pump_(MessageLoop::CreateMessagePumpForType(GetParam())) {}
+
+ protected:
+  std::unique_ptr<MessagePump> message_pump_;
+};
+
+TEST_P(MessagePumpTest, QuitStopsWork) {
+  testing::StrictMock<MockMessagePumpDelegate> delegate;
+
+  // Not expecting any calls to DoDelayedWork or DoIdleWork after quitting.
+  EXPECT_CALL(delegate, DoWork).WillOnce(Invoke([this] {
+    message_pump_->Quit();
+    return false;
+  }));
+  EXPECT_CALL(delegate, DoDelayedWork(_)).Times(0);
+  EXPECT_CALL(delegate, DoIdleWork()).Times(0);
+
+  message_pump_->ScheduleWork();
+  message_pump_->Run(&delegate);
+}
+
+INSTANTIATE_TEST_CASE_P(,
+                        MessagePumpTest,
+                        ::testing::Values(MessageLoop::TYPE_DEFAULT,
+                                          MessageLoop::TYPE_UI,
+                                          MessageLoop::TYPE_IO));
+
+}  // namespace
+}  // namespace base
\ No newline at end of file