mac: Delete the MessagePump timer invalidation hack.

It was previously disabled and is now unused.

Bug: 925998
Change-Id: I504065c37a4436369021f8f20928f3f071b0f143
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2250579Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#779803}

base/message_loop/message_pump_mac.h

@@ -36,9 +36,7 @@
 #include <CoreFoundation/CoreFoundation.h>
 
 #include "base/macros.h"
-#include "base/memory/weak_ptr.h"
 #include "base/message_loop/timer_slack.h"
-#include "base/optional.h"
 #include "build/build_config.h"
 
 #if defined(__OBJC__)
@@ -145,10 +143,6 @@ class BASE_EXPORT MessagePumpCFRunLoopBase : public MessagePump {
   // Get the current mode mask from |enabled_modes_|.
   int GetModeMask() const;
 
-  // Controls whether the timer invalidation performance optimization is
-  // allowed.
-  void SetTimerInvalidationAllowed(bool allowed);
-
  private:
   class ScopedModeEnabler;
 
@@ -159,28 +153,6 @@ class BASE_EXPORT MessagePumpCFRunLoopBase : public MessagePump {
   // avoids querying Now() for key callers.
   void ScheduleDelayedWorkImpl(TimeDelta delta);
 
-  // Returns true if the timer invalidation feature is enabled.
-  static bool IsTimerInvalidationEnabled();
-
-  // 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
-  // the above timer invalidation functions are based, can change from release
-  // to release and should not be accessed directly (this struct last changed at
-  // least in 2008 in CF-476).
-  //
-  // 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 right bit.
-  static bool CanInvalidateCFRunLoopTimers();
-
-  // Sets a Core Foundation object's "invalid" bit to |valid|. Based on code
-  // from CFRunLoop.c.
-  static void ChromeCFRunLoopTimerSetValid(CFRunLoopTimerRef timer, bool valid);
-
-  // Controls the validity of the delayed work timer. Does nothing if timer
-  // invalidation is disallowed.
-  void SetDelayedWorkTimerValid(bool valid);
-
   // Timer callback scheduled by ScheduleDelayedWork.  This does not do any
   // work, but it signals |work_source_| so that delayed work can be performed
   // within the appropriate priority constraints.
@@ -283,14 +255,6 @@ class BASE_EXPORT MessagePumpCFRunLoopBase : public MessagePump {
   bool delegateless_work_;
   bool delegateless_idle_work_;
 
-  // Whether or not timer invalidation can be used in order to reduce the number
-  // of reschedulings.
-  bool allow_timer_invalidation_;
-
-  // If changing timer validitity was attempted while it was disallowed, this
-  // value tracks the desired state of the timer.
-  Optional<bool> pending_timer_validity_;
-
   DISALLOW_COPY_AND_ASSIGN(MessagePumpCFRunLoopBase);
 };
 

base/message_loop/message_pump_mac.mm

@@ -53,48 +53,6 @@ bool g_not_using_cr_app = false;
 
 // The MessagePump controlling [NSApp run].
 MessagePumpNSApplication* g_app_pump;
-
-Feature kMessagePumpTimerInvalidation{"MessagePumpMacTimerInvalidation",
-                                      FEATURE_DISABLED_BY_DEFAULT};
-
-// Various CoreFoundation definitions.
-typedef struct __CFRuntimeBase {
-  uintptr_t _cfisa;
-  uint8_t _cfinfo[4];
-  uint32_t _rc;
-} CFRuntimeBase;
-
-#if defined(__BIG_ENDIAN__)
-#define __CF_BIG_ENDIAN__ 1
-#define __CF_LITTLE_ENDIAN__ 0
-#endif
-
-#if defined(__LITTLE_ENDIAN__)
-#define __CF_LITTLE_ENDIAN__ 1
-#define __CF_BIG_ENDIAN__ 0
-#endif
-
-#define CF_INFO_BITS (!!(__CF_BIG_ENDIAN__)*3)
-
-#define __CFBitfieldMask(N1, N2) \
-  ((((UInt32)~0UL) << (31UL - (N1) + (N2))) >> (31UL - N1))
-#define __CFBitfieldSetValue(V, N1, N2, X)   \
-  ((V) = ((V) & ~__CFBitfieldMask(N1, N2)) | \
-         (((X) << (N2)) & __CFBitfieldMask(N1, N2)))
-
-// Marking timers as invalid at the right time by flipping their valid bit helps
-// significantly reduce power use (see the explanation in
-// RunDelayedWorkTimer()), however there is no public API for doing so.
-// CFRuntime.h states that CFRuntimeBase can change from release to release
-// and should not be accessed directly. The last known change of this struct
-// occurred in 2008 in CF-476 / 10.5; unfortunately the source for 10.11 and
-// 10.12 is not available for inspection at this time.
-// CanInvalidateCFRunLoopTimers() will at least prevent us from invalidating
-// timers if this function starts flipping the wrong bit on a future OS release.
-void __ChromeCFRunLoopTimerSetValid(CFRunLoopTimerRef timer, bool valid) {
-  __CFBitfieldSetValue(((CFRuntimeBase*)timer)->_cfinfo[CF_INFO_BITS], 3, 3,
-                       valid);
-}
 #endif  // !defined(OS_IOS)
 
 }  // namespace
@@ -217,16 +175,6 @@ void MessagePumpCFRunLoopBase::ScheduleDelayedWork(
 }
 
 void MessagePumpCFRunLoopBase::ScheduleDelayedWorkImpl(TimeDelta delta) {
-  // Flip the timer's validation bit just before setting the new fire time. Do
-  // this now because CFRunLoopTimerSetNextFireDate() likely checks the validity
-  // of a timer before proceeding to set its fire date. Making the timer valid
-  // now won't have any side effects (such as a premature firing of the timer)
-  // because we're only flipping a bit.
-  //
-  // Please see the comment in RunDelayedWorkTimer() for more info on the whys
-  // of invalidation.
-  SetDelayedWorkTimerValid(true);
-
   // The tolerance needs to be set before the fire date or it may be ignored.
   if (timer_slack_ == TIMER_SLACK_MAXIMUM) {
     CFRunLoopTimerSetTolerance(delayed_work_timer_, delta.InSecondsF() * 0.5);
@@ -256,8 +204,7 @@ MessagePumpCFRunLoopBase::MessagePumpCFRunLoopBase(int initial_mode_mask)
       deepest_nesting_level_(0),
       keep_running_(true),
       delegateless_work_(false),
-      delegateless_idle_work_(false),
-      allow_timer_invalidation_(true) {
+      delegateless_idle_work_(false) {
   run_loop_ = CFRunLoopGetCurrent();
   CFRetain(run_loop_);
 
@@ -368,105 +315,11 @@ int MessagePumpCFRunLoopBase::GetModeMask() const {
   return mask;
 }
 
-#if !defined(OS_IOS)
-// static
-bool MessagePumpCFRunLoopBase::IsTimerInvalidationEnabled() {
-  return FeatureList::IsEnabled(kMessagePumpTimerInvalidation);
-}
-
-// 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.
-// static
-bool MessagePumpCFRunLoopBase::CanInvalidateCFRunLoopTimers() {
-  if (!IsTimerInvalidationEnabled()) {
-    return false;
-  }
-
-  CFRunLoopTimerContext timer_context = CFRunLoopTimerContext();
-  timer_context.info = nullptr;
-  ScopedCFTypeRef<CFRunLoopTimerRef> test_timer(
-      CFRunLoopTimerCreate(NULL,                // allocator
-                           kCFTimeIntervalMax,  // fire time
-                           kCFTimeIntervalMax,  // interval
-                           0,                   // flags
-                           0,                   // priority
-                           nullptr, &timer_context));
-  // Should be valid from the start.
-  if (!CFRunLoopTimerIsValid(test_timer)) {
-    return false;
-  }
-  // Confirm that the private API can mark the timer invalid.
-  __ChromeCFRunLoopTimerSetValid(test_timer, false);
-  if (CFRunLoopTimerIsValid(test_timer)) {
-    return false;
-  }
-  // Confirm that the private API can mark the timer valid.
-  __ChromeCFRunLoopTimerSetValid(test_timer, true);
-  return CFRunLoopTimerIsValid(test_timer);
-}
-#endif  // !defined(OS_IOS)
-
-// static
-void MessagePumpCFRunLoopBase::ChromeCFRunLoopTimerSetValid(
-    CFRunLoopTimerRef timer,
-    bool valid) {
-#if !defined(OS_IOS)
-  static bool can_invalidate_timers = CanInvalidateCFRunLoopTimers();
-  if (can_invalidate_timers) {
-    __ChromeCFRunLoopTimerSetValid(timer, valid);
-  }
-#endif  // !defined(OS_IOS)
-}
-
-void MessagePumpCFRunLoopBase::SetDelayedWorkTimerValid(bool valid) {
-  if (allow_timer_invalidation_) {
-    ChromeCFRunLoopTimerSetValid(delayed_work_timer_, valid);
-  } else {
-    pending_timer_validity_ = valid;
-  }
-}
-
-void MessagePumpCFRunLoopBase::SetTimerInvalidationAllowed(bool allowed) {
-  if (!allowed)
-    ChromeCFRunLoopTimerSetValid(delayed_work_timer_, true);
-  allow_timer_invalidation_ = allowed;
-  if (allowed && pending_timer_validity_.has_value()) {
-    SetDelayedWorkTimerValid(*pending_timer_validity_);
-    pending_timer_validity_ = nullopt;
-  }
-}
-
 // Called from the run loop.
 // static
 void MessagePumpCFRunLoopBase::RunDelayedWorkTimer(CFRunLoopTimerRef timer,
                                                    void* info) {
   MessagePumpCFRunLoopBase* self = static_cast<MessagePumpCFRunLoopBase*>(info);
-
-  // The message pump's timer needs to fire at changing and unpredictable
-  // intervals. Creating a new timer for each firing time is very expensive, so
-  // the message pump instead uses a repeating timer with a very large repeat
-  // rate. After each firing of the timer, the run loop sets the timer's next
-  // firing time to the distant future, essentially pausing the timer until the
-  // pump sets the next firing time. This is the solution recommended by Apple.
-  //
-  // It turns out, however, that scheduling timers is also quite expensive, and
-  // that every one of the message pump's timer firings incurs two
-  // reschedulings. The first rescheduling occurs in ScheduleDelayedWork(),
-  // which sets the desired next firing time. The second comes after exiting
-  // this method (the timer's callback method), when the run loop sets the
-  // timer's next firing time to far in the future.
-  //
-  // The code in __CFRunLoopDoTimer() inside CFRunLoop.c calls the timer's
-  // callback, confirms that the timer is valid, and then sets its future
-  // firing time based on its repeat frequency. Flipping the valid bit here
-  // causes the __CFRunLoopDoTimer() to skip setting the future firing time.
-  // Note that there's public API to invalidate a timer but it goes beyond
-  // flipping the valid bit, making the timer unusable in the future.
-  //
-  // ScheduleDelayedWork() flips the valid bit back just before setting the
-  // timer's new firing time.
-  self->SetDelayedWorkTimerValid(false);
-
   // The timer fired, assume we have work and let RunWork() figure out what to
   // do and what to schedule after.
   base::mac::CallWithEHFrame(^{
@@ -798,14 +651,11 @@ ScopedPumpMessagesInPrivateModes::ScopedPumpMessagesInPrivateModes() {
   if ([NSApp modalWindow])
     return;
   g_app_pump->SetModeMask(kAllModesMask);
-  // Disable timer invalidation to avoid hangs. See crbug.com/912273.
-  g_app_pump->SetTimerInvalidationAllowed(false);
 }
 
 ScopedPumpMessagesInPrivateModes::~ScopedPumpMessagesInPrivateModes() {
   DCHECK(g_app_pump);
   g_app_pump->SetModeMask(kNSApplicationModalSafeModeMask);
-  g_app_pump->SetTimerInvalidationAllowed(true);
 }
 
 int ScopedPumpMessagesInPrivateModes::GetModeMaskForTest() {

base/message_loop/message_pump_mac_unittest.mm

@@ -29,110 +29,6 @@ constexpr int kNSApplicationModalSafeModeMask = 0x3;
 
 namespace base {
 
-class TestMessagePumpCFRunLoopBase {
- public:
-  bool IsTimerInvalidationEnabled() {
-    return MessagePumpCFRunLoopBase::IsTimerInvalidationEnabled();
-  }
-
-  bool TestCanInvalidateTimers() {
-    return MessagePumpCFRunLoopBase::CanInvalidateCFRunLoopTimers();
-  }
-  static void SetTimerValid(CFRunLoopTimerRef timer, bool valid) {
-    MessagePumpCFRunLoopBase::ChromeCFRunLoopTimerSetValid(timer, valid);
-  }
-
-  static void PerformTimerCallback(CFRunLoopTimerRef timer, void* info) {
-    TestMessagePumpCFRunLoopBase* self =
-        static_cast<TestMessagePumpCFRunLoopBase*>(info);
-    self->timer_callback_called_ = true;
-
-    if (self->invalidate_timer_in_callback_) {
-      SetTimerValid(timer, false);
-    }
-  }
-
-  bool invalidate_timer_in_callback_;
-
-  bool timer_callback_called_;
-};
-
-TEST(MessagePumpMacTest, TestCanInvalidateTimers) {
-  TestMessagePumpCFRunLoopBase message_pump_test;
-  if (!message_pump_test.IsTimerInvalidationEnabled())
-    return;
-
-  // Catch whether or not the use of private API ever starts failing.
-  EXPECT_TRUE(message_pump_test.TestCanInvalidateTimers());
-}
-
-TEST(MessagePumpMacTest, TestInvalidatedTimerReuse) {
-  TestMessagePumpCFRunLoopBase message_pump_test;
-  if (!message_pump_test.IsTimerInvalidationEnabled())
-    return;
-
-  CFRunLoopTimerContext timer_context = CFRunLoopTimerContext();
-  timer_context.info = &message_pump_test;
-  const CFTimeInterval kCFTimeIntervalMax =
-      std::numeric_limits<CFTimeInterval>::max();
-  ScopedCFTypeRef<CFRunLoopTimerRef> test_timer(CFRunLoopTimerCreate(
-      NULL,                // allocator
-      kCFTimeIntervalMax,  // fire time
-      kCFTimeIntervalMax,  // interval
-      0,                   // flags
-      0,                   // priority
-      TestMessagePumpCFRunLoopBase::PerformTimerCallback, &timer_context));
-  CFRunLoopAddTimer(CFRunLoopGetCurrent(), test_timer,
-                    kMessageLoopExclusiveRunLoopMode);
-
-  // Sanity check.
-  EXPECT_TRUE(CFRunLoopTimerIsValid(test_timer));
-
-  // Confirm that the timer fires as expected, and that it's not a one-time-use
-  // timer (those timers are invalidated after they fire).
-  CFAbsoluteTime next_fire_time = CFAbsoluteTimeGetCurrent() + 0.01;
-  CFRunLoopTimerSetNextFireDate(test_timer, next_fire_time);
-  message_pump_test.timer_callback_called_ = false;
-  message_pump_test.invalidate_timer_in_callback_ = false;
-  CFRunLoopRunInMode(kMessageLoopExclusiveRunLoopMode, 0.02, true);
-  EXPECT_TRUE(message_pump_test.timer_callback_called_);
-  EXPECT_TRUE(CFRunLoopTimerIsValid(test_timer));
-
-  // As a repeating timer, the timer should have a new fire date set in the
-  // future.
-  EXPECT_GT(CFRunLoopTimerGetNextFireDate(test_timer), next_fire_time);
-
-  // Try firing the timer, and invalidating it within its callback.
-  next_fire_time = CFAbsoluteTimeGetCurrent() + 0.01;
-  CFRunLoopTimerSetNextFireDate(test_timer, next_fire_time);
-  message_pump_test.timer_callback_called_ = false;
-  message_pump_test.invalidate_timer_in_callback_ = true;
-  CFRunLoopRunInMode(kMessageLoopExclusiveRunLoopMode, 0.02, true);
-  EXPECT_TRUE(message_pump_test.timer_callback_called_);
-  EXPECT_FALSE(CFRunLoopTimerIsValid(test_timer));
-
-  // The CFRunLoop believes the timer is invalid, so it should not have a
-  // fire date.
-  EXPECT_EQ(0, CFRunLoopTimerGetNextFireDate(test_timer));
-
-  // Now mark the timer as valid and confirm that it still fires correctly.
-  TestMessagePumpCFRunLoopBase::SetTimerValid(test_timer, true);
-  EXPECT_TRUE(CFRunLoopTimerIsValid(test_timer));
-  next_fire_time = CFAbsoluteTimeGetCurrent() + 0.01;
-  CFRunLoopTimerSetNextFireDate(test_timer, next_fire_time);
-  message_pump_test.timer_callback_called_ = false;
-  message_pump_test.invalidate_timer_in_callback_ = false;
-  CFRunLoopRunInMode(kMessageLoopExclusiveRunLoopMode, 0.02, true);
-  EXPECT_TRUE(message_pump_test.timer_callback_called_);
-  EXPECT_TRUE(CFRunLoopTimerIsValid(test_timer));
-
-  // Confirm that the run loop again gave it a new fire date in the future.
-  EXPECT_GT(CFRunLoopTimerGetNextFireDate(test_timer), next_fire_time);
-
-  CFRunLoopRemoveTimer(CFRunLoopGetCurrent(), test_timer,
-                       kMessageLoopExclusiveRunLoopMode);
-}
-
 namespace {
 
 // PostedTasks are only executed while the message pump has a delegate. That is,
@@ -223,84 +119,6 @@ TEST(MessagePumpMacTest, ScopedPumpMessagesAttemptWithModalDialog) {
   EXPECT_EQ(NSAlertFirstButtonReturn, result);
 }
 
-// This is a regression test for a scenario where the invalidation of the
-// delayed work timer (using non-public APIs) causes a nested native run loop to
-// hang. The exact root cause of the hang is unknown since it involves the
-// closed-source Core Foundation runtime, but the steps needed to trigger it
-// are:
-//
-//   1. Post a delayed task that will run some time after step #4.
-//   2. Allow Chrome tasks to run in nested run loops (with
-//      ScopedNestableTaskAllower).
-//   3. Allow running Chrome tasks during private run loop modes (with
-//      ScopedPumpMessagesInPrivateModes).
-//   4. Open a pop-up menu via [NSMenu popupContextMenu]. This will start a
-//      private native run loop to process menu interaction.
-//   5. In a posted task, close the menu with [NSMenu cancelTracking].
-//
-// At this point the menu closes visually but the nested run loop (flakily)
-// hangs forever in a live-lock, i.e., Chrome tasks keep executing but the
-// NSMenu call in #4 never returns.
-//
-// The workaround is to avoid timer invalidation during nested native run loops.
-//
-// DANGER: As the pop-up menu captures keyboard input, the bug will make the
-// machine's keyboard inoperable during the live-lock. Use a TTY-based remote
-// terminal such as SSH (as opposed to Chromoting) to investigate the issue.
-//
-TEST(MessagePumpMacTest, DontInvalidateTimerInNativeRunLoop) {
-  test::SingleThreadTaskEnvironment task_environment(
-      test::SingleThreadTaskEnvironment::MainThreadType::UI);
-  NSWindow* window =
-      [[[NSWindow alloc] initWithContentRect:NSMakeRect(0, 0, 100, 100)
-                                   styleMask:NSBorderlessWindowMask
-                                     backing:NSBackingStoreBuffered
-                                       defer:NO] autorelease];
-  NSMenu* menu = [[NSMenu alloc] initWithTitle:@"Test menu"];
-  [menu insertItemWithTitle:@"Dummy item"
-                     action:@selector(dummy)
-              keyEquivalent:@"a"
-                    atIndex:0];
-  NSEvent* event = [NSEvent otherEventWithType:NSApplicationDefined
-                                      location:NSZeroPoint
-                                 modifierFlags:0
-                                     timestamp:0
-                                  windowNumber:0
-                                       context:nil
-                                       subtype:0
-                                         data1:0
-                                         data2:0];
-
-  // Post a task to open the menu. This needs to be a separate task so that
-  // nested task execution can be allowed.
-  ThreadTaskRunnerHandle::Get()->PostTask(
-      FROM_HERE, base::BindOnce(
-                     [](NSWindow* window, NSMenu* menu, NSEvent* event) {
-                       MessageLoopCurrent::ScopedNestableTaskAllower allow;
-                       ScopedPumpMessagesInPrivateModes pump_private;
-                       // When the bug triggers, this call never returns.
-                       [NSMenu popUpContextMenu:menu
-                                      withEvent:event
-                                        forView:[window contentView]];
-                     },
-                     window, menu, event));
-
-  // 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(
-          [](RunLoop* run_loop, NSMenu* menu) {
-            [menu cancelTracking];
-            run_loop->Quit();
-          },
-          &run_loop, menu),
-      base::TimeDelta::FromMilliseconds(100));
-
-  EXPECT_NO_FATAL_FAILURE(run_loop.Run());
-}
-
 TEST(MessagePumpMacTest, QuitWithModalWindow) {
   test::SingleThreadTaskEnvironment task_environment(
       test::SingleThreadTaskEnvironment::MainThreadType::UI);