Revert of Don't peek messages in the MessagePumpForUI class when we receive...

Revert of Don't peek messages in the MessagePumpForUI class when we receive our kMsgHaveWork message. (patchset #13 id:240001 of https://codereview.chromium.org/1156503005/)

Reason for revert:
I suspect this CL breaks Windows 8 content_unittests, as it's listed both in
https://build.chromium.org/p/chromium.fyi/builders/Win8%20Tests%20%281%29/builds/8675 and our WebRTC-specific waterfall: https://build.chromium.org/p/chromium.webrtc/builders/Win8%20Tester/builds/14164

I'll try to reland if it turns out not to solve the breakage.

Original issue's description:
> Don't peek messages in the MessagePumpForUI class when we receive our kMsgHaveWork message.
>
> Currently the MessagePumpForUI class peeks Windows messages when we receive the kMsgHaveWork message in
> our main message loop and in nested modal loops. This is because the posted message starves the message loop
> of input and other lower priority messages. While this is ok for the main message loop our peeking and dispatching
> messages in nested loops is wrong and violates the silent contract where in the nested loop should be the one peeking
> and dispatching messages.
>
> To fix this the approach we are taking is to create a worker thread which uses a waitable timer of 3 ms which posts
> the kMsgHaveWork message to the main loop when the timer fires. As a result we can safely delete all the code
> in the MessagePumpForUI::ScheduleWork function and simplify the ProcessPumpReplacementMessage function.
>
> The MessagePumpForUI::ScheduleWork function now checks the delay for the task at the head of the queue
> If it is 0 then we post the message right away as it is a regular task. Added functions (GetNewlyAddedTaskDelay) to the MessagePump::Delegate class and the IncomingTaskQueue for this.
>
> The other change here is to change the GPU process to use the IO message pump by default and use the UI pump only
> if we are using opengl. Reason being this patch causes a delay in processing tasks due to the worker thread which
> causes tests like webgl_conformance to fail. We will continue working on addressing this over the coming days.
>
> BUG=492837
> R=cpu
>
> Committed: https://crrev.com/b8e126c8b532b1327f38afe2bdf59aa5ff933971
> Cr-Commit-Position: refs/heads/master@{#333572}

TBR=cpu@chromium.org,jar@chromium.org,kbr@chromium.org,geofflang@chromium.org,scottmg@chromium.org,ananta@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=492837

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

Cr-Commit-Position: refs/heads/master@{#333700}

base/message_loop/incoming_task_queue.cc

@@ -119,11 +119,6 @@ void IncomingTaskQueue::StartScheduling() {
     ScheduleWork();
 }
 
-TimeTicks IncomingTaskQueue::GetNewlyAddedTaskDelay() {
-  return !incoming_queue_.empty() ? incoming_queue_.front().delayed_run_time :
-      TimeTicks();
-}
-
 IncomingTaskQueue::~IncomingTaskQueue() {
   // Verify that WillDestroyCurrentMessageLoop() has been called.
   DCHECK(!message_loop_);

base/message_loop/incoming_task_queue.h

@@ -57,9 +57,6 @@ class BASE_EXPORT IncomingTaskQueue
   // scheduling work.
   void StartScheduling();
 
-  // Returns the delay for the most recently added task.
-  TimeTicks GetNewlyAddedTaskDelay();
-
  private:
   friend class RefCountedThreadSafe<IncomingTaskQueue>;
   virtual ~IncomingTaskQueue();

base/message_loop/message_loop.cc

@@ -640,10 +640,6 @@ bool MessageLoop::DoIdleWork() {
   return false;
 }
 
-TimeTicks MessageLoop::GetNewlyAddedTaskDelay() {
-  return incoming_task_queue_->GetNewlyAddedTaskDelay();
-}
-
 void MessageLoop::DeleteSoonInternal(const tracked_objects::Location& from_here,
                                      void(*deleter)(const void*),
                                      const void* object) {

base/message_loop/message_loop.h

@@ -475,7 +475,6 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate {
   bool DoWork() override;
   bool DoDelayedWork(TimeTicks* next_delayed_work_time) override;
   bool DoIdleWork() override;
-  TimeTicks GetNewlyAddedTaskDelay() override;
 
   const Type type_;
 

base/message_loop/message_pump.h

@@ -42,9 +42,6 @@ class BASE_EXPORT MessagePump : public NonThreadSafe {
     // Returns true to indicate that idle work was done. Returning false means
     // the pump will now wait.
     virtual bool DoIdleWork() = 0;
-
-    // Returns the delay for the newly added task.
-    virtual TimeTicks GetNewlyAddedTaskDelay() = 0;
   };
 
   MessagePump();

base/message_loop/message_pump_win.cc

@@ -12,7 +12,6 @@
 #include "base/process/memory.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/strings/stringprintf.h"
-#include "base/threading/thread.h"
 #include "base/trace_event/trace_event.h"
 #include "base/win/wrapped_window_proc.h"
 
@@ -35,10 +34,6 @@ static const wchar_t kWndClassFormat[] = L"Chrome_MessagePumpWindow_%p";
 // task (a series of such messages creates a continuous task pump).
 static const int kMsgHaveWork = WM_USER + 1;
 
-// The default delay for the waitable timer used to wake up the UI worker
-// thread.
-static const int64 kDefaultUIWorkerThreadWakeupTimerMs = 3;
-
 //-----------------------------------------------------------------------------
 // MessagePumpWin public:
 
@@ -95,38 +90,35 @@ int MessagePumpWin::GetCurrentDelay() const {
 MessagePumpForUI::MessagePumpForUI()
     : atom_(0) {
   InitMessageWnd();
-
-  ui_worker_thread_timer_.Set(::CreateWaitableTimer(NULL, FALSE, NULL));
-  ui_worker_thread_.reset(new base::Thread("UI Pump Worker thread"));
-  ui_worker_thread_->Start();
-  ui_worker_thread_->task_runner()->PostTask(
-      FROM_HERE,
-      base::Bind(&MessagePumpForUI::DoWorkerThreadRunLoop,
-                 base::Unretained(this)));
 }
 
 MessagePumpForUI::~MessagePumpForUI() {
   DestroyWindow(message_hwnd_);
   UnregisterClass(MAKEINTATOM(atom_),
                   GetModuleFromAddress(&WndProcThunk));
-
-  ::QueueUserAPC(
-        reinterpret_cast<PAPCFUNC>(&MessagePumpForUI::ShutdownWorkerThread),
-        ui_worker_thread_->thread_handle().platform_handle(), NULL);
-  ui_worker_thread_->Stop();
 }
 
 void MessagePumpForUI::ScheduleWork() {
-  // If we have a regular posted task at the head of queue then we need to
-  // process it quickly.
-  if (state_ && state_->delegate->GetNewlyAddedTaskDelay().is_null()) {
+  if (InterlockedExchange(&have_work_, 1))
+    return;  // Someone else continued the pumping.
+
   // Make sure the MessagePump does some work for us.
-    PostWorkMessage();
-    return;
-  }
-  // Set a one shot timer to fire after 3 milliseconds. The actual resolution
-  // of the timer is dependent on timeBeginPeriod being called.
-  SetWakeupTimer(kDefaultUIWorkerThreadWakeupTimerMs);
+  BOOL ret = PostMessage(message_hwnd_, kMsgHaveWork,
+                         reinterpret_cast<WPARAM>(this), 0);
+  if (ret)
+    return;  // There was room in the Window Message queue.
+
+  // We have failed to insert a have-work message, so there is a chance that we
+  // will starve tasks/timers while sitting in a nested message loop.  Nested
+  // loops only look at Windows Message queues, and don't look at *our* task
+  // queues, etc., so we might not get a time slice in such. :-(
+  // We could abort here, but the fear is that this failure mode is plausibly
+  // common (queue is full, of about 2000 messages), so we'll do a near-graceful
+  // recovery.  Nested loops are pretty transient (we think), so this will
+  // probably be recoverable.
+  InterlockedExchange(&have_work_, 0);  // Clarify that we didn't really insert.
+  UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem", MESSAGE_POST_ERROR,
+                            MESSAGE_LOOP_PROBLEM_MAX);
 }
 
 void MessagePumpForUI::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
@@ -417,65 +409,45 @@ bool MessagePumpForUI::ProcessMessageHelper(const MSG& msg) {
 }
 
 bool MessagePumpForUI::ProcessPumpReplacementMessage() {
-  // Since we discarded a kMsgHaveWork message, we must update the flag.
-  InterlockedExchange(&have_work_, 0);
-  return true;
-}
-
-void MessagePumpForUI::DoWorkerThreadRunLoop() {
-  DCHECK(ui_worker_thread_timer_.Get());
-  while (TRUE) {
-    DWORD ret = WaitForSingleObjectEx(
-        ui_worker_thread_timer_.Get(), INFINITE, TRUE);
-    // The only APC this thread could receive is the Shutdown APC.
-    if (ret == WAIT_IO_COMPLETION)
-      return;
-
-    // Make sure the MessagePump does some work for us.
-    PostWorkMessage();
-
-    // Set a one shot timer to process pending delayed tasks if any in the
-    // queue. The actual resolution of the timer is dependent on the
-    // timeBeginPeriod API being called.
-    SetWakeupTimer(kDefaultUIWorkerThreadWakeupTimerMs);
+  // When we encounter a kMsgHaveWork message, this method is called to peek
+  // and process a replacement message, such as a WM_PAINT or WM_TIMER.  The
+  // goal is to make the kMsgHaveWork as non-intrusive as possible, even though
+  // a continuous stream of such messages are posted.  This method carefully
+  // peeks a message while there is no chance for a kMsgHaveWork to be pending,
+  // then resets the have_work_ flag (allowing a replacement kMsgHaveWork to
+  // possibly be posted), and finally dispatches that peeked replacement.  Note
+  // that the re-post of kMsgHaveWork may be asynchronous to this thread!!
+
+  bool have_message = false;
+  MSG msg;
+  // We should not process all window messages if we are in the context of an
+  // OS modal loop, i.e. in the context of a windows API call like MessageBox.
+  // This is to ensure that these messages are peeked out by the OS modal loop.
+  if (MessageLoop::current()->os_modal_loop()) {
+    // We only peek out WM_PAINT and WM_TIMER here for reasons mentioned above.
+    have_message = PeekMessage(&msg, NULL, WM_PAINT, WM_PAINT, PM_REMOVE) ||
+                   PeekMessage(&msg, NULL, WM_TIMER, WM_TIMER, PM_REMOVE);
+  } else {
+    have_message = PeekMessage(&msg, NULL, 0, 0, PM_REMOVE) != FALSE;
   }
-}
 
-// static
-void CALLBACK MessagePumpForUI::ShutdownWorkerThread(ULONG_PTR param) {
-  // This function is empty because we only use the fact that an APC was posted
-  // to the worker thread to shut it down.
-  return;
-}
+  DCHECK(!have_message || kMsgHaveWork != msg.message ||
+         msg.hwnd != message_hwnd_);
 
-void MessagePumpForUI::PostWorkMessage() {
-  BOOL posted = PostMessage(message_hwnd_, kMsgHaveWork,
-                            reinterpret_cast<WPARAM>(this),
-                            0);
-  if (!posted) {
-    // We have failed to insert a have-work message, so there is a chance
-    // that we will starve tasks/timers while sitting in a nested message
-    // loop. Nested loops only look at Windows Message queues, and don't
-    // look at *our* task queues, etc., so we might not get a time slice in
-    // such. :-(
-    // We could abort here, but the fear is that this failure mode is
-    // plausibly common (queue is full, of about 2000 messages), so we'll
-    // do a near-graceful recovery. Nested loops are pretty transient
-    // (we think), so this will probably be recoverable.
-    UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem",
-                              MESSAGE_POST_ERROR,
-                              MESSAGE_LOOP_PROBLEM_MAX);
-  }
-}
+  // Since we discarded a kMsgHaveWork message, we must update the flag.
+  int old_have_work = InterlockedExchange(&have_work_, 0);
+  DCHECK(old_have_work);
 
-void MessagePumpForUI::SetWakeupTimer(int64 delay_ms) {
-  // Set the timer for the delay passed in. The actual resolution of the
-  // timer is dependent on whether timeBeginPeriod was called.
-  LARGE_INTEGER due_time = {0};
-  due_time.QuadPart = -delay_ms * 10000;
-  BOOL timer_set = ::SetWaitableTimer(ui_worker_thread_timer_.Get(),
-      &due_time, 0, NULL, NULL, FALSE);
-  CHECK(timer_set);
+  // We don't need a special time slice if we didn't have_message to process.
+  if (!have_message)
+    return false;
+
+  // Guarantee we'll get another time slice in the case where we go into native
+  // windows code.   This ScheduleWork() may hurt performance a tiny bit when
+  // tasks appear very infrequently, but when the event queue is busy, the
+  // kMsgHaveWork events get (percentage wise) rarer and rarer.
+  ScheduleWork();
+  return ProcessMessageHelper(msg);
 }
 
 //-----------------------------------------------------------------------------

base/message_loop/message_pump_win.h

@@ -11,7 +11,6 @@
 
 #include "base/base_export.h"
 #include "base/basictypes.h"
-#include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_pump.h"
 #include "base/message_loop/message_pump_dispatcher.h"
 #include "base/observer_list.h"
@@ -20,8 +19,6 @@
 
 namespace base {
 
-class Thread;
-
 // MessagePumpWin serves as the base for specialized versions of the MessagePump
 // for Windows. It provides basic functionality like handling of observers and
 // controlling the lifetime of the message pump.
@@ -138,39 +135,11 @@ class BASE_EXPORT MessagePumpForUI : public MessagePumpWin {
   bool ProcessMessageHelper(const MSG& msg);
   bool ProcessPumpReplacementMessage();
 
-  // We spawn a worker thread to periodically post (3 ms) the kMsgHaveWork
-  // message to the UI message pump. This is to ensure that the main thread
-  // gets to process tasks and delayed tasks when there is no activity in the
-  // Windows message pump or when there is a nested modal loop (sizing/moving/
-  // drag drop/message boxes) etc.
-  void DoWorkerThreadRunLoop();
-
-  // This function is posted as part of a user mode APC to shutdown the worker
-  // thread when the main message pump is shutting down.
-  static void CALLBACK ShutdownWorkerThread(ULONG_PTR param);
-
-  // Helper function for posting the kMsgHaveWork message to wake up the pump
-  // for processing tasks.
-  void PostWorkMessage();
-
-  // Helper function to set the waitable timer used to wake up the UI worker
-  // thread for processing delayed tasks.
-  // |delay_ms| : The delay in milliseconds.
-  void SetWakeupTimer(int64 delay_ms);
-
   // Atom representing the registered window class.
   ATOM atom_;
 
   // A hidden message-only window.
   HWND message_hwnd_;
-
-  // This thread is used to periodically wake up the main thread to process
-  // tasks.
-  scoped_ptr<base::Thread> ui_worker_thread_;
-
-  // The UI worker thread waits on this timer indefinitely. When the main
-  // thread has tasks ready to be processed it sets the timer.
-  base::win::ScopedHandle ui_worker_thread_timer_;
 };
 
 //-----------------------------------------------------------------------------

content/gpu/gpu_main.cc

@@ -160,22 +160,9 @@ int GpuMain(const MainFunctionParams& parameters) {
   bool dead_on_arrival = false;
 
 #if defined(OS_WIN)
-  base::MessageLoop::Type loop_type = base::MessageLoop::TYPE_IO;
   // Use a UI message loop because ANGLE and the desktop GL platform can
   // create child windows to render to.
-  // TODO(ananta) : Recent changes to the UI message pump class on Windows
-  // will cause delays in tasks getting processed in the GPU process which
-  // will show up on the bots in webgl conformance tests, perf tests etc.
-  // It will be great if we can work around the issues with desktop GL and
-  // avoid creating a child window in the GPU process which requires a UI
-  // message pump.
-  if ((command_line.HasSwitch(switches::kUseGL) &&
-       command_line.GetSwitchValueASCII(switches::kUseGL) == "desktop") ||
-      (command_line.HasSwitch(switches::kUseANGLE) &&
-       command_line.GetSwitchValueASCII(switches::kUseANGLE) == "gl")) {
-    loop_type = base::MessageLoop::TYPE_UI;
-  }
-  base::MessageLoop main_message_loop(loop_type);
+  base::MessageLoop main_message_loop(base::MessageLoop::TYPE_UI);
 #elif defined(OS_LINUX) && defined(USE_X11)
   // We need a UI loop so that we can grab the Expose events. See GLSurfaceGLX
   // and https://crbug.com/326995.