Implement base::BindPostTask()

This function binds a callback to a task runner. The functionality
already existed in media::BindToLoop() so it's intended to replace it
and is built on the same BindPostTaskTrampoline logic. BindToLoop() and
BindToCurrentLoop() will be replaced in a follow up CL since they are
used rather widely already.

There are some changes to the existing BindToLoop implementation:
1. Get the FROM_HERE location from the caller automatically rather than
   using the BindPostTask() location.
2. If BindPostTaskTrampoline has a OnceClosure, don't rebind the closure
   before posting a task.
3. Add static_assert to provide a readable error message explaining the
   input callback must have a void return type.
4. Don't support RepeatingCallback. This is more problematic since we
   will always PostTask() to destroy the callback. This is easier to add
   later than it would be to remove support.

Bug: 1140582
Change-Id: Ibb224b44c0b0c01dd88d29e94c7e9449d3353ef5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2495055
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827818}

base/BUILD.gn

@@ -153,6 +153,8 @@ component("base") {
     "big_endian.h",
     "bind.h",
     "bind_internal.h",
+    "bind_post_task.h",
+    "bind_post_task_internal.h",
     "bit_cast.h",
     "bits.h",
     "build_time.cc",
@@ -2709,6 +2711,7 @@ test("base_unittests") {
     "base64_unittest.cc",
     "base64url_unittest.cc",
     "big_endian_unittest.cc",
+    "bind_post_task_unittest.cc",
     "bind_unittest.cc",
     "bit_cast_unittest.cc",
     "bits_unittest.cc",
@@ -3473,6 +3476,7 @@ action("build_date") {
 if (enable_nocompile_tests) {
   nocompile_test("base_nocompile_tests") {
     sources = [
+      "bind_post_task_unittest.nc",
       "callback_list_unittest.nc",
       "callback_unittest.nc",
       "containers/buffer_iterator_unittest.nc",

base/bind_post_task.h

+// Copyright 2020 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.
+
+#ifndef BASE_BIND_POST_TASK_H_
+#define BASE_BIND_POST_TASK_H_
+
+#include <memory>
+#include <type_traits>
+
+#include "base/bind.h"
+#include "base/bind_post_task_internal.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/memory/ref_counted.h"
+#include "base/task_runner.h"
+
+// BindPostTask() is a helper function for binding a OnceCallback to a task
+// runner. BindPostTask(task_runner, callback) returns a task runner bound
+// callback with an identical type to |callback|. The returned callback will
+// take the same arguments as the input |callback|. Invoking Run() on the
+// returned callback will post a task to run |callback| on target |task_runner|
+// with the provided arguments.
+//
+// This is typically used when a callback must be invoked on a specific task
+// runner but is provided as a result callback to a function that runs
+// asynchronously on a different task runner.
+//
+// Example:
+//    // |result_cb| can only be safely run on |my_task_runner|.
+//    auto result_cb = BindOnce(&Foo::ReceiveReply, foo);
+//    // Note that even if |returned_cb| is never run |result_cb| will attempt
+//    // to be destroyed on |my_task_runner|.
+//    auto returned_cb = BindPostTask(my_task_runner, std::move(result_cb));
+//    // RunAsyncTask() will run the provided callback upon completion.
+//    other_task_runner->PostTask(FROM_HERE,
+//                                BindOnce(&RunAsyncTask,
+//                                         std::move(returned_cb)));
+//
+// If the example provided |result_cb| to RunAsyncTask() then |result_cb| would
+// run unsafely on |other_task_runner|. Instead RunAsyncTask() will run
+// |returned_cb| which will post a task to |current_task_runner| before running
+// |result_cb| safely.
+//
+// An alternative approach in the example above is to change RunAsyncTask() to
+// also take a task runner as an argument and have RunAsyncTask() post the task.
+// For cases where that isn't desirable BindPostTask() provides a convenient
+// alternative.
+//
+// The input |callback| will always attempt to be destroyed on the target task
+// runner. Even if the returned callback is never invoked, a task will be posted
+// to destroy the input |callback|. However, if the target task runner has
+// shutdown this is no longer possible. PostTask() will return false and the
+// callback will be destroyed immediately on the current thread.
+//
+// The input |callback| must have a void return type to be compatible with
+// PostTask(). If you want to drop the callback return value then use
+// base::IgnoreResult(&Func) when creating the input |callback|.
+
+namespace base {
+
+// Creates a OnceCallback that will run |callback| on |task_runner|. If the
+// returned callback is destroyed without being run then |callback| will be
+// be destroyed on |task_runner|.
+template <typename ReturnType, typename... Args>
+OnceCallback<void(Args...)> BindPostTask(
+    scoped_refptr<TaskRunner> task_runner,
+    OnceCallback<ReturnType(Args...)> callback,
+    const Location& location = FROM_HERE) {
+  static_assert(std::is_same<ReturnType, void>::value,
+                "OnceCallback must have void return type in order to produce a "
+                "closure for PostTask(). Use base::IgnoreResult() to drop the "
+                "return value if desired.");
+
+  using Helper = internal::BindPostTaskTrampoline<OnceCallback<void(Args...)>>;
+
+  return base::BindOnce(
+      &Helper::template Run<Args...>,
+      std::make_unique<Helper>(std::move(task_runner), location,
+                               std::move(callback)));
+}
+
+}  // namespace base
+
+#endif  // BASE_BIND_POST_TASK_H_

base/bind_post_task_internal.h

+// Copyright 2020 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.
+
+#ifndef BASE_BIND_POST_TASK_INTERNAL_H_
+#define BASE_BIND_POST_TASK_INTERNAL_H_
+
+#include <utility>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/location.h"
+#include "base/memory/ref_counted.h"
+#include "base/task_runner.h"
+
+namespace base {
+namespace internal {
+
+// Helper class to ensure that the input callback is always invoked and
+// destroyed on the provided task runner.
+template <typename CallbackType>
+class BindPostTaskTrampoline {
+ public:
+  BindPostTaskTrampoline(scoped_refptr<TaskRunner> task_runner,
+                         const Location& location,
+                         CallbackType callback)
+      : task_runner_(std::move(task_runner)),
+        location_(location),
+        callback_(std::move(callback)) {
+    DCHECK(task_runner_);
+    // Crash immediately instead of when trying to Run() `callback_` on the
+    // destination `task_runner_`.
+    CHECK(callback_);
+  }
+
+  BindPostTaskTrampoline(const BindPostTaskTrampoline& other) = delete;
+  BindPostTaskTrampoline& operator=(const BindPostTaskTrampoline& other) =
+      delete;
+
+  ~BindPostTaskTrampoline() {
+    if (callback_) {
+      // Post a task to ensure that `callback_` is destroyed on `task_runner_`.
+      // The callback's BindState may own an object that isn't threadsafe and is
+      // unsafe to destroy on a different task runner.
+      //
+      // Note that while this guarantees `callback_` will be destroyed when the
+      // posted task runs, it doesn't guarantee the ref-counted BindState is
+      // destroyed at the same time. If the callback was copied before being
+      // passed to BindPostTaskTrampoline then the BindState can outlive
+      // `callback_`, so the user must ensure any other copies of the callback
+      // are also destroyed on the correct task runner.
+      task_runner_->PostTask(location_, BindOnce(&DestroyCallbackOnTaskRunner,
+                                                 std::move(callback_)));
+    }
+  }
+
+  template <typename... Args>
+  void Run(Args... args) {
+    // GetClosure() consumes `callback_`.
+    task_runner_->PostTask(location_,
+                           GetClosure(&callback_, std::forward<Args>(args)...));
+  }
+
+ private:
+  static OnceClosure GetClosure(OnceClosure* callback) {
+    // `callback` is already a closure, no need to call BindOnce().
+    return std::move(*callback);
+  }
+
+  template <typename... Args>
+  static OnceClosure GetClosure(OnceCallback<void(Args...)>* callback,
+                                Args&&... args) {
+    return BindOnce(std::move(*callback), std::forward<Args>(args)...);
+  }
+
+  static void DestroyCallbackOnTaskRunner(CallbackType callback) {}
+
+  const scoped_refptr<TaskRunner> task_runner_;
+  const Location location_;
+  CallbackType callback_;
+};
+
+}  // namespace internal
+}  // namespace base
+
+#endif  // BASE_BIND_POST_TASK_INTERNAL_H_

base/bind_post_task_unittest.cc

+// Copyright 2020 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/bind_post_task.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/sequence_checker_impl.h"
+#include "base/sequenced_task_runner.h"
+#include "base/test/task_environment.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "base/threading/thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+namespace {
+
+void SetBool(bool* variable, bool value) {
+  *variable = value;
+}
+
+void SetInt(int* variable, int value) {
+  *variable = value;
+}
+
+int Multiply(int value) {
+  return value * 5;
+}
+
+void ClearReference(OnceClosure callback) {}
+
+class SequenceRestrictionChecker {
+ public:
+  explicit SequenceRestrictionChecker(bool& set_on_destroy)
+      : set_on_destroy_(set_on_destroy) {}
+
+  ~SequenceRestrictionChecker() {
+    EXPECT_TRUE(checker_.CalledOnValidSequence());
+    set_on_destroy_ = true;
+  }
+
+  void Run() { EXPECT_TRUE(checker_.CalledOnValidSequence()); }
+
+ private:
+  SequenceCheckerImpl checker_;
+  bool& set_on_destroy_;
+};
+
+}  // namespace
+
+class BindPostTaskTest : public testing::Test {
+ protected:
+  test::SingleThreadTaskEnvironment task_environment_;
+  scoped_refptr<SequencedTaskRunner> task_runner_ =
+      SequencedTaskRunnerHandle::Get();
+};
+
+TEST_F(BindPostTaskTest, OnceClosure) {
+  bool val = false;
+  OnceClosure cb = BindOnce(&SetBool, &val, true);
+  OnceClosure post_cb = BindPostTask(task_runner_, std::move(cb));
+
+  std::move(post_cb).Run();
+  EXPECT_FALSE(val);
+
+  RunLoop().RunUntilIdle();
+  EXPECT_TRUE(val);
+}
+
+TEST_F(BindPostTaskTest, OnceCallback) {
+  OnceCallback<void(bool*, bool)> cb = BindOnce(&SetBool);
+  OnceCallback<void(bool*, bool)> post_cb =
+      BindPostTask(task_runner_, std::move(cb));
+
+  bool val = false;
+  std::move(post_cb).Run(&val, true);
+  EXPECT_FALSE(val);
+
+  RunLoop().RunUntilIdle();
+  EXPECT_TRUE(val);
+}
+
+TEST_F(BindPostTaskTest, OnceWithIgnoreResult) {
+  OnceCallback<void(int)> post_cb =
+      BindPostTask(task_runner_, BindOnce(IgnoreResult(&Multiply)));
+  std::move(post_cb).Run(1);
+  RunLoop().RunUntilIdle();
+}
+
+TEST_F(BindPostTaskTest, OnceThen) {
+  int value = 0;
+
+  // Multiply() returns an int and SetInt() takes an int as a parameter.
+  OnceClosure then_cb =
+      BindOnce(&Multiply, 5)
+          .Then(BindPostTask(task_runner_, BindOnce(&SetInt, &value)));
+
+  std::move(then_cb).Run();
+  EXPECT_EQ(value, 0);
+  RunLoop().RunUntilIdle();
+  EXPECT_EQ(value, 25);
+}
+
+// Ensure that the input callback is run/destroyed on the correct thread even if
+// the callback returned from BindPostTask() is run on a different thread.
+TEST_F(BindPostTaskTest, OnceRunDestroyedOnBound) {
+  Thread target_thread("testing");
+  ASSERT_TRUE(target_thread.Start());
+
+  // SequenceRestrictionChecker checks it's creation, Run() and deletion all
+  // happen on the main thread.
+  bool destroyed = false;
+  auto checker = std::make_unique<SequenceRestrictionChecker>(destroyed);
+
+  // `checker` is owned by `cb` which is wrapped in `post_cb`. `post_cb` is run
+  // on a different thread which triggers a PostTask() back to the test main
+  // thread to invoke `cb` which runs SequenceRestrictionChecker::Run(). After
+  // `cb` has been invoked `checker` is destroyed along with the BindState.
+  OnceClosure cb =
+      BindOnce(&SequenceRestrictionChecker::Run, std::move(checker));
+  OnceClosure post_cb = BindPostTask(task_runner_, std::move(cb));
+  target_thread.task_runner()->PostTask(FROM_HERE, std::move(post_cb));
+
+  target_thread.FlushForTesting();
+  EXPECT_FALSE(destroyed);
+  RunLoop().RunUntilIdle();
+  EXPECT_TRUE(destroyed);
+}
+
+// Ensure that the input callback is destroyed on the correct thread even if the
+// callback returned from BindPostTask() is destroyed without being run on a
+// different thread.
+TEST_F(BindPostTaskTest, OnceNotRunDestroyedOnBound) {
+  Thread target_thread("testing");
+  ASSERT_TRUE(target_thread.Start());
+
+  // SequenceRestrictionChecker checks it's creation and deletion all happen on
+  // the test main thread.
+  bool destroyed = false;
+  auto checker = std::make_unique<SequenceRestrictionChecker>(destroyed);
+
+  // `checker` is owned by `cb` which is wrapped in `post_cb`. `post_cb` is
+  // deleted on a different thread which triggers a PostTask() back to the test
+  // main thread to destroy `cb` and `checker`.
+  OnceClosure cb =
+      BindOnce(&SequenceRestrictionChecker::Run, std::move(checker));
+  OnceClosure post_cb = BindPostTask(task_runner_, std::move(cb));
+  target_thread.task_runner()->PostTask(
+      FROM_HERE, BindOnce(&ClearReference, std::move(post_cb)));
+
+  target_thread.FlushForTesting();
+  EXPECT_FALSE(destroyed);
+  RunLoop().RunUntilIdle();
+  EXPECT_TRUE(destroyed);
+}
+
+}  // namespace base

base/bind_post_task_unittest.nc

+// Copyright 2020 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.
+
+// This is a "No Compile Test" suite.
+// http://dev.chromium.org/developers/testing/no-compile-tests
+
+#include "base/bind_post_task.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+
+namespace base {
+
+int ReturnInt() {
+  return 5;
+}
+
+#if defined(NCTEST_ONCE_NON_VOID_RETURN)  // [r"||fatal error: static_assert failed due to requirement 'std::is_same<int, void>::value' \"OnceCallback must have void return type in order to produce a closure for PostTask\(\). Use base::IgnoreResult\(\) to drop the return value if desired.\""]
+// OnceCallback with non-void return type.
+void WontCompile() {
+  OnceCallback<int()> cb = BindOnce(&ReturnInt);
+  auto post_cb = BindPostTask(SequencedTaskRunnerHandle::Get(), std::move(cb));
+  std::move(post_cb).Run();
+}
+
+#endif
+
+}  // namespace base

docs/callback.md

@@ -191,6 +191,47 @@ helper in the [base::PassThrough CL](https://chromium-review.googlesource.com/c/
 If this would be helpful for you, please let danakj@chromium.org know or ping
 the CL.
 
+### Chaining callbacks across different task runners
+
+```cpp
+// The task runner for a different thread.
+scoped_refptr<base::SequencedTaskRunner> other_task_runner = ...;
+
+// A function to compute some interesting result, except it can only be run
+// safely from `other_task_runner` and not the current thread.
+int ComputeResult();
+
+base::OnceCallback<int()> compute_result_cb = base::BindOnce(&ComputeResult);
+
+// Task runner for the current thread.
+scoped_refptr<base::SequencedTaskRunner> current_task_runner =
+    base::SequencedTaskRunnerHandle::Get();
+
+// A function to accept the result, except it can only be run safely from the
+// current thread.
+void ProvideResult(int result);
+
+base::OnceCallback<void(int)> provide_result_cb =
+    base::BindOnce(&ProvideResult);
+```
+
+Using `Then()` to join `compute_result_cb` and `provide_result_cb` directly
+would be inappropriate. `ComputeResult()` and `ProvideResult()` would run on the
+same thread which isn't safe. However, `base::BindPostTask()` can be used to
+ensure `provide_result_cb` will run on `current_task_runner`.
+
+```cpp
+// The following two statements post a task to `other_task_runner` to run
+// `task`. This will invoke ComputeResult() on a different thread to get the
+// result value then post a task back to `current_task_runner` to invoke
+// ProvideResult() with the result.
+OnceClosure task =
+    std::move(compute_result_cb)
+        .Then(base::BindPostTask(current_task_runner,
+                                 std::move(provide_result_cb)));
+other_task_runner->PostTask(FROM_HERE, std::move(task));
+```
+
 ## Quick reference for basic stuff
 
 ### Binding A Bare Function