[MessageLoop] Lock-free ScheduleWork() scheme

The Lock is causing hangs because of priority inversion
mixed with priority boosting (ScheduleWork() tends to
boost the destination thread which may deschedule the
posting thread; if the posting thread is a background
thread this boost-induded-desched-while-holding-lock
can cause a livelock). See https://crbug.com/890978#c10
for example crashes catching this.

The Lock was only necessary for startup/shutdown and is
being replaced by a lock-free atomic scheme in this CL.

MessagePump::ScheduleWork() itself was already thread-safe
(but the Android impl did unnecessarily check a non-atomic bool)

This adds a WaitableEvent in ~MessageLoop(); hence the requirement
for a wait-allowance in net's EmbeddedTestServer.

TBR=zhongyi@chromium.org (embedded_test_server.cc side-effects)

Bug: 890978, 874237
Change-Id: I0916e5a99035a935b0a23a770af256f334e78c43
Reviewed-on: https://chromium-review.googlesource.com/c/1278631
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601600}

base/message_loop/message_loop.h

@@ -260,8 +260,10 @@ class BASE_EXPORT MessageLoop : public MessagePump::Delegate,
   // destructor to make sure all the task's destructors get called.
   void DeletePendingTasks();
 
-  // Wakes up the message pump. Can be called on any thread. The caller is
-  // responsible for synchronizing ScheduleWork() calls.
+  // Wakes up the message pump. Thread-safe (and callers should avoid holding a
+  // Lock at all cost while making this call as some platforms' priority
+  // boosting features have been observed to cause the caller to get descheduled
+  // : https://crbug.com/890978).
   void ScheduleWork();
 
   // Returns |next_run_time| capped at 1 day from |recent_time_|. This is used

base/message_loop/message_pump.h

@@ -112,9 +112,11 @@ class BASE_EXPORT MessagePump {
   virtual void Quit() = 0;
 
   // Schedule a DoWork callback to happen reasonably soon.  Does nothing if a
-  // DoWork callback is already scheduled.  This method may be called from any
-  // thread.  Once this call is made, DoWork should not be "starved" at least
-  // until it returns a value of false.
+  // DoWork callback is already scheduled. Once this call is made, DoWork should
+  // not be "starved" at least until it returns a value of false. Thread-safe
+  // (and callers should avoid holding a Lock at all cost while making this call
+  // as some platforms' priority boosting features have been observed to cause
+  // the caller to get descheduled : https://crbug.com/890978).
   virtual void ScheduleWork() = 0;
 
   // Schedule a DoDelayedWork callback to happen at the specified time,

base/message_loop/message_pump_android.cc

@@ -278,9 +278,6 @@ void MessagePumpForUI::Quit() {
 }
 
 void MessagePumpForUI::ScheduleWork() {
-  if (ShouldQuit())
-    return;
-
   // Write (add) 1 to the eventfd. This tells the Looper to wake up and call our
   // callback, allowing us to run tasks. This also allows us to detect, when we
   // clear the fd, whether additional work was scheduled after we finished

net/test/embedded_test_server/embedded_test_server.cc

@@ -501,7 +501,7 @@ bool EmbeddedTestServer::PostTaskToIOThreadAndWait(
   // already.
   //
   // To handle this situation, create temporary message loop to support the
-  // PostTaskAndReply operation if the current thread as no message loop.
+  // PostTaskAndReply operation if the current thread has no message loop.
   std::unique_ptr<base::MessageLoop> temporary_loop;
   if (!base::MessageLoopCurrent::Get())
     temporary_loop.reset(new base::MessageLoop());
@@ -513,6 +513,9 @@ bool EmbeddedTestServer::PostTaskToIOThreadAndWait(
   }
   run_loop.Run();
 
+  base::ScopedAllowBaseSyncPrimitivesForTesting allow_wait_for_loop_destruction;
+  temporary_loop.reset();
+
   return true;
 }