Made MessagePump a non-thread safe class.

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
new file mode 100644
index 0000000000000000000000000000000000000000..db99d8750c6932b558ba4d7684c222de28f6f044
--- /dev/null
+++ b/base/message_loop/incoming_task_queue.cc
@@ -0,0 +1,169 @@
+// 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 "base/debug/trace_event.h"
+#include "base/location.h"
+#include "base/message_loop/message_loop.h"
+#include "base/synchronization/waitable_event.h"
+
+namespace base {
+namespace internal {
+
+IncomingTaskQueue::IncomingTaskQueue(MessageLoop* message_loop)
+    : message_loop_(message_loop),
+      next_sequence_num_(0) {
+}
+
+bool IncomingTaskQueue::AddToIncomingQueue(
+    const tracked_objects::Location& from_here,
+    const Closure& task,
+    TimeDelta delay,
+    bool nestable) {
+  AutoLock locked(incoming_queue_lock_);

[jar1, 2013/07/29 23:30:50]

Calling to acquire the lock here is IMO much too early, and should instigate
contention on this lock (which should have a negative impact on perf).

This lock is *only* intended to protect incomming_queue_, so we should only be
manipulating that queue while holding the lock... and to the extent possible, do
nothing more.  Calls that might do allocations, or operating system calls,
should probably be pulled outside if at all possible.

[alexeypa (please no reviews), 2013/07/30 00:31:13]

Before I made this change, there were two locks. One was taken while posting
messages via MessageLoop. The duration of the lock was the same as
|incoming_queue_lock_| except that CalculateDelayedRuntime() and
pump_->ShceduleWork() were not under the lock.

Both locks were taken if a task was posted via MessageLoopProxy. The outer lock
was taken before CalculateDelayedRuntime() was called and released after
pump_->ShceduleWork().
 
I agree, it is a great point. We should be careful about lifetimes of
MessageLoop and MessagePump though. It makes sense to use a separate lock to
protect ScheduleWork(). The problem is where to put it. MessageLoop can be
destroyed at the same time as another thread is trying to wake up the message
pump.

+  PendingTask pending_task(
+      from_here, task, CalculateDelayedRuntime(delay), nestable);

[jar1, 2013/07/29 23:30:50]

The rule with locks is to try to protect data, and not to protect code.  In this
case, I *think* the lock was held to try to make the call to
CalculateDelayedRuntime() safe.  I suspect the intent was to avoid the races by
the posting entity in the update of the member variables that involve the
HighResTimer.  This was historically a problem, but I don't think this approach
is the right way to deal with it.

JamesR has some suggestions for how to move around how/when we do the adjust the
high res timer... and I think we can get away without having to do the locking
here.

+  return PostPendingTask(&pending_task);

[jar1, 2013/07/29 23:30:50]

Here again we're calling to do work while holding a lock... which is very
hazardous.

Often a good pattern (if you ever needed to do this) is to postfix the name with
the word "Locked" to  make sure all callers know that you are holding a lock. 
That said... I think it would be much better to not hold the lock... as I think
you have troubles in the function (see below).

+}
+
+bool IncomingTaskQueue::TryAddToIncomingQueue(
+    const tracked_objects::Location& from_here,
+    const Closure& task) {
+  if (!incoming_queue_lock_.Try()) {
+    // Reset |task|.
+    Closure local_task = task;
+    return false;
+  }
+
+  AutoLock locked(incoming_queue_lock_, AutoLock::AlreadyAcquired());
+  PendingTask pending_task(
+      from_here, task, CalculateDelayedRuntime(TimeDelta()), true);
+  return PostPendingTask(&pending_task);
+}
+
+bool IncomingTaskQueue::IsHighResolutionTimerEnabledForTesting() {
+#if defined(OS_WIN)
+  return !high_resolution_timer_expiration_.is_null();
+#else
+  return true;
+#endif
+}
+
+bool IncomingTaskQueue::IsIdleForTesting() {
+  AutoLock lock(incoming_queue_lock_);
+  return incoming_queue_.empty();
+}
+
+void IncomingTaskQueue::LockWaitUnLockForTesting(WaitableEvent* caller_wait,
+                                                 WaitableEvent* caller_signal) {
+  AutoLock lock(incoming_queue_lock_);
+  caller_wait->Signal();
+  caller_signal->Wait();
+}
+
+void IncomingTaskQueue::ReloadWorkQueue(TaskQueue* work_queue) {
+  // Make sure no tasks are lost.
+  DCHECK(work_queue->empty());
+
+  // Acquire all we can from the inter-thread queue with one lock acquisition.
+  AutoLock lock(incoming_queue_lock_);
+  if (!incoming_queue_.empty())
+    incoming_queue_.Swap(work_queue);  // Constant time
+
+  DCHECK(incoming_queue_.empty());
+}
+
+void IncomingTaskQueue::WillDestroyCurrentMessageLoop() {
+#if defined(OS_WIN)
+  // If we left the high-resolution timer activated, deactivate it now.
+  // Doing this is not-critical, it is mainly to make sure we track
+  // the high resolution timer activations properly in our unit tests.
+  if (!high_resolution_timer_expiration_.is_null()) {
+    Time::ActivateHighResolutionTimer(false);
+    high_resolution_timer_expiration_ = TimeTicks();
+  }
+#endif
+
+  AutoLock lock(incoming_queue_lock_);
+  message_loop_ = NULL;
+}
+
+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;
+
+#if defined(OS_WIN)
+    if (high_resolution_timer_expiration_.is_null()) {
+      // Windows timers are granular to 15.6ms.  If we only set high-res
+      // timers for those under 15.6ms, then a 18ms timer ticks at ~32ms,
+      // which as a percentage is pretty inaccurate.  So enable high
+      // res timers for any timer which is within 2x of the granularity.
+      // This is a tradeoff between accuracy and power management.
+      bool needs_high_res_timers = delay.InMilliseconds() <
+          (2 * Time::kMinLowResolutionThresholdMs);
+      if (needs_high_res_timers) {
+        if (Time::ActivateHighResolutionTimer(true)) {
+          high_resolution_timer_expiration_ = TimeTicks::Now() +
+              TimeDelta::FromMilliseconds(
+                  MessageLoop::kHighResolutionTimerModeLeaseTimeMs);
+        }
+      }
+    }
+#endif
+  } else {
+    DCHECK_EQ(delay.InMilliseconds(), 0) << "delay should not be negative";
+  }
+
+#if defined(OS_WIN)
+  if (!high_resolution_timer_expiration_.is_null()) {
+    if (TimeTicks::Now() > high_resolution_timer_expiration_) {
+      Time::ActivateHighResolutionTimer(false);
+      high_resolution_timer_expiration_ = TimeTicks();
+    }
+  }
+#endif
+
+  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();

[jar1, 2013/07/29 23:30:50]

It is good that you had this.... but if this stays, the code would be much more
readable if the method had the postfix "Locked."

+
+  if (!message_loop_) {
+    pending_task->task.Reset();
+    return false;
+  }
+
+  // Initialize the sequence number. The sequence number is used for delayed
+  // tasks (to faciliate 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_++;
+
+  TRACE_EVENT_FLOW_BEGIN0("task", "MessageLoop::PostTask",
+      TRACE_ID_MANGLE(message_loop_->GetTaskTraceID(*pending_task)));
+
+  bool was_empty = incoming_queue_.empty();
+  incoming_queue_.push(*pending_task);
+  pending_task->task.Reset();
+
+  // Wake up the pump.
+  message_loop_->ScheduleWork(was_empty);

[jar1, 2013/07/29 23:30:50]

This is where things really get dicey.  The lock was really only intended to
protect the queue.... but now it is being held while makeing a call to
ScheduleWork()... which does not have a postfix... and hence it is not clear to
the reviewer that a lock is held, and they work done should be very quickly
finished.

[alexeypa (please no reviews), 2013/07/30 00:31:13]

This is new code, so not really. The lock was intended to protect ScheduleWork()
and message pump's lifetime as well. Now I agree that is not very good from
performance standpoint. 
This is documented next to MessagePump::ScheduleWork() declaration. 

I think it should be possible to have a separate lock protecting MessageLoop's
registration with IncomingTaskQueue. It will synchronize ScheduleWork() calls
and message pump destruction as well.

+
+  return true;
+}
+
+}  // namespace internal
+}  // namespace base