Revert "Reduce leaks in PostTaskAndReplyImpl::PostTaskAndReply."

This reverts commit 970f8498.

Reason for revert: Appears to be causing leaks in chromeos_unittests (PipeReaderTest.Cancel).

Bot: https://ci.chromium.org/buildbot/chromium.memory/Linux%20Chromium%20OS%20ASan%20LSan%20Tests%20%281%29/

Build failure: 
https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.memory%2FLinux_Chromium_OS_ASan_LSan_Tests__1_%2F26886%2F%2B%2Frecipes%2Fsteps%2Fchromeos_unittests%2F0%2Fstdout

Original change's description:
> Reduce leaks in PostTaskAndReplyImpl::PostTaskAndReply.
> 
> 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).
> 
> 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;luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> Change-Id: I05205d1b0250811abe61e2204ba32919d16c16c0
> Reviewed-on: https://chromium-review.googlesource.com/902191
> Reviewed-by: François Doray <fdoray@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Kenneth Russell <kbr@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Commit-Queue: François Doray <fdoray@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#548034}

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

Change-Id: Ib91c72333fabb4e33c1689c5ad39a5ed53ce3beb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
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;luci.chromium.try:android_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/996732Reviewed-by: Darren Shen <shend@chromium.org>
Commit-Queue: Darren Shen <shend@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548243}

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

chrome/browser/resource_coordinator/tab_manager_unittest.cc

@@ -135,10 +135,7 @@ enum TestIndicies {
 class TabManagerTest : public ChromeRenderViewHostTestHarness {
  public:
   TabManagerTest()
-      : scoped_context_(
-            std::make_unique<base::TestMockTimeTaskRunner::ScopedContext>(
-                task_runner_)),
-        scoped_set_tick_clock_for_testing_(task_runner_->GetMockTickClock()) {
+      : scoped_set_tick_clock_for_testing_(task_runner_->GetMockTickClock()) {
     base::MessageLoop::current()->SetTaskRunner(task_runner_);
   }
 
@@ -172,8 +169,7 @@ class TabManagerTest : public ChromeRenderViewHostTestHarness {
     contents2_.reset();
     contents3_.reset();
 
-    task_runner_->RunUntilIdle();
-    scoped_context_.reset();
+    base::MessageLoop::current()->SetTaskRunner(original_task_runner_);
     ChromeRenderViewHostTestHarness::TearDown();
   }
 
@@ -242,6 +238,10 @@ class TabManagerTest : public ChromeRenderViewHostTestHarness {
     }
   }
 
+ private:
+  scoped_refptr<base::SingleThreadTaskRunner> original_task_runner_ =
+      base::ThreadTaskRunnerHandle::Get();
+
  protected:
   std::unique_ptr<NavigationHandle> CreateTabAndNavigation(const char* url) {
     content::WebContents* web_contents = CreateTestWebContents();
@@ -253,7 +253,6 @@ class TabManagerTest : public ChromeRenderViewHostTestHarness {
   TabManager* tab_manager_ = nullptr;
   scoped_refptr<base::TestMockTimeTaskRunner> task_runner_ =
       base::MakeRefCounted<base::TestMockTimeTaskRunner>();
-  std::unique_ptr<base::TestMockTimeTaskRunner::ScopedContext> scoped_context_;
   ScopedSetTickClockForTesting scoped_set_tick_clock_for_testing_;
   std::unique_ptr<BackgroundTabNavigationThrottle> throttle1_;
   std::unique_ptr<BackgroundTabNavigationThrottle> throttle2_;

components/history/core/browser/history_model_worker_unittest.cc

@@ -70,16 +70,9 @@ class HistoryModelWorkerTest : public testing::Test {
   }
 
   ~HistoryModelWorkerTest() override {
-    // Run tasks that might still have a reference to |worker_|.
-    ui_thread_->RunUntilIdle();
-    history_thread_->RunUntilIdle();
-
-    // Release the last reference to |worker_|.
-    EXPECT_TRUE(worker_->HasOneRef());
+    // HistoryModelWorker posts a cleanup task to the UI thread in its
+    // destructor. Run it to prevent a leak.
     worker_ = nullptr;
-
-    // Run the DeleteSoon() task posted from ~HistoryModelWorker. This prevents
-    // a leak.
     ui_thread_->RunUntilIdle();
   }
 

gpu/ipc/host/shader_disk_cache_unittest.cc

@@ -35,14 +35,7 @@ class ShaderDiskCacheTest : public testing::Test {
   ShaderCacheFactory* factory() { return &factory_; }
 
  private:
-  void TearDown() override {
-    factory_.RemoveCacheInfo(kDefaultClientId);
-
-    // Run all pending tasks before destroying ScopedTaskEnvironment. Otherwise,
-    // SimpleEntryImpl instances bound to pending tasks are destroyed in an
-    // incorrect state (see |state_| DCHECK in ~SimpleEntryImpl).
-    scoped_task_environment_.RunUntilIdle();
-  }
+  void TearDown() override { factory_.RemoveCacheInfo(kDefaultClientId); }
 
   base::test::ScopedTaskEnvironment scoped_task_environment_;
   base::ScopedTempDir temp_dir_;