Introduce ScopedFizzleBlockShutdownTasks

This allows PostTaskAndReplyRelay and BindPostTaskTrampoline to signal
that tasks posted to delete a task's reply can be dropped if posted
after shutdown, even if the originating sequence is BLOCK_SHUTDOWN.

This can happen if a BLOCK_SHUTDOWN sequence posts a task to the main
thread and that task doesn't get run during shutdown. In that case, the
thread pool would be shutdown and the main thread's task queue deleted.
When this happens, any task that has a reply would then trigger a task
to be posted to the originating sequence (BLOCK_SHUTDOWN) to destroy
the reply, but posting a BLOCK_SHUTDOWN task after shutdown is invalid
and triggers a  DCHECK.

Bug: 1386236,1375270
Change-Id: I940a062f75cf4defc2c3f3ce0f85fc94de18c211
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4054884
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Anthony Vallée-Dubois <anthonyvd@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1084519}

base/task/bind_post_task_internal.h

@@ -12,6 +12,7 @@
 #include "base/check.h"
 #include "base/location.h"
 #include "base/task/task_runner.h"
+#include "base/task/thread_pool/thread_pool_instance.h"
 
 namespace base {
 namespace internal {
@@ -39,6 +40,13 @@ class BindPostTaskTrampoline {
 
   ~BindPostTaskTrampoline() {
     if (callback_) {
+      // Allow this task to be leaked on shutdown even if `task_runner_` has the
+      // TaskShutdownBehaviour::BLOCK_SHUTDOWN trait. Without `fizzler`, such a
+      // task runner would DCHECK when posting to `task_runner_` after shutdown.
+      // Ignore this DCHECK as the poster isn't in control when its Callback is
+      // destroyed late into shutdown. Ref. crbug.com/1375270.
+      base::ThreadPoolInstance::ScopedFizzleBlockShutdownTasks fizzler;
+
       // 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.

base/task/sequenced_task_runner.cc

@@ -45,10 +45,7 @@ DelayedTaskHandle SequencedTaskRunner::PostCancelableDelayedTask(
   DelayedTaskHandle delayed_task_handle(
       std::move(delayed_task_handle_delegate));
 
-  // If the task fails to be posted, the handle will automatically be
-  // invalidated upon destruction of the callback object.
-  if (!PostDelayedTask(from_here, std::move(task), delay))
-    DCHECK(!delayed_task_handle.IsValid());
+  PostDelayedTask(from_here, std::move(task), delay);
 
   return delayed_task_handle;
 }

base/task/sequenced_task_runner.h

@@ -181,10 +181,10 @@ class BASE_EXPORT SequencedTaskRunner : public TaskRunner {
   // directly. Consider using higher level timer primitives in
   // base/timer/timer.h.
   //
-  // The handle is only valid while the task is pending execution. This means
-  // that it will be invalid if the posting failed, and will be invalid while
-  // the task is executing. Calling CancelTask() on an invalid handle is a
-  // no-op.
+  // The handle is only guaranteed valid while the task is pending execution.
+  // This means that it may be invalid if the posting failed, and will be
+  // invalid while the task is executing. Calling CancelTask() on an invalid
+  // handle is a no-op.
   //
   // This method and the handle it returns are not thread-safe and can only be
   // used from the sequence this task runner runs its tasks on.

base/task/thread_pool/pooled_single_thread_task_runner_manager.cc

@@ -10,6 +10,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/debug/leak_annotations.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/raw_ptr.h"
 #include "base/ranges/algorithm.h"
@@ -379,6 +380,12 @@ class WorkerThreadCOMDelegate : public WorkerThreadDelegate {
           return nullptr;
         transaction.PushImmediateTask(std::move(pump_message_task));
         return registered_task_source;
+      } else {
+        // `pump_message_task`'s destructor may run sequence-affine code, so it
+        // must be leaked when `WillPostTask` returns false.
+        auto leak = std::make_unique<Task>(std::move(pump_message_task));
+        ANNOTATE_LEAKING_OBJECT_PTR(leak.get());
+        leak.release();
       }
     }
     return nullptr;
@@ -486,6 +493,11 @@ class PooledSingleThreadTaskRunnerManager::PooledSingleThreadTaskRunner
   bool PostTask(Task task) {
     if (!outer_->task_tracker_->WillPostTask(&task,
                                              sequence_->shutdown_behavior())) {
+      // `task`'s destructor may run sequence-affine code, so it must be leaked
+      // when `WillPostTask` returns false.
+      auto leak = std::make_unique<Task>(std::move(task));
+      ANNOTATE_LEAKING_OBJECT_PTR(leak.get());
+      leak.release();
       return false;
     }
 

base/task/thread_pool/task_tracker.cc

@@ -120,6 +120,12 @@ auto EmitThreadPoolTraceEventMetadata(perfetto::EventContext& ctx,
 #endif  //  BUILDFLAG(ENABLE_BASE_TRACING)
 }
 
+base::ThreadLocalBoolean& GetFizzleBlockShutdownTaskFlag() {
+  static base::NoDestructor<base::ThreadLocalBoolean>
+      fizzle_block_shutdown_tasks;
+  return *fizzle_block_shutdown_tasks;
+}
+
 }  // namespace
 
 // Atomic internal state used by TaskTracker to track items that are blocking
@@ -311,12 +317,14 @@ bool TaskTracker::WillPostTask(Task* task,
     // A non BLOCK_SHUTDOWN task is allowed to be posted iff shutdown hasn't
     // started and the task is not delayed.
     if (shutdown_behavior != TaskShutdownBehavior::BLOCK_SHUTDOWN ||
-        !task->delayed_run_time.is_null()) {
+        !task->delayed_run_time.is_null() ||
+        GetFizzleBlockShutdownTaskFlag().Get()) {
       return false;
     }
 
-    // A BLOCK_SHUTDOWN task posted after shutdown has completed is an
-    // ordering bug. This aims to catch those early.
+    // A BLOCK_SHUTDOWN task posted after shutdown has completed without setting
+    // `fizzle_block_shutdown_tasks` is an ordering bug. This aims to catch
+    // those early.
     CheckedAutoLock auto_lock(shutdown_lock_);
     DCHECK(shutdown_event_);
     DCHECK(!shutdown_event_->IsSignaled())
@@ -414,6 +422,14 @@ bool TaskTracker::IsShutdownComplete() const {
   return shutdown_event_ && shutdown_event_->IsSignaled();
 }
 
+void TaskTracker::BeginFizzlingBlockShutdownTasks() {
+  GetFizzleBlockShutdownTaskFlag().Set(true);
+}
+
+void TaskTracker::EndFizzlingBlockShutdownTasks() {
+  GetFizzleBlockShutdownTaskFlag().Set(false);
+}
+
 void TaskTracker::RunTask(Task task,
                           TaskSource* task_source,
                           const TaskTraits& traits) {

base/task/thread_pool/task_tracker.h

@@ -26,6 +26,7 @@
 #include "base/task/thread_pool/task_source.h"
 #include "base/task/thread_pool/tracked_ref.h"
 #include "base/thread_annotations.h"
+#include "base/threading/thread_local.h"
 
 namespace base {
 
@@ -95,6 +96,9 @@ class BASE_EXPORT TaskTracker {
   // DelayedTaskManager (if delayed). Returns true if this operation is allowed
   // (the operation should be performed if-and-only-if it is). This method may
   // also modify metadata on |task| if desired.
+  // If this returns false, `task` must be leaked by the caller if deleting it
+  // on the current sequence may invoke sequence-affine code that belongs to
+  // another sequence.
   bool WillPostTask(Task* task, TaskShutdownBehavior shutdown_behavior);
 
   // Informs this TaskTracker that |task| that is about to be pushed to a task
@@ -133,6 +137,9 @@ class BASE_EXPORT TaskTracker {
     return tracked_ref_factory_.GetTrackedRef();
   }
 
+  void BeginFizzlingBlockShutdownTasks();
+  void EndFizzlingBlockShutdownTasks();
+
   // Returns true if there are task sources that haven't completed their
   // execution (still queued or in progress). If it returns false: the side-
   // effects of all completed tasks are guaranteed to be visible to the caller.

base/task/thread_pool/task_tracker_unittest.cc

@@ -472,6 +472,15 @@ TEST_P(ThreadPoolTaskTrackerTest, WillPostAfterShutdown) {
 
   // |task_tracker_| shouldn't allow a task to be posted after shutdown.
   if (GetParam() == TaskShutdownBehavior::BLOCK_SHUTDOWN) {
+    // When the task tracker is allowed to fizzle block shutdown tasks,
+    // WillPostTask will return false and leak the task.
+    tracker_.BeginFizzlingBlockShutdownTasks();
+    EXPECT_FALSE(tracker_.WillPostTask(&task, GetParam()));
+    tracker_.EndFizzlingBlockShutdownTasks();
+
+    // If a BLOCK_SHUTDOWN task is posted after shutdown without explicitly
+    // allowing BLOCK_SHUTDOWN task fizzling, WillPostTask DCHECKs to find
+    // ordering bugs.
     EXPECT_DCHECK_DEATH(tracker_.WillPostTask(&task, GetParam()));
   } else {
     EXPECT_FALSE(tracker_.WillPostTask(&task, GetParam()));

base/task/thread_pool/test_utils.cc

@@ -7,6 +7,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "base/debug/leak_annotations.h"
 #include "base/memory/raw_ptr.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/task/thread_pool/pooled_parallel_task_runner.h"
@@ -164,8 +165,14 @@ bool MockPooledTaskRunnerDelegate::PostTaskWithSequence(
   DCHECK(task.task);
   DCHECK(sequence);
 
-  if (!task_tracker_->WillPostTask(&task, sequence->shutdown_behavior()))
+  if (!task_tracker_->WillPostTask(&task, sequence->shutdown_behavior())) {
+    // `task`'s destructor may run sequence-affine code, so it must be leaked
+    // when `WillPostTask` returns false.
+    auto leak = std::make_unique<Task>(std::move(task));
+    ANNOTATE_LEAKING_OBJECT_PTR(leak.get());
+    leak.release();
     return false;
+  }
 
   if (task.delayed_run_time.is_null()) {
     PostTaskWithSequenceNow(std::move(task), std::move(sequence));

base/task/thread_pool/thread_pool_impl.cc

@@ -14,6 +14,7 @@
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/debug/alias.h"
+#include "base/debug/leak_annotations.h"
 #include "base/feature_list.h"
 #include "base/message_loop/message_pump_type.h"
 #include "base/metrics/field_trial_params.h"
@@ -386,6 +387,14 @@ void ThreadPoolImpl::EndBestEffortFence() {
   UpdateCanRunPolicy();
 }
 
+void ThreadPoolImpl::BeginFizzlingBlockShutdownTasks() {
+  task_tracker_->BeginFizzlingBlockShutdownTasks();
+}
+
+void ThreadPoolImpl::EndFizzlingBlockShutdownTasks() {
+  task_tracker_->EndFizzlingBlockShutdownTasks();
+}
+
 bool ThreadPoolImpl::PostTaskWithSequenceNow(Task task,
                                              scoped_refptr<Sequence> sequence) {
   auto transaction = sequence->BeginTransaction();
@@ -424,8 +433,14 @@ bool ThreadPoolImpl::PostTaskWithSequence(Task task,
   DEBUG_ALIAS_FOR_CSTR(task_posted_from, task.posted_from.file_name(), 32);
 #endif
 
-  if (!task_tracker_->WillPostTask(&task, sequence->shutdown_behavior()))
+  if (!task_tracker_->WillPostTask(&task, sequence->shutdown_behavior())) {
+    // `task`'s destructor may run sequence-affine code, so it must be leaked
+    // when `WillPostTask` returns false.
+    auto leak = std::make_unique<Task>(std::move(task));
+    ANNOTATE_LEAKING_OBJECT_PTR(leak.get());
+    leak.release();
     return false;
+  }
 
   if (task.delayed_run_time.is_null()) {
     return PostTaskWithSequenceNow(std::move(task), std::move(sequence));

base/task/thread_pool/thread_pool_impl.h

@@ -76,6 +76,8 @@ class BASE_EXPORT ThreadPoolImpl : public ThreadPoolInstance,
   void EndFence() override;
   void BeginBestEffortFence() override;
   void EndBestEffortFence() override;
+  void BeginFizzlingBlockShutdownTasks() override;
+  void EndFizzlingBlockShutdownTasks() override;
 
   // TaskExecutor:
   bool PostDelayedTask(const Location& from_here,

base/task/thread_pool/thread_pool_instance.cc

@@ -66,6 +66,20 @@ ThreadPoolInstance::ScopedBestEffortExecutionFence::
   g_thread_pool->EndBestEffortFence();
 }
 
+ThreadPoolInstance::ScopedFizzleBlockShutdownTasks::
+    ScopedFizzleBlockShutdownTasks() {
+  // It's possible for this to be called without a ThreadPool present in tests.
+  if (g_thread_pool)
+    g_thread_pool->BeginFizzlingBlockShutdownTasks();
+}
+
+ThreadPoolInstance::ScopedFizzleBlockShutdownTasks::
+    ~ScopedFizzleBlockShutdownTasks() {
+  // It's possible for this to be called without a ThreadPool present in tests.
+  if (g_thread_pool)
+    g_thread_pool->EndFizzlingBlockShutdownTasks();
+}
+
 #if !BUILDFLAG(IS_NACL)
 // static
 void ThreadPoolInstance::CreateAndStartWithDefaultParams(StringPiece name) {

base/task/thread_pool/thread_pool_instance.h

@@ -123,6 +123,19 @@ class BASE_EXPORT ThreadPoolInstance {
     ~ScopedBestEffortExecutionFence();
   };
 
+  // Used to allow posting `BLOCK_SHUTDOWN` tasks after shutdown in a scope. The
+  // tasks will fizzle (not run) but not trigger any checks that aim to catch
+  // this class of ordering bugs.
+  class BASE_EXPORT ScopedFizzleBlockShutdownTasks {
+   public:
+    ScopedFizzleBlockShutdownTasks();
+    ScopedFizzleBlockShutdownTasks(const ScopedFizzleBlockShutdownTasks&) =
+        delete;
+    ScopedFizzleBlockShutdownTasks& operator=(
+        const ScopedFizzleBlockShutdownTasks&) = delete;
+    ~ScopedFizzleBlockShutdownTasks();
+  };
+
   // Destroying a ThreadPoolInstance is not allowed in production; it is always
   // leaked. In tests, it should only be destroyed after JoinForTesting() has
   // returned.
@@ -180,6 +193,9 @@ class BASE_EXPORT ThreadPoolInstance {
   // after this call.
   virtual void JoinForTesting() = 0;
 
+  virtual void BeginFizzlingBlockShutdownTasks() = 0;
+  virtual void EndFizzlingBlockShutdownTasks() = 0;
+
   // CreateAndStartWithDefaultParams(), Create(), and SetInstance() register a
   // ThreadPoolInstance to handle tasks posted through the thread_pool.h API for
   // this process.

base/task/thread_pool_unittest.cc

@@ -7,8 +7,12 @@
 #include "base/bind.h"
 #include "base/location.h"
 #include "base/run_loop.h"
+#include "base/task/bind_post_task.h"
+#include "base/task/single_thread_task_executor.h"
+#include "base/task/thread_pool/thread_pool_instance.h"
 #include "base/test/bind.h"
 #include "base/test/task_environment.h"
+#include "base/test/test_waitable_event.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
@@ -41,4 +45,86 @@ TEST(ThreadPool, PostTaskAndReplyWithResultFourArgs) {
   run_loop.Run();
 }
 
+TEST(ThreadPool, BindPostTaskFizzlesOnShutdown) {
+  // Tests that a callback bound to a BLOCK_SHUTDOWN sequence doesn't trigger a
+  // DCHECK when it's deleted without running.
+  base::ThreadPoolInstance::CreateAndStartWithDefaultParams(
+      "BindPostTaskFizzlesOnShutdownTest");
+
+  {
+    // Creating this callback and deleting it after the thread pool is shutdown
+    // used to trigger a DCHECK in task_tracker because the
+    // BindPostTaskTrampoline destructor has to delete its state on the sequence
+    // it's bound to. There is a DCHECK that ensures BLOCK_SHUTDOWN tasks aren't
+    // posted after shutdown, but BindPostTaskTrampoline uses
+    // base::ThreadPoolInstance::ScopedFizzleBlockShutdownTasks to avoid
+    // triggering it.
+    auto bound_callback =
+        base::BindPostTask(base::ThreadPool::CreateSequencedTaskRunner(
+                               {TaskShutdownBehavior::BLOCK_SHUTDOWN}),
+                           base::BindOnce([]() { ADD_FAILURE(); }));
+    base::ThreadPoolInstance::Get()->Shutdown();
+  }
+
+  ThreadPoolInstance::Get()->JoinForTesting();
+  ThreadPoolInstance::Set(nullptr);
+}
+
+TEST(ThreadPool, PostTaskAndReplyFizzlesOnShutdown) {
+  // Tests that a PostTaskAndReply from a BLOCK_SHUTDOWN sequence doesn't
+  // trigger a DCHECK when it's not run at shutdown.
+  base::ThreadPoolInstance::CreateAndStartWithDefaultParams(
+      "PostTaskAndReplyFizzlesOnShutdown");
+
+  {
+    base::SingleThreadTaskExecutor executor;
+    auto blocking_task_runner = base::ThreadPool::CreateSequencedTaskRunner(
+        {TaskShutdownBehavior::BLOCK_SHUTDOWN});
+
+    base::RunLoop run_loop;
+
+    // The setup that this test is exercising is as follows:
+    // - A task is posted using PostTaskAndReply from a BLOCK_SHUTDOWN sequence
+    // to the main thread
+    // - The task is not run on the main thread
+    // - Shutdown happens, the ThreadPool is shutdown
+    // - The main thread destroys its un-run tasks
+    // - ~PostTaskAndReplyRelay calls `DeleteSoon` to delete its reply's state
+    // on the sequence it's bound to
+    // - TaskTracker ensures that no BLOCK_SHUTDOWN tasks can be posted after
+    // shutdown. ~PostTaskAndReplyRelay avoids triggering this DCHECK by using a
+    // base::ThreadPoolInstance::ScopedFizzleBlockShutdownTasks
+
+    // Post a task to the BLOCK_SHUTDOWN thread pool sequence to setup the test.
+    base::TestWaitableEvent event;
+    blocking_task_runner->PostTask(
+        FROM_HERE, base::BindLambdaForTesting([&]() {
+          // Enqueue a task whose only purpose is to exit the run loop, ensuring
+          // the following task is never run.
+          executor.task_runner()->PostTask(FROM_HERE,
+                                           base::BindLambdaForTesting([&]() {
+                                             event.Wait();
+                                             run_loop.Quit();
+                                           }));
+
+          // Post the task for which the reply will trigger the `DeleteSoon`
+          // from `~PostTaskAndReplyRelay`.
+          executor.task_runner()->PostTaskAndReply(
+              FROM_HERE, base::BindOnce([]() { ADD_FAILURE(); }),
+              base::BindOnce([]() { ADD_FAILURE(); }));
+
+          event.Signal();
+        }));
+
+    // Run until the first task posted to the SingleThreadTaskExecutor quits the
+    // run loop, resulting in the `PostTaskAndReply` being queued but not run.
+    run_loop.Run();
+
+    base::ThreadPoolInstance::Get()->Shutdown();
+  }
+
+  ThreadPoolInstance::Get()->JoinForTesting();
+  ThreadPoolInstance::Set(nullptr);
+}
+
 }  // namespace base