Revert "Reduce leaks in PostTaskAndReplyImpl::PostTaskAndReply (reland)."

This reverts commit 8e10a0af.

Reason for revert: surfaces flakes in browser_tests, see crbug.com/829122
(only reverting base/ changes, test fixes can stay in)

Original change's description:
> Reduce leaks in PostTaskAndReplyImpl::PostTaskAndReply (reland).
>
> This CL first landed as https://chromium-review.googlesource.com/902191.
> It was reverted because of a leak in PipeReaderTest.Cancel, which is
> fixed by this CL by flushing the ScopedTaskEnvironment before the test
> ends.
>
> Before, there was always a leak when the RunTaskAndPostReply callback
> posted by PostTaskAndReplyImpl::PostTaskAndReply didn't run.
>
> With this CL, the "task" is never leaked and the "reply" is only
> leaked if the execution environment is shutdown before the deletion
> happens (e.g. MessageLoop deleted, TaskScheduler shutdown).
>
> TBR=gab@chromium.org,sky@chromium.org,kbr@chromium.org
>
> Bug: 807013
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> Change-Id: Ic28839da7aed25fc56a4abadc565b7f4a6e0b5c5
> Reviewed-on: https://chromium-review.googlesource.com/997892
> Reviewed-by: François Doray <fdoray@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
> Commit-Queue: François Doray <fdoray@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#548919}

TBR=stevenjb@chromium.org,sky@chromium.org,gab@chromium.org,fdoray@chromium.org,kbr@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 807013, 829122
Change-Id: Iff4ca23f3cf011b0587478f479a1676e618b741b
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1003792
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#549398}

base/threading/post_task_and_reply_impl.cc

@@ -10,6 +10,7 @@
 #include "base/debug/leak_annotations.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
+#include "base/sequence_checker.h"
 #include "base/sequenced_task_runner.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 
@@ -17,87 +18,56 @@ namespace base {
 
 namespace {
 
+// This relay class remembers the sequence that it was created on, and ensures
+// that both the |task| and |reply| Closures are deleted on this same sequence.
+// Also, |task| is guaranteed to be deleted before |reply| is run or deleted.
+//
+// If RunReplyAndSelfDestruct() doesn't run because the originating execution
+// context is no longer available, then the |task| and |reply| Closures are
+// leaked. Leaking is considered preferable to having a thread-safetey
+// violations caused by invoking the Closure destructor on the wrong sequence.
 class PostTaskAndReplyRelay {
  public:
   PostTaskAndReplyRelay(const Location& from_here,
                         OnceClosure task,
                         OnceClosure reply)
-      : from_here_(from_here),
-        task_(std::move(task)),
-        reply_(std::move(reply)) {}
-  PostTaskAndReplyRelay(PostTaskAndReplyRelay&&) = default;
+      : sequence_checker_(),
+        from_here_(from_here),
+        origin_task_runner_(SequencedTaskRunnerHandle::Get()),
+        reply_(std::move(reply)),
+        task_(std::move(task)) {}
 
   ~PostTaskAndReplyRelay() {
-    // This destructor can run:
-    // 1) On origin sequence, when:
-    //    1a) Posting |task_| fails.
-    //    1b) |reply_| is cancelled before running.
-    //    1c) |reply_| completes.
-    // 2) On destination sequence, when:
-    //    2a) |task_| is cancelled before running.
-    //    2b) Posting |reply_| fails.
-
-    // |task_| can still be alive if the destination task runner was no longer
-    // accepting tasks or if it was cancelled before being run. Destroy it ahead
-    // of |reply_| to keep happens-before expectations sane. |task_| can be
-    // safely destroyed on origin or destination sequence per being constructed
-    // on the former and designed to be ran on the latter.
-    DCHECK(task_.is_null() || !reply_.is_null());
-    task_.Reset();
-
-    if (reply_) {
-      if (reply_task_runner_->RunsTasksInCurrentSequence()) {
-        // Case 1a) or 1b).
-        reply_.Reset();
-      } else {
-        // Case 2a) or 2b).
-        // Destroy |reply_| asynchronously on |reply_task_runner_| since it can
-        // rightfully be affine to it. As always, DeleteSoon() might leak its
-        // argument if the target execution environment is shutdown (e.g.
-        // MessageLoop deleted, TaskScheduler shutdown).
-        auto reply_to_delete = std::make_unique<OnceClosure>(std::move(reply_));
-        ANNOTATE_LEAKING_OBJECT_PTR(reply_to_delete.get());
-        reply_task_runner_->DeleteSoon(from_here_, std::move(reply_to_delete));
-      }
-    }
+    DCHECK(sequence_checker_.CalledOnValidSequence());
   }
 
-  // No assignment operator because of const members.
-  PostTaskAndReplyRelay& operator=(PostTaskAndReplyRelay&&) = delete;
+  void RunTaskAndPostReply() {
+    std::move(task_).Run();
+    origin_task_runner_->PostTask(
+        from_here_, BindOnce(&PostTaskAndReplyRelay::RunReplyAndSelfDestruct,
+                             base::Unretained(this)));
+  }
 
-  // Static function is used because it is not possible to bind a method call to
-  // a non-pointer type.
-  static void RunTaskAndPostReply(PostTaskAndReplyRelay relay) {
-    DCHECK(relay.task_);
-    std::move(relay.task_).Run();
+ private:
+  void RunReplyAndSelfDestruct() {
+    DCHECK(sequence_checker_.CalledOnValidSequence());
 
-    // Keep a reference to the reply TaskRunner for the PostTask() call before
-    // |relay| is moved into a callback.
-    scoped_refptr<SequencedTaskRunner> reply_task_runner =
-        relay.reply_task_runner_;
+    // Ensure |task_| has already been released before |reply_| to ensure that
+    // no one accidentally depends on |task_| keeping one of its arguments alive
+    // while |reply_| is executing.
+    DCHECK(!task_);
 
-    reply_task_runner->PostTask(
-        relay.from_here_,
-        BindOnce(&PostTaskAndReplyRelay::RunReply, std::move(relay)));
-  }
+    std::move(reply_).Run();
 
- private:
-  // Static function is used because it is not possible to bind a method call to
-  // a non-pointer type.
-  static void RunReply(PostTaskAndReplyRelay relay) {
-    DCHECK(!relay.task_);
-    DCHECK(relay.reply_);
-    // Case 1c).
-    std::move(relay.reply_).Run();
+    // Cue mission impossible theme.
+    delete this;
   }
 
+  const SequenceChecker sequence_checker_;
   const Location from_here_;
-  OnceClosure task_;
+  const scoped_refptr<SequencedTaskRunner> origin_task_runner_;
   OnceClosure reply_;
-  const scoped_refptr<SequencedTaskRunner> reply_task_runner_ =
-      SequencedTaskRunnerHandle::Get();
-
-  DISALLOW_COPY_AND_ASSIGN(PostTaskAndReplyRelay);
+  OnceClosure task_;
 };
 
 }  // namespace
@@ -107,13 +77,23 @@ namespace internal {
 bool PostTaskAndReplyImpl::PostTaskAndReply(const Location& from_here,
                                             OnceClosure task,
                                             OnceClosure reply) {
-  DCHECK(task) << from_here.ToString();
-  DCHECK(reply) << from_here.ToString();
+  DCHECK(!task.is_null()) << from_here.ToString();
+  DCHECK(!reply.is_null()) << from_here.ToString();
+  PostTaskAndReplyRelay* relay =
+      new PostTaskAndReplyRelay(from_here, std::move(task), std::move(reply));
+  // PostTaskAndReplyRelay self-destructs after executing |reply|. On the flip
+  // side though, it is intentionally leaked if the |task| doesn't complete
+  // before the origin sequence stops executing tasks. Annotate |relay| as leaky
+  // to avoid having to suppress every callsite which happens to flakily trigger
+  // this race.
+  ANNOTATE_LEAKING_OBJECT_PTR(relay);
+  if (!PostTask(from_here, BindOnce(&PostTaskAndReplyRelay::RunTaskAndPostReply,
+                                    Unretained(relay)))) {
+    delete relay;
+    return false;
+  }
 
-  return PostTask(from_here,
-                  BindOnce(&PostTaskAndReplyRelay::RunTaskAndPostReply,
-                           PostTaskAndReplyRelay(from_here, std::move(task),
-                                                 std::move(reply))));
+  return true;
 }
 
 }  // namespace internal

base/threading/post_task_and_reply_impl.h

@@ -18,17 +18,17 @@ namespace internal {
 // custom execution context.
 //
 // If you're looking for a concrete implementation of PostTaskAndReply, you
-// probably want base::TaskRunner or base/task_scheduler/post_task.h
+// probably want base::TaskRunner.
+//
+// TODO(fdoray): Move this to the anonymous namespace of base/task_runner.cc.
 class BASE_EXPORT PostTaskAndReplyImpl {
  public:
   virtual ~PostTaskAndReplyImpl() = default;
 
-  // Posts |task| by calling PostTask(). On completion, posts |reply| to the
-  // sequence that called this. Can only be called when
-  // SequencedTaskRunnerHandle::IsSet(). |task| is deleted on the target
-  // TaskRunner or on the sequence that called this. |reply| is deleted on the
-  // sequence that called this, or leaked if it can't be deleted before the
-  // sequence stops accepting tasks.
+  // Posts |task| by calling PostTask(). On completion, |reply| is posted to the
+  // sequence or thread that called this. Can only be called when
+  // SequencedTaskRunnerHandle::IsSet(). Both |task| and |reply| are guaranteed
+  // to be deleted on the sequence or thread that called this.
   bool PostTaskAndReply(const Location& from_here,
                         OnceClosure task,
                         OnceClosure reply);

base/threading/post_task_and_reply_impl_unittest.cc

@@ -10,7 +10,8 @@
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/test/test_mock_time_task_runner.h"
+#include "base/test/test_simple_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -56,94 +57,53 @@ class MockObject {
   MockObject() = default;
 
   MOCK_METHOD1(Task, void(scoped_refptr<ObjectToDelete>));
-  MOCK_METHOD1(Reply, void(scoped_refptr<ObjectToDelete>));
+  MOCK_METHOD0(Reply, void());
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockObject);
 };
 
-class PostTaskAndReplyImplTest : public testing::Test {
- protected:
-  PostTaskAndReplyImplTest() = default;
-
-  void PostTaskAndReplyToMockObject() {
-    // Expect the post to succeed.
-    EXPECT_TRUE(
-        PostTaskAndReplyTaskRunner(post_runner_.get())
-            .PostTaskAndReply(
-                FROM_HERE,
-                BindOnce(&MockObject::Task, Unretained(&mock_object_),
-                         MakeRefCounted<ObjectToDelete>(&delete_task_flag_)),
-                BindOnce(&MockObject::Reply, Unretained(&mock_object_),
-                         MakeRefCounted<ObjectToDelete>(&delete_reply_flag_))));
-
-    // Expect the first task to be posted to |post_runner_|.
-    EXPECT_TRUE(post_runner_->HasPendingTask());
-    EXPECT_FALSE(reply_runner_->HasPendingTask());
-    EXPECT_FALSE(delete_task_flag_);
-    EXPECT_FALSE(delete_reply_flag_);
-  }
-
-  scoped_refptr<TestMockTimeTaskRunner> post_runner_ =
-      MakeRefCounted<TestMockTimeTaskRunner>();
-  scoped_refptr<TestMockTimeTaskRunner> reply_runner_ =
-      MakeRefCounted<TestMockTimeTaskRunner>(
-          TestMockTimeTaskRunner::Type::kBoundToThread);
-  testing::StrictMock<MockObject> mock_object_;
-  bool delete_task_flag_ = false;
-  bool delete_reply_flag_ = false;
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(PostTaskAndReplyImplTest);
-};
-
 }  // namespace
 
-TEST_F(PostTaskAndReplyImplTest, PostTaskAndReply) {
-  PostTaskAndReplyToMockObject();
-
-  EXPECT_CALL(mock_object_, Task(_));
-  post_runner_->RunUntilIdle();
-  testing::Mock::VerifyAndClear(&mock_object_);
-  // The task should have been deleted right after being run.
-  EXPECT_TRUE(delete_task_flag_);
-  EXPECT_FALSE(delete_reply_flag_);
-
-  // Expect the reply to be posted to |reply_runner_|.
-  EXPECT_FALSE(post_runner_->HasPendingTask());
-  EXPECT_TRUE(reply_runner_->HasPendingTask());
-
-  EXPECT_CALL(mock_object_, Reply(_));
-  reply_runner_->RunUntilIdle();
-  testing::Mock::VerifyAndClear(&mock_object_);
-  EXPECT_TRUE(delete_task_flag_);
-  // The reply should have been deleted right after being run.
-  EXPECT_TRUE(delete_reply_flag_);
-
-  // Expect no pending task in |post_runner_| and |reply_runner_|.
-  EXPECT_FALSE(post_runner_->HasPendingTask());
-  EXPECT_FALSE(reply_runner_->HasPendingTask());
-}
-
-TEST_F(PostTaskAndReplyImplTest, PostTaskAndReplyDoesNotRun) {
-  PostTaskAndReplyToMockObject();
-
-  EXPECT_CALL(mock_object_, Task(_));
-  post_runner_->RunUntilIdle();
-  testing::Mock::VerifyAndClear(&mock_object_);
-  // The task should have been deleted right after being run.
-  EXPECT_TRUE(delete_task_flag_);
-  EXPECT_FALSE(delete_reply_flag_);
-
-  // Expect the reply to be posted to |reply_runner_|.
-  EXPECT_FALSE(post_runner_->HasPendingTask());
-  EXPECT_TRUE(reply_runner_->HasPendingTask());
-
-  // Clear the |reply_runner_| queue without running tasks. The reply callback
-  // should be deleted.
-  reply_runner_->ClearPendingTasks();
-  EXPECT_TRUE(delete_task_flag_);
-  EXPECT_TRUE(delete_reply_flag_);
+TEST(PostTaskAndReplyImplTest, PostTaskAndReply) {
+  scoped_refptr<TestSimpleTaskRunner> post_runner(new TestSimpleTaskRunner);
+  scoped_refptr<TestSimpleTaskRunner> reply_runner(new TestSimpleTaskRunner);
+  ThreadTaskRunnerHandle task_runner_handle(reply_runner);
+
+  testing::StrictMock<MockObject> mock_object;
+  bool delete_flag = false;
+
+  EXPECT_TRUE(PostTaskAndReplyTaskRunner(post_runner.get())
+                  .PostTaskAndReply(
+                      FROM_HERE,
+                      BindOnce(&MockObject::Task, Unretained(&mock_object),
+                               MakeRefCounted<ObjectToDelete>(&delete_flag)),
+                      BindOnce(&MockObject::Reply, Unretained(&mock_object))));
+
+  // Expect the task to be posted to |post_runner|.
+  EXPECT_TRUE(post_runner->HasPendingTask());
+  EXPECT_FALSE(reply_runner->HasPendingTask());
+  EXPECT_FALSE(delete_flag);
+
+  EXPECT_CALL(mock_object, Task(_));
+  post_runner->RunUntilIdle();
+  testing::Mock::VerifyAndClear(&mock_object);
+
+  // |task| should have been deleted right after being run.
+  EXPECT_TRUE(delete_flag);
+
+  // Expect the reply to be posted to |reply_runner|.
+  EXPECT_FALSE(post_runner->HasPendingTask());
+  EXPECT_TRUE(reply_runner->HasPendingTask());
+
+  EXPECT_CALL(mock_object, Reply());
+  reply_runner->RunUntilIdle();
+  testing::Mock::VerifyAndClear(&mock_object);
+  EXPECT_TRUE(delete_flag);
+
+  // Expect no pending task in |post_runner| and |reply_runner|.
+  EXPECT_FALSE(post_runner->HasPendingTask());
+  EXPECT_FALSE(reply_runner->HasPendingTask());
 }
 
 }  // namespace internal