diff --git a/base/task/bind_post_task_internal.h b/base/task/bind_post_task_internal.h index f0034bf9db426..e2915eb4c6c48 100644 --- a/base/task/bind_post_task_internal.h +++ b/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. diff --git a/base/task/sequenced_task_runner.cc b/base/task/sequenced_task_runner.cc index 702dc22109ecb..c1323467b5bc6 100644 --- a/base/task/sequenced_task_runner.cc +++ b/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; } diff --git a/base/task/sequenced_task_runner.h b/base/task/sequenced_task_runner.h index 33213f979d8c0..4ea5510fb8038 100644 --- a/base/task/sequenced_task_runner.h +++ b/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. diff --git a/base/task/thread_pool/pooled_single_thread_task_runner_manager.cc b/base/task/thread_pool/pooled_single_thread_task_runner_manager.cc index 23d88f32ba83f..6585aff566550 100644 --- a/base/task/thread_pool/pooled_single_thread_task_runner_manager.cc +++ b/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(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(std::move(task)); + ANNOTATE_LEAKING_OBJECT_PTR(leak.get()); + leak.release(); return false; } diff --git a/base/task/thread_pool/task_tracker.cc b/base/task/thread_pool/task_tracker.cc index 98f316c71d3cd..8dfae92c34190 100644 --- a/base/task/thread_pool/task_tracker.cc +++ b/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 + 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) { diff --git a/base/task/thread_pool/task_tracker.h b/base/task/thread_pool/task_tracker.h index ce86c56c522e9..508ac6eb48df3 100644 --- a/base/task/thread_pool/task_tracker.h +++ b/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. diff --git a/base/task/thread_pool/task_tracker_unittest.cc b/base/task/thread_pool/task_tracker_unittest.cc index 55b8964022889..df0b6509c1e91 100644 --- a/base/task/thread_pool/task_tracker_unittest.cc +++ b/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())); diff --git a/base/task/thread_pool/test_utils.cc b/base/task/thread_pool/test_utils.cc index 46e4cbd72d24c..63f8ea2e0219d 100644 --- a/base/task/thread_pool/test_utils.cc +++ b/base/task/thread_pool/test_utils.cc @@ -7,6 +7,7 @@ #include #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(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)); diff --git a/base/task/thread_pool/thread_pool_impl.cc b/base/task/thread_pool/thread_pool_impl.cc index ad911cdd90fda..d6177c83b2539 100644 --- a/base/task/thread_pool/thread_pool_impl.cc +++ b/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) { 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(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)); diff --git a/base/task/thread_pool/thread_pool_impl.h b/base/task/thread_pool/thread_pool_impl.h index b1a21edcdef75..ebc0833adff81 100644 --- a/base/task/thread_pool/thread_pool_impl.h +++ b/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, diff --git a/base/task/thread_pool/thread_pool_instance.cc b/base/task/thread_pool/thread_pool_instance.cc index d5173c7afb9dd..c68874ab0a6a7 100644 --- a/base/task/thread_pool/thread_pool_instance.cc +++ b/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) { diff --git a/base/task/thread_pool/thread_pool_instance.h b/base/task/thread_pool/thread_pool_instance.h index 4f0214661a060..2366076ef0ae4 100644 --- a/base/task/thread_pool/thread_pool_instance.h +++ b/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. diff --git a/base/task/thread_pool_unittest.cc b/base/task/thread_pool_unittest.cc index e7e7482f71b9f..da265c5235bf8 100644 --- a/base/task/thread_pool_unittest.cc +++ b/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 diff --git a/base/threading/post_task_and_reply_impl.cc b/base/threading/post_task_and_reply_impl.cc index a346c5944cd85..65292faf16292 100644 --- a/base/threading/post_task_and_reply_impl.cc +++ b/base/threading/post_task_and_reply_impl.cc @@ -11,6 +11,8 @@ #include "base/debug/leak_annotations.h" #include "base/memory/ref_counted.h" #include "base/task/sequenced_task_runner.h" +#include "base/task/thread_pool/thread_pool_instance.h" +#include "base/threading/sequenced_task_runner_handle.h" namespace base { @@ -73,6 +75,12 @@ class PostTaskAndReplyRelay { // Case 2: if (!reply_task_runner_->RunsTasksInCurrentSequence()) { DCHECK(reply_); + // Allow this task to be leaked on shutdown even if `reply_task_runner_` + // has the TaskShutdownBehaviour::BLOCK_SHUTDOWN trait. Without `fizzler`, + // such a task runner would DCHECK when posting to `reply_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; SequencedTaskRunner* reply_task_runner_raw = reply_task_runner_.get(); auto relay_to_delete =