Reduce leaks in PostTaskAndReplyImpl::PostTaskAndReply (reland #2).

This CL previously landed as
https://chromium-review.googlesource.com/997892. It was reverted because
of an access race in ChromeStorageImpl https://crbug.com/829122. This
CL fixes the issue by deleting both callbacks on the origin sequence
when they can't run.

Before, there was always leak when an internal task posted by
PostTaskAndReplyImpl didn't run. With this CL, PostTaskAndReplyImpl
notices when its internal callbacks aren't scheduled and schedules
deletion of all its internal state (including callbacks that didn't
run) on the origin sequence via a DeleteSoon(). Note that a deletion
scheduled via DeleteSoon() might not happen if Chrome is shutting
down.

R=gab@chromium.org

Bug: 807013
Change-Id: I17abffd3e7afcc861c8c92dce30c14ce8629b125
Reviewed-on: https://chromium-review.googlesource.com/1006045
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550189}

base/threading/post_task_and_reply_impl.cc

@@ -10,7 +10,6 @@
 #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"
 
@@ -18,56 +17,93 @@ 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)
-      : sequence_checker_(),
-        from_here_(from_here),
-        origin_task_runner_(SequencedTaskRunnerHandle::Get()),
-        reply_(std::move(reply)),
-        task_(std::move(task)) {}
+      : from_here_(from_here),
+        task_(std::move(task)),
+        reply_(std::move(reply)) {}
+  PostTaskAndReplyRelay(PostTaskAndReplyRelay&&) = default;
 
   ~PostTaskAndReplyRelay() {
-    DCHECK(sequence_checker_.CalledOnValidSequence());
+    if (reply_) {
+      // This can run:
+      // 1) On origin sequence, when:
+      //    1a) Posting |task_| fails.
+      //    1b) |reply_| is cancelled before running.
+      //    1c) The DeleteSoon() below is scheduled.
+      // 2) On destination sequence, when:
+      //    2a) |task_| is cancelled before running.
+      //    2b) Posting |reply_| fails.
+
+      if (!reply_task_runner_->RunsTasksInCurrentSequence()) {
+        // Case 2a) or 2b).
+        //
+        // Destroy callbacks asynchronously on |reply_task_runner| since their
+        // destructors 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).
+        //
+        // Note: while it's obvious why |reply_| can be affine to
+        // |reply_task_runner|, the reason that |task_| can also be affine to it
+        // is that it if neither tasks ran, |task_| may still hold an object
+        // which was intended to be moved to |reply_| when |task_| ran (such an
+        // object's destruction can be affine to |reply_task_runner_| -- e.g.
+        // https://crbug.com/829122).
+        auto relay_to_delete =
+            std::make_unique<PostTaskAndReplyRelay>(std::move(*this));
+        ANNOTATE_LEAKING_OBJECT_PTR(relay_to_delete.get());
+        reply_task_runner_->DeleteSoon(from_here_, std::move(relay_to_delete));
+      }
+
+      // Case 1a), 1b), 1c).
+      //
+      // Callbacks will be destroyed synchronously at the end of this scope.
+    } else {
+      // This can run when both callbacks have run or have been moved to another
+      // PostTaskAndReplyRelay instance. If |reply_| is null, |task_| must be
+      // null too.
+      DCHECK(!task_);
+    }
   }
 
-  void RunTaskAndPostReply() {
-    std::move(task_).Run();
-    origin_task_runner_->PostTask(
-        from_here_, BindOnce(&PostTaskAndReplyRelay::RunReplyAndSelfDestruct,
-                             base::Unretained(this)));
-  }
+  // No assignment operator because of const members.
+  PostTaskAndReplyRelay& operator=(PostTaskAndReplyRelay&&) = delete;
 
- private:
-  void RunReplyAndSelfDestruct() {
-    DCHECK(sequence_checker_.CalledOnValidSequence());
+  // 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();
 
-    // 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_);
+    // 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_;
 
-    std::move(reply_).Run();
+    reply_task_runner->PostTask(
+        relay.from_here_,
+        BindOnce(&PostTaskAndReplyRelay::RunReply, std::move(relay)));
+  }
 
-    // Cue mission impossible theme.
-    delete this;
+ 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_);
+    std::move(relay.reply_).Run();
   }
 
-  const SequenceChecker sequence_checker_;
   const Location from_here_;
-  const scoped_refptr<SequencedTaskRunner> origin_task_runner_;
-  OnceClosure reply_;
   OnceClosure task_;
+  OnceClosure reply_;
+  const scoped_refptr<SequencedTaskRunner> reply_task_runner_ =
+      SequencedTaskRunnerHandle::Get();
+
+  DISALLOW_COPY_AND_ASSIGN(PostTaskAndReplyRelay);
 };
 
 }  // namespace
@@ -77,23 +113,13 @@ namespace internal {
 bool PostTaskAndReplyImpl::PostTaskAndReply(const Location& from_here,
                                             OnceClosure task,
                                             OnceClosure reply) {
-  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;
-  }
+  DCHECK(task) << from_here.ToString();
+  DCHECK(reply) << from_here.ToString();
 
-  return true;
+  return PostTask(from_here,
+                  BindOnce(&PostTaskAndReplyRelay::RunTaskAndPostReply,
+                           PostTaskAndReplyRelay(from_here, std::move(task),
+                                                 std::move(reply))));
 }
 
 }  // namespace internal

base/threading/post_task_and_reply_impl.h

@@ -18,17 +18,20 @@ namespace internal {
 // custom execution context.
 //
 // If you're looking for a concrete implementation of PostTaskAndReply, you
-// probably want base::TaskRunner.
-//
-// TODO(fdoray): Move this to the anonymous namespace of base/task_runner.cc.
+// probably want base::TaskRunner or base/task_scheduler/post_task.h
 class BASE_EXPORT PostTaskAndReplyImpl {
  public:
   virtual ~PostTaskAndReplyImpl() = default;
 
-  // 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.
+  // Posts |task| by calling PostTask(). On completion, posts |reply| to the
+  // origin sequence. Can only be called when
+  // SequencedTaskRunnerHandle::IsSet(). Each callback is deleted synchronously
+  // after running, or scheduled for asynchronous deletion on the origin
+  // sequence if it can't run (e.g. if a TaskRunner skips it on shutdown). See
+  // SequencedTaskRunner::DeleteSoon() for when objects scheduled for
+  // asynchronous deletion can be leaked. Note: All //base task posting APIs
+  // require callbacks to support deletion on the posting sequence if they can't
+  // be scheduled.
   bool PostTaskAndReply(const Location& from_here,
                         OnceClosure task,
                         OnceClosure reply);

base/threading/post_task_and_reply_impl_unittest.cc

@@ -6,12 +6,12 @@
 
 #include <utility>
 
+#include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "base/test/test_simple_task_runner.h"
-#include "base/threading/thread_task_runner_handle.h"
+#include "base/test/test_mock_time_task_runner.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -57,53 +57,141 @@ class MockObject {
   MockObject() = default;
 
   MOCK_METHOD1(Task, void(scoped_refptr<ObjectToDelete>));
-  MOCK_METHOD0(Reply, void());
+  MOCK_METHOD1(Reply, void(scoped_refptr<ObjectToDelete>));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockObject);
 };
 
+class MockRunsTasksInCurrentSequenceTaskRunner : public TestMockTimeTaskRunner {
+ public:
+  MockRunsTasksInCurrentSequenceTaskRunner(
+      TestMockTimeTaskRunner::Type type =
+          TestMockTimeTaskRunner::Type::kStandalone)
+      : TestMockTimeTaskRunner(type) {}
+
+  void RunUntilIdleWithRunsTasksInCurrentSequence() {
+    AutoReset<bool> reset(&runs_tasks_in_current_sequence_, true);
+    RunUntilIdle();
+  }
+
+  void ClearPendingTasksWithRunsTasksInCurrentSequence() {
+    AutoReset<bool> reset(&runs_tasks_in_current_sequence_, true);
+    ClearPendingTasks();
+  }
+
+  // TestMockTimeTaskRunner:
+  bool RunsTasksInCurrentSequence() const override {
+    return runs_tasks_in_current_sequence_;
+  }
+
+ private:
+  ~MockRunsTasksInCurrentSequenceTaskRunner() override = default;
+
+  bool runs_tasks_in_current_sequence_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(MockRunsTasksInCurrentSequenceTaskRunner);
+};
+
+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<MockRunsTasksInCurrentSequenceTaskRunner> post_runner_ =
+      MakeRefCounted<MockRunsTasksInCurrentSequenceTaskRunner>();
+  scoped_refptr<MockRunsTasksInCurrentSequenceTaskRunner> reply_runner_ =
+      MakeRefCounted<MockRunsTasksInCurrentSequenceTaskRunner>(
+          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(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());
+TEST_F(PostTaskAndReplyImplTest, PostTaskAndReply) {
+  PostTaskAndReplyToMockObject();
+
+  EXPECT_CALL(mock_object_, Task(_));
+  post_runner_->RunUntilIdleWithRunsTasksInCurrentSequence();
+  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_->RunUntilIdleWithRunsTasksInCurrentSequence();
+  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, TaskDoesNotRun) {
+  PostTaskAndReplyToMockObject();
+
+  // Clear the |post_runner_|. Both callbacks should be scheduled for deletion
+  // on the |reply_runner_|.
+  post_runner_->ClearPendingTasksWithRunsTasksInCurrentSequence();
+  EXPECT_FALSE(post_runner_->HasPendingTask());
+  EXPECT_TRUE(reply_runner_->HasPendingTask());
+  EXPECT_FALSE(delete_task_flag_);
+  EXPECT_FALSE(delete_reply_flag_);
+
+  // Run the |reply_runner_|. Both callbacks should be deleted.
+  reply_runner_->RunUntilIdleWithRunsTasksInCurrentSequence();
+  EXPECT_TRUE(delete_task_flag_);
+  EXPECT_TRUE(delete_reply_flag_);
+}
+
+TEST_F(PostTaskAndReplyImplTest, ReplyDoesNotRun) {
+  PostTaskAndReplyToMockObject();
+
+  EXPECT_CALL(mock_object_, Task(_));
+  post_runner_->RunUntilIdleWithRunsTasksInCurrentSequence();
+  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_->ClearPendingTasksWithRunsTasksInCurrentSequence();
+  EXPECT_TRUE(delete_task_flag_);
+  EXPECT_TRUE(delete_reply_flag_);
 }
 
 }  // namespace internal