Pass Callback to TaskRunner by value and consume it on invocation

This is a preparation CL for http://crrev.com/2637843002, which replaces
the Callback parameter of TaskRunner::PostTask with OnceCallback.
This one replaces the passed-by-const-ref Callback parameter of
TaskRunner::PostTask() with pass-by-value.

With the pass-by-const-ref manner as the old code does, we can't avoid
leaving a reference to the callback object on the original thread. That
is, the callback object may be destroyed either on the target thread or
the original thread. That's problematic when a non-thread-safe object is
bound to the callback.

Pass-by-value and move() in this CL mitigate the nondeterminism: if the
caller of TaskRunner::PostTask() passes the callback object as rvalue,
TaskRunner::PostTask() leaves no reference on the original thread.
I.e. the reference is not left if the callback is passed directly from
Bind(), or passed with std::move() as below.

  task_runner->PostTask(FROM_HERE, base::Bind(&Foo));

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, std::move(cb));

Otherwise, if the caller passes the callback as lvalue, a reference to
the callback is left on the original thread as we do in the previous code.
I.e. a reference is left if the callback is passed from other non-temporary
variable.

  base::Closure cb = base::Bind(&Foo);
  task_runner->PostTask(FROM_HERE, cb);

This is less controversial part of http://crrev.com/2637843002. This CL
is mainly to land it incrementally.

TBR=shrike@chromium.org
BUG=704027
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2726523002
Cr-Commit-Position: refs/heads/master@{#460288}

base/critical_closure.h

@@ -5,6 +5,8 @@
 #ifndef BASE_CRITICAL_CLOSURE_H_
 #define BASE_CRITICAL_CLOSURE_H_
 
+#include <utility>
+
 #include "base/callback.h"
 #include "base/macros.h"
 #include "build/build_config.h"
@@ -27,7 +29,7 @@ bool IsMultiTaskingSupported();
 // |ios::ScopedCriticalAction|.
 class CriticalClosure {
  public:
-  explicit CriticalClosure(const Closure& closure);
+  explicit CriticalClosure(Closure closure);
   ~CriticalClosure();
   void Run();
 
@@ -55,13 +57,13 @@ class CriticalClosure {
 // background running time, |MakeCriticalClosure| should be applied on them
 // before posting.
 #if defined(OS_IOS)
-inline Closure MakeCriticalClosure(const Closure& closure) {
+inline Closure MakeCriticalClosure(Closure closure) {
   DCHECK(internal::IsMultiTaskingSupported());
   return base::Bind(&internal::CriticalClosure::Run,
-                    Owned(new internal::CriticalClosure(closure)));
+                    Owned(new internal::CriticalClosure(std::move(closure))));
 }
 #else  // defined(OS_IOS)
-inline Closure MakeCriticalClosure(const Closure& closure) {
+inline Closure MakeCriticalClosure(Closure closure) {
   // No-op for platforms where the application does not need to acquire
   // background time for closures to finish when it goes into the background.
   return closure;

base/critical_closure_internal_ios.mm

@@ -13,7 +13,8 @@ bool IsMultiTaskingSupported() {
   return [[UIDevice currentDevice] isMultitaskingSupported];
 }
 
-CriticalClosure::CriticalClosure(const Closure& closure) : closure_(closure) {}
+CriticalClosure::CriticalClosure(Closure closure)
+    : closure_(std::move(closure)) {}
 
 CriticalClosure::~CriticalClosure() {}
 

base/deferred_sequenced_task_runner.cc

@@ -4,6 +4,8 @@
 
 #include "base/deferred_sequenced_task_runner.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/logging.h"
 
@@ -13,12 +15,16 @@ DeferredSequencedTaskRunner::DeferredTask::DeferredTask()
     : is_non_nestable(false) {
 }
 
-DeferredSequencedTaskRunner::DeferredTask::DeferredTask(
-    const DeferredTask& other) = default;
+DeferredSequencedTaskRunner::DeferredTask::DeferredTask(DeferredTask&& other) =
+    default;
 
 DeferredSequencedTaskRunner::DeferredTask::~DeferredTask() {
 }
 
+DeferredSequencedTaskRunner::DeferredTask&
+DeferredSequencedTaskRunner::DeferredTask::operator=(DeferredTask&& other) =
+    default;
+
 DeferredSequencedTaskRunner::DeferredSequencedTaskRunner(
     scoped_refptr<SequencedTaskRunner> target_task_runner)
     : started_(false), target_task_runner_(std::move(target_task_runner)) {}
@@ -28,15 +34,17 @@ DeferredSequencedTaskRunner::~DeferredSequencedTaskRunner() {
 
 bool DeferredSequencedTaskRunner::PostDelayedTask(
     const tracked_objects::Location& from_here,
-    const Closure& task,
+    Closure task,
     TimeDelta delay) {
   AutoLock lock(lock_);
   if (started_) {
     DCHECK(deferred_tasks_queue_.empty());
-    return target_task_runner_->PostDelayedTask(from_here, task, delay);
+    return target_task_runner_->PostDelayedTask(from_here, std::move(task),
+                                                delay);
   }
 
-  QueueDeferredTask(from_here, task, delay, false /* is_non_nestable */);
+  QueueDeferredTask(from_here, std::move(task), delay,
+                    false /* is_non_nestable */);
   return true;
 }
 
@@ -46,54 +54,47 @@ bool DeferredSequencedTaskRunner::RunsTasksOnCurrentThread() const {
 
 bool DeferredSequencedTaskRunner::PostNonNestableDelayedTask(
     const tracked_objects::Location& from_here,
-    const Closure& task,
+    Closure task,
     TimeDelta delay) {
   AutoLock lock(lock_);
   if (started_) {
     DCHECK(deferred_tasks_queue_.empty());
-    return target_task_runner_->PostNonNestableDelayedTask(from_here,
-                                                           task,
-                                                           delay);
+    return target_task_runner_->PostNonNestableDelayedTask(
+        from_here, std::move(task), delay);
   }
-  QueueDeferredTask(from_here, task, delay, true /* is_non_nestable */);
+  QueueDeferredTask(from_here, std::move(task), delay,
+                    true /* is_non_nestable */);
   return true;
 }
 
 void DeferredSequencedTaskRunner::QueueDeferredTask(
     const tracked_objects::Location& from_here,
-    const Closure& task,
+    Closure task,
     TimeDelta delay,
     bool is_non_nestable) {
   DeferredTask deferred_task;
   deferred_task.posted_from = from_here;
-  deferred_task.task = task;
+  deferred_task.task = std::move(task);
   deferred_task.delay = delay;
   deferred_task.is_non_nestable = is_non_nestable;
-  deferred_tasks_queue_.push_back(deferred_task);
+  deferred_tasks_queue_.push_back(std::move(deferred_task));
 }
 
-
 void DeferredSequencedTaskRunner::Start() {
   AutoLock lock(lock_);
   DCHECK(!started_);
   started_ = true;
   for (std::vector<DeferredTask>::iterator i = deferred_tasks_queue_.begin();
       i != deferred_tasks_queue_.end();
       ++i) {
-    const DeferredTask& task = *i;
+    DeferredTask& task = *i;
     if (task.is_non_nestable) {
-      target_task_runner_->PostNonNestableDelayedTask(task.posted_from,
-                                                      task.task,
-                                                      task.delay);
+      target_task_runner_->PostNonNestableDelayedTask(
+          task.posted_from, std::move(task.task), task.delay);
     } else {
       target_task_runner_->PostDelayedTask(task.posted_from,
-                                           task.task,
-                                           task.delay);
+                                           std::move(task.task), task.delay);
     }
-    // Replace the i-th element in the |deferred_tasks_queue_| with an empty
-    // |DelayedTask| to ensure that |task| is destroyed before the next task
-    // is posted.
-    *i = DeferredTask();
   }
   deferred_tasks_queue_.clear();
 }

base/deferred_sequenced_task_runner.h

@@ -28,13 +28,13 @@ class BASE_EXPORT DeferredSequencedTaskRunner : public SequencedTaskRunner {
 
   // TaskRunner implementation
   bool PostDelayedTask(const tracked_objects::Location& from_here,
-                       const Closure& task,
+                       Closure task,
                        TimeDelta delay) override;
   bool RunsTasksOnCurrentThread() const override;
 
   // SequencedTaskRunner implementation
   bool PostNonNestableDelayedTask(const tracked_objects::Location& from_here,
-                                  const Closure& task,
+                                  Closure task,
                                   TimeDelta delay) override;
 
   // Start the execution - posts all queued tasks to the target executor. The
@@ -46,8 +46,9 @@ class BASE_EXPORT DeferredSequencedTaskRunner : public SequencedTaskRunner {
  private:
   struct DeferredTask  {
     DeferredTask();
-    DeferredTask(const DeferredTask& other);
+    DeferredTask(DeferredTask&& other);
     ~DeferredTask();
+    DeferredTask& operator=(DeferredTask&& other);
 
     tracked_objects::Location posted_from;
     Closure task;
@@ -60,7 +61,7 @@ class BASE_EXPORT DeferredSequencedTaskRunner : public SequencedTaskRunner {
 
   // Creates a |Task| object and adds it to |deferred_tasks_queue_|.
   void QueueDeferredTask(const tracked_objects::Location& from_here,
-                         const Closure& task,
+                         Closure task,
                          TimeDelta delay,
                          bool is_non_nestable);
 

base/message_loop/incoming_task_queue.cc

@@ -5,6 +5,7 @@
 #include "base/message_loop/incoming_task_queue.h"
 
 #include <limits>
+#include <utility>
 
 #include "base/location.h"
 #include "base/message_loop/message_loop.h"
@@ -59,16 +60,16 @@ IncomingTaskQueue::IncomingTaskQueue(MessageLoop* message_loop)
 
 bool IncomingTaskQueue::AddToIncomingQueue(
     const tracked_objects::Location& from_here,
-    const Closure& task,
+    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();
 
-  PendingTask pending_task(from_here, task, CalculateDelayedRuntime(delay),
-                           nestable);
+  PendingTask pending_task(from_here, std::move(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

base/message_loop/incoming_task_queue.h

@@ -6,6 +6,7 @@
 #define BASE_MESSAGE_LOOP_INCOMING_TASK_QUEUE_H_
 
 #include "base/base_export.h"
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/pending_task.h"
@@ -35,7 +36,7 @@ class BASE_EXPORT IncomingTaskQueue
   // returns false. In all cases, the ownership of |task| is transferred to the
   // called method.
   bool AddToIncomingQueue(const tracked_objects::Location& from_here,
-                          const Closure& task,
+                          Closure task,
                           TimeDelta delay,
                           bool nestable);
 

base/message_loop/message_loop_task_runner.cc

@@ -4,6 +4,8 @@
 
 #include "base/message_loop/message_loop_task_runner.h"
 
+#include <utility>
+
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/message_loop/incoming_task_queue.h"
@@ -24,18 +26,20 @@ void MessageLoopTaskRunner::BindToCurrentThread() {
 
 bool MessageLoopTaskRunner::PostDelayedTask(
     const tracked_objects::Location& from_here,
-    const base::Closure& task,
+    Closure task,
     base::TimeDelta delay) {
   DCHECK(!task.is_null()) << from_here.ToString();
-  return incoming_queue_->AddToIncomingQueue(from_here, task, delay, true);
+  return incoming_queue_->AddToIncomingQueue(from_here, std::move(task), delay,
+                                             true);
 }
 
 bool MessageLoopTaskRunner::PostNonNestableDelayedTask(
     const tracked_objects::Location& from_here,
-    const base::Closure& task,
+    Closure task,
     base::TimeDelta delay) {
   DCHECK(!task.is_null()) << from_here.ToString();
-  return incoming_queue_->AddToIncomingQueue(from_here, task, delay, false);
+  return incoming_queue_->AddToIncomingQueue(from_here, std::move(task), delay,
+                                             false);
 }
 
 bool MessageLoopTaskRunner::RunsTasksOnCurrentThread() const {

base/message_loop/message_loop_task_runner.h

@@ -6,6 +6,7 @@
 #define BASE_MESSAGE_LOOP_MESSAGE_LOOP_TASK_RUNNER_H_
 
 #include "base/base_export.h"
+#include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/pending_task.h"
@@ -31,10 +32,10 @@ class BASE_EXPORT MessageLoopTaskRunner : public SingleThreadTaskRunner {
 
   // SingleThreadTaskRunner implementation
   bool PostDelayedTask(const tracked_objects::Location& from_here,
-                       const base::Closure& task,
+                       Closure task,
                        base::TimeDelta delay) override;
   bool PostNonNestableDelayedTask(const tracked_objects::Location& from_here,
-                                  const base::Closure& task,
+                                  Closure task,
                                   base::TimeDelta delay) override;
   bool RunsTasksOnCurrentThread() const override;
 

base/sequenced_task_runner.cc

@@ -4,14 +4,17 @@
 
 #include "base/sequenced_task_runner.h"
 
+#include <utility>
+
 #include "base/bind.h"
 
 namespace base {
 
 bool SequencedTaskRunner::PostNonNestableTask(
     const tracked_objects::Location& from_here,
-    const Closure& task) {
-  return PostNonNestableDelayedTask(from_here, task, base::TimeDelta());
+    Closure task) {
+  return PostNonNestableDelayedTask(from_here, std::move(task),
+                                    base::TimeDelta());
 }
 
 bool SequencedTaskRunner::DeleteOrReleaseSoonInternal(

base/sequenced_task_runner.h

@@ -6,6 +6,7 @@
 #define BASE_SEQUENCED_TASK_RUNNER_H_
 
 #include "base/base_export.h"
+#include "base/callback.h"
 #include "base/sequenced_task_runner_helpers.h"
 #include "base/task_runner.h"
 
@@ -109,11 +110,11 @@ class BASE_EXPORT SequencedTaskRunner : public TaskRunner {
   // below.
 
   bool PostNonNestableTask(const tracked_objects::Location& from_here,
-                           const Closure& task);
+                           Closure task);
 
   virtual bool PostNonNestableDelayedTask(
       const tracked_objects::Location& from_here,
-      const Closure& task,
+      Closure task,
       base::TimeDelta delay) = 0;
 
   // Submits a non-nestable task to delete the given object.  Returns

base/task_runner.cc

@@ -23,7 +23,7 @@ class PostTaskAndReplyTaskRunner : public internal::PostTaskAndReplyImpl {
 
  private:
   bool PostTask(const tracked_objects::Location& from_here,
-                const Closure& task) override;
+                Closure task) override;
 
   // Non-owning.
   TaskRunner* destination_;
@@ -36,15 +36,15 @@ PostTaskAndReplyTaskRunner::PostTaskAndReplyTaskRunner(
 
 bool PostTaskAndReplyTaskRunner::PostTask(
     const tracked_objects::Location& from_here,
-    const Closure& task) {
-  return destination_->PostTask(from_here, task);
+    Closure task) {
+  return destination_->PostTask(from_here, std::move(task));
 }
 
 }  // namespace
 
 bool TaskRunner::PostTask(const tracked_objects::Location& from_here,
-                          const Closure& task) {
-  return PostDelayedTask(from_here, task, base::TimeDelta());
+                          Closure task) {
+  return PostDelayedTask(from_here, std::move(task), base::TimeDelta());
 }
 
 bool TaskRunner::PostTaskAndReply(const tracked_objects::Location& from_here,

base/task_runner.h

@@ -61,16 +61,15 @@ class BASE_EXPORT TaskRunner
   // will not be run.
   //
   // Equivalent to PostDelayedTask(from_here, task, 0).
-  bool PostTask(const tracked_objects::Location& from_here,
-                const Closure& task);
+  bool PostTask(const tracked_objects::Location& from_here, Closure task);
 
   // Like PostTask, but tries to run the posted task only after
   // |delay_ms| has passed.
   //
   // It is valid for an implementation to ignore |delay_ms|; that is,
   // to have PostDelayedTask behave the same as PostTask.
   virtual bool PostDelayedTask(const tracked_objects::Location& from_here,
-                               const Closure& task,
+                               Closure task,
                                base::TimeDelta delay) = 0;
 
   // Returns true if the current thread is a thread on which a task