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

base/message_loop/incoming_task_queue.cc

 // Copyright 2013 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/incoming_task_queue.h"
 
 #include <limits>
 
 #include "base/location.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 19 matching lines @@
 #if defined(OS_ANDROID)
   // The Android UI message loop needs to get notified each time a task is
   // added
   // to the incoming queue.
   return type == MessageLoop::TYPE_UI || type == MessageLoop::TYPE_JAVA;
 #else
   return false;
 #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)
     : high_res_task_count_(0),
       message_loop_(message_loop),
       next_sequence_num_(0),
       message_loop_scheduled_(false),
       always_schedule_work_(AlwaysNotifyPump(message_loop_->type())),
       is_ready_for_scheduling_(false) {
 }
 
 bool IncomingTaskQueue::AddToIncomingQueue(
     const tracked_objects::Location& from_here,
     const Closure& task,
     TimeDelta delay,
     bool nestable) {
   DLOG_IF(WARNING,
           delay.InSeconds() > kTaskDelayWarningThresholdInSeconds)
       << "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)
   // We consider the task needs a high resolution timer if the delay is
   // more than 0 and less than 32ms. This caps the relative error to
   // less than 50% : a 33ms wait can wake at 48ms since the default
   // 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
   return PostPendingTask(&pending_task);
 }
 
 bool IncomingTaskQueue::HasHighResolutionTasks() {
   AutoLock lock(incoming_queue_lock_);
   return high_res_task_count_ > 0;
 }
@@ skipping 17 matching lines @@
   } else {
     incoming_queue_.Swap(work_queue);
   }
   // Reset the count of high resolution tasks since our queue is now empty.
   int high_res_tasks = high_res_task_count_;
   high_res_task_count_ = 0;
   return high_res_tasks;
 }
 
 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() {
   // Verify that WillDestroyCurrentMessageLoop() has been called.
   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);
+#if defined(OS_WIN)
+    if (pending_task->is_high_res)
+      ++high_res_task_count_;
+#endif
 
-  bool was_empty = incoming_queue_.empty();
-  incoming_queue_.push(*pending_task);
-  pending_task->task.Reset();
+    // 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_++;
 
-  if (is_ready_for_scheduling_ &&
-      (always_schedule_work_ || (!message_loop_scheduled_ && was_empty))) {
-    ScheduleWork();
+    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;
+    }
   }
 
+  // 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();
+
   return true;
 }
 
-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;
-}
-
 }  // namespace internal
 }  // namespace base