Avoid holding |incoming_queue_lock_| while waking up the message loop.

base/message_loop/incoming_task_queue.cc

Index: base/message_loop/incoming_task_queue.cc
diff --git a/base/message_loop/incoming_task_queue.cc b/base/message_loop/incoming_task_queue.cc
index ebecc1ba54ce35df2cfef606eca13c8ed4062569..2020cd4da61da1e0d1e36829392e225469e8ed07 100644
--- a/base/message_loop/incoming_task_queue.cc
+++ b/base/message_loop/incoming_task_queue.cc
@@ -37,6 +37,16 @@ bool AlwaysNotifyPump(MessageLoop::Type type) {
 #endif
 }
 
+// Calculates the time at which a PendingTask should run.
+TimeTicks CalculateDelayedRuntime(TimeDelta delay) {
+  TimeTicks delayed_run_time;
+  if (delay > TimeDelta())
+    delayed_run_time = TimeTicks::Now() + delay;
+  else
+    DCHECK_EQ(delay.InMilliseconds(), 0) << "delay should not be negative";
+  return delayed_run_time;
+}
+
 }  // namespace
 
 IncomingTaskQueue::IncomingTaskQueue(MessageLoop* message_loop)
@@ -58,7 +68,6 @@ bool IncomingTaskQueue::AddToIncomingQueue(
       << "Requesting super-long task delay period of " << delay.InSeconds()
       << " seconds from here: " << from_here.ToString();
 
-  AutoLock locked(incoming_queue_lock_);
   PendingTask pending_task(
       from_here, task, CalculateDelayedRuntime(delay), nestable);
 #if defined(OS_WIN)
@@ -68,7 +77,6 @@ bool IncomingTaskQueue::AddToIncomingQueue(
   // resolution on Windows is between 10 and 15ms.
   if (delay > TimeDelta() &&
       delay.InMilliseconds() < (2 * Time::kMinLowResolutionThresholdMs)) {
-    ++high_res_task_count_;
     pending_task.is_high_res = true;
   }
 #endif
@@ -106,17 +114,24 @@ int IncomingTaskQueue::ReloadWorkQueue(TaskQueue* work_queue) {
 }
 
 void IncomingTaskQueue::WillDestroyCurrentMessageLoop() {
-  AutoLock lock(incoming_queue_lock_);
+  AutoWriteLock lock(message_loop_lock_);
   message_loop_ = NULL;
 }
 
 void IncomingTaskQueue::StartScheduling() {
-  AutoLock lock(incoming_queue_lock_);
-  DCHECK(!is_ready_for_scheduling_);
-  DCHECK(!message_loop_scheduled_);
-  is_ready_for_scheduling_ = true;
-  if (!incoming_queue_.empty())
-    ScheduleWork();
+  bool schedule_work;
+  {
+    AutoLock lock(incoming_queue_lock_);
+    DCHECK(!is_ready_for_scheduling_);
+    DCHECK(!message_loop_scheduled_);
+    is_ready_for_scheduling_ = true;
+    schedule_work = !incoming_queue_.empty();
+  }
+  if (schedule_work) {
+    AutoReadLock lock(message_loop_lock_);

[danakj, 2016/05/19 22:29:37]

nit: name "hold_message_loop"

[Anand Mistry (off Chromium), 2016/05/20 04:45:38]

Removed as per comment below.

+    DCHECK(message_loop_);

[danakj, 2016/05/19 22:29:37]

This method is called from the MessageLoop which is what lets you DCHECK this
right? So why bother holding the read lock at all? It can't go away inside here.
You could just document this fact?

[Anand Mistry (off Chromium), 2016/05/20 04:45:38]

Done. Thinking about it a bit more, calling ScheduleWork() here is completely
unnecessary because this function is called before the MessageLoop starts
running, and MessageLoop checks for tasks before sleeping. But I'm keeping it,
to be on the safe side.

+    message_loop_->ScheduleWork();

[danakj, 2016/05/19 22:29:37]

How come this doesn't set message_loop_scheduled_ = true anymore?

[Anand Mistry (off Chromium), 2016/05/20 04:45:38]

It's not necessary. The worst that can happen is a spurious wake-up on startup.

+  }
 }
 
 IncomingTaskQueue::~IncomingTaskQueue() {
@@ -124,58 +139,60 @@ IncomingTaskQueue::~IncomingTaskQueue() {
   DCHECK(!message_loop_);
 }
 
-TimeTicks IncomingTaskQueue::CalculateDelayedRuntime(TimeDelta delay) {
-  TimeTicks delayed_run_time;
-  if (delay > TimeDelta())
-    delayed_run_time = TimeTicks::Now() + delay;
-  else
-    DCHECK_EQ(delay.InMilliseconds(), 0) << "delay should not be negative";
-  return delayed_run_time;
-}
-
 bool IncomingTaskQueue::PostPendingTask(PendingTask* pending_task) {
   // Warning: Don't try to short-circuit, and handle this thread's tasks more
   // directly, as it could starve handling of foreign threads.  Put every task
   // into this queue.
 
-  // This should only be called while the lock is taken.
-  incoming_queue_lock_.AssertAcquired();
+  // Ensures |message_loop_| isn't null'd while running.

[danakj, 2016/05/19 22:29:37]

"nulled". tho it's more like ensures the message loop isn't destroyed right? cuz
once it's null here it will also no longer exist.

[Anand Mistry (off Chromium), 2016/05/20 04:45:37]

yeha, changed to destroyed. The nulling happens in MessageLoop's destructor, so
they're essentially the same.

+  AutoReadLock ml_lock(message_loop_lock_);

[danakj, 2016/05/19 22:29:37]

nit: name this variable "hold_message_loop"

[Anand Mistry (off Chromium), 2016/05/20 04:45:38]

Done.

 
   if (!message_loop_) {
     pending_task->task.Reset();
     return false;
   }
 
-  // Initialize the sequence number. The sequence number is used for delayed
-  // tasks (to facilitate FIFO sorting when two tasks have the same
-  // delayed_run_time value) and for identifying the task in about:tracing.
-  pending_task->sequence_num = next_sequence_num_++;
+  bool schedule_work = false;
+  {
+    AutoLock locked(incoming_queue_lock_);

[danakj, 2016/05/19 22:29:37]

nit: name this variable "hold"

[Anand Mistry (off Chromium), 2016/05/20 04:45:37]

Done.

 
-  message_loop_->task_annotator()->DidQueueTask("MessageLoop::PostTask",
-                                                *pending_task);
-
-  bool was_empty = incoming_queue_.empty();
-  incoming_queue_.push(*pending_task);
-  pending_task->task.Reset();
+#if defined(OS_WIN)
+    if (pending_task->is_high_res)
+      ++high_res_task_count_;
+#endif
 
-  if (is_ready_for_scheduling_ &&
-      (always_schedule_work_ || (!message_loop_scheduled_ && was_empty))) {
-    ScheduleWork();
+    // Initialize the sequence number. The sequence number is used for delayed
+    // tasks (to facilitate FIFO sorting when two tasks have the same
+    // delayed_run_time value) and for identifying the task in about:tracing.
+    pending_task->sequence_num = next_sequence_num_++;
+
+    message_loop_->task_annotator()->DidQueueTask("MessageLoop::PostTask",
+                                                  *pending_task);
+
+    bool was_empty = incoming_queue_.empty();
+    incoming_queue_.push(std::move(*pending_task));
+
+    if (is_ready_for_scheduling_ &&
+        (always_schedule_work_ || (!message_loop_scheduled_ && was_empty))) {
+      schedule_work = true;
+      // After we've scheduled the message loop, we do not need to do so again
+      // until we know it has processed all of the work in our queue and is
+      // waiting for more work again. The message loop will always attempt to
+      // reload from the incoming queue before waiting again so we clear this
+      // flag in ReloadWorkQueue().
+      message_loop_scheduled_ = true;
+    }
   }
 
-  return true;
-}
+  // Wake up the message loop and schedule work. This is done outside
+  // |incoming_queue_lock_| because signalling the message loop may cause this
+  // thread to be switched. If |incoming_queue_lock_| is held, any other thread
+  // that wants to post a task will be blocked until this thread switches back
+  // in and releases |incoming_queue_lock_|.
+  if (schedule_work)
+    message_loop_->ScheduleWork();
 
-void IncomingTaskQueue::ScheduleWork() {
-  DCHECK(is_ready_for_scheduling_);
-  // Wake up the message loop.
-  message_loop_->ScheduleWork();
-  // After we've scheduled the message loop, we do not need to do so again
-  // until we know it has processed all of the work in our queue and is
-  // waiting for more work again. The message loop will always attempt to
-  // reload from the incoming queue before waiting again so we clear this flag
-  // in ReloadWorkQueue().
-  message_loop_scheduled_ = true;
+  return true;
 }
 
 }  // namespace internal