Skip to content

GitLab

T
tangled
Commit 2d84bccd authored by Etienne Bergeron's avatar Etienne Bergeron Committed by Commit Bot
Browse files
Options

Reland "Avoid running TLS destructors at background priority" 

This is a reland of 8e4ff348

The DCHECK in PlatformThreadLocalStorage::OnThreadExit(...)
was incorrect for unittest. To trigger the issue that caused
the revert of the original CL, an DCHECK needs to fail on
a background thread. It is not necessary to set back the priority
when the process is aborting.

Relanding without DCHECK to get the fix right away. Will find another
way than DCHECK of verifying code correctness since _exit() can
legitimately be called by any code and hit that code path.
(see: crbug/1097092 for followup)

See:

  void Dummy() { DCHECK(false); }

  TEST(Broken, Priority) {
    base::test::TaskEnvironment task_environment_;
    base::ThreadPool::PostTask(
        FROM_HERE, {base::TaskPriority::BEST_EFFORT, base::MayBlock()},
        base::BindOnce(&Dummy));
  }

This broken unittest will produce the following output:

[ RUN      ] Broken.Priority
[16064:10444:0619/025028.262:543712406:FATAL:storage_queue_unittest.cc(30)] Check failed: false.
Backtrace:
        base::debug::CollectStackTrace [0x00007FF90564E4B2+18] (C:\src\chromium\src\base\debug\stack_trace_win.cc:284)
        base::debug::StackTrace::StackTrace [0x00007FF9055568F2+18] (C:\src\chromium\src\base\debug\stack_trace.cc:203)
        logging::LogMessage::~LogMessage [0x00007FF90556EE34+148] (C:\src\chromium\src\base\logging.cc:605)
        logging::LogMessage::~LogMessage [0x00007FF90556FBE0+16] (C:\src\chromium\src\base\logging.cc:598)
        Dummy [0x00007FF731A8E7A4+72] (C:\src\chromium\src\chrome\browser\policy\messaging_layer\storage\storage_queue_unittest.cc:30)
        base::TaskAnnotator::RunTask [0x00007FF9055DC156+454] (C:\src\chromium\src\base\task\common\task_annotator.cc:142)
        base::internal::TaskTracker::RunSkipOnShutdown [0x00007FF905614397+23] (C:\src\chromium\src\base\task\thread_pool\task_tracker.cc:769)
        base::internal::TaskTracker::RunTask [0x00007FF905613AA7+1335] (C:\src\chromium\src\base\task\thread_pool\task_tracker.cc:636)
        base::test::TaskEnvironment::TestTaskTracker::RunTask [0x00007FF734BF64D3+231] (C:\src\chromium\src\base\test\task_environment.cc:783)
        base::internal::TaskTracker::RunAndPopNextTask [0x00007FF9056131C2+498] (C:\src\chromium\src\base\task\thread_pool\task_tracker.cc:507)
        base::internal::WorkerThread::RunWorker [0x00007FF90562165C+1036] (C:\src\chromium\src\base\task\thread_pool\worker_thread.cc:349)
        base::internal::WorkerThread::RunBackgroundPooledWorker [0x00007FF905620FE8+24] (C:\src\chromium\src\base\task\thread_pool\worker_thread.cc:232)
        base::`anonymous namespace'::ThreadFunc [0x00007FF905667D0B+219] (C:\src\chromium\src\base\threading\platform_thread_win.cc:114)
        BaseThreadInitThunk [0x00007FF962404034+20]
        RtlUserThreadStart [0x00007FF964443691+33]
Task trace:
Backtrace:
        Broken_Priority_Test::TestBody [0x00007FF731A8E851+153] (C:\src\chromium\src\chrome\browser\policy\messaging_layer\storage\storage_queue_unittest.cc:35)

[16064:10444:0619/025030.124:543714281:FATAL:thread_local_storage.cc(333)] Check failed: PlatformThread::GetCurrentThreadPriority() >= ThreadPriority::NORMAL (0 vs. 1)
Backtrace:
        base::debug::CollectStackTrace [0x00007FF90564E4B2+18] (C:\src\chromium\src\base\debug\stack_trace_win.cc:284)
        base::debug::StackTrace::StackTrace [0x00007FF9055568F2+18] (C:\src\chromium\src\base\debug\stack_trace.cc:203)
        logging::LogMessage::~LogMessage [0x00007FF90556EE34+148] (C:\src\chromium\src\base\logging.cc:605)
        logging::LogMessage::~LogMessage [0x00007FF90556FBE0+16] (C:\src\chromium\src\base\logging.cc:598)
        base::internal::PlatformThreadLocalStorage::OnThreadExit [0x00007FF9056322DF+175] (C:\src\chromium\src\base\threading\thread_local_storage.cc:335)
        RtlActivateActivationContextUnsafeFast [0x00007FF96440B583+291]
        RtlActivateActivationContextUnsafeFast [0x00007FF96440B67F+543]
        LdrShutdownProcess [0x00007FF964417F70+272]
        RtlExitUserProcess [0x00007FF964417E48+216]
        FatalExit [0x00007FF96240CA7A+10]
        exit [0x00007FF96156CC08+200]
        exit [0x00007FF96156CBBB+123]
        base::TestSuite::UnitTestAssertHandler [0x00007FF734BFB748+230] (C:\src\chromium\src\base\test\test_suite.cc:518)
        base::internal::Invoker<base::internal::BindState<void (base::TestSuite::*)(const char *, int, base::BasicStringPiece<std::__1::basic_string<char,std::__1::char_traits<char>,std::__1::allocator<char> > >, base::BasicStringPiece<std::__1::basic_string<char [0x00007FF734BFD2DE+68] (C:\src\chromium\src\base\bind_internal.h:691)
        logging::LogMessage::~LogMessage [0x00007FF90556F342+1442] (C:\src\chromium\src\base\logging.cc:914)
        logging::LogMessage::~LogMessage [0x00007FF90556FBE0+16] (C:\src\chromium\src\base\logging.cc:598)
        Dummy [0x00007FF731A8E7A4+72] (C:\src\chromium\src\chrome\browser\policy\messaging_layer\storage\storage_queue_unittest.cc:30)
        base::TaskAnnotator::RunTask [0x00007FF9055DC156+454] (C:\src\chromium\src\base\task\common\task_annotator.cc:142)
        base::internal::TaskTracker::RunSkipOnShutdown [0x00007FF905614397+23] (C:\src\chromium\src\base\task\thread_pool\task_tracker.cc:769)
        base::internal::TaskTracker::RunTask [0x00007FF905613AA7+1335] (C:\src\chromium\src\base\task\thread_pool\task_tracker.cc:636)
        base::test::TaskEnvironment::TestTaskTracker::RunTask [0x00007FF734BF64D3+231] (C:\src\chromium\src\base\test\task_environment.cc:783)
        base::internal::TaskTracker::RunAndPopNextTask [0x00007FF9056131C2+498] (C:\src\chromium\src\base\task\thread_pool\task_tracker.cc:507)
        base::internal::WorkerThread::RunWorker [0x00007FF90562165C+1036] (C:\src\chromium\src\base\task\thread_pool\worker_thread.cc:349)
        base::internal::WorkerThread::RunBackgroundPooledWorker [0x00007FF905620FE8+24] (C:\src\chromium\src\base\task\thread_pool\worker_thread.cc:232)
        base::`anonymous namespace'::ThreadFunc [0x00007FF905667D0B+219] (C:\src\chromium\src\base\threading\platform_thread_win.cc:114)
        BaseThreadInitThunk [0x00007FF962404034+20]
        RtlUserThreadStart [0x00007FF964443691+33]

Original change's description:
> Avoid running TLS destructors at background priority
>
> This CL is adding a ScopedThreadPriority that save the current
> thread priority and restore it when leaving the scope. This is
> required since it is possible to run code outside of the Run(...)
> scope with Background priority.
>
> On Windows, this is causing shutdown hangs because the LoaderLock
> can be held by a background thread while doing TLS destruction.
>
>
> Without this fix, the DCHECK in the code run with Background priority
> and fails:
>
> [9888:28656:0617/210735.064:FATAL:thread_local_storage.cc(331)] Check failed: PlatformThread::GetCurrentThreadPriority() != ThreadPriority::BACKGROUND (0 vs. 0)
> Backtrace:
>         base::debug::CollectStackTrace [0x00007FF92445E7D2+18] (C:\src\chromium\src\base\debug\stack_trace_win.cc:284)
>         base::debug::StackTrace::StackTrace [0x00007FF9243668F2+18] (C:\src\chromium\src\base\debug\stack_trace.cc:203)
>         logging::LogMessage::~LogMessage [0x00007FF92437F094+148] (C:\src\chromium\src\base\logging.cc:605)
>         logging::LogMessage::~LogMessage [0x00007FF92437FE40+16] (C:\src\chromium\src\base\logging.cc:598)
>         base::internal::PlatformThreadLocalStorage::OnThreadExit [0x00007FF9244425FA+170] (C:\src\chromium\src\base\threading\thread_local_storage.cc:333)
>         RtlActivateActivationContextUnsafeFast [0x00007FF96440B583+291]
>         RtlActivateActivationContextUnsafeFast [0x00007FF96440B67F+543]
>         LdrShutdownThread [0x00007FF964408B8C+348]
>         RtlExitUserThread [0x00007FF9644436FE+62]
>         BaseThreadInitThunk [0x00007FF96240403C+28]
>         RtlUserThreadStart [0x00007FF964443691+33]
>
> Bug: 1096203
> Change-Id: Ibc011ac370dde4b6a872da6ac81938f52965a5a6
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2251199
> Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Reviewed-by: François Doray <fdoray@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#779932}

Bug: 1096203
Change-Id: Ia0a5e01cd4d0918809b5262b2f6547880c0b5a03
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2252880
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Reviewed-by: default avatarGabriel Charette <gab@chromium.org>
Reviewed-by: default avatarFrançois Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#780344}
parent e53dbe17
Hide whitespace changes
Inline Side-by-side
Showing with 8 additions and 0 deletions
base/threading/platform_thread_win.cc
View file @ 2d84bccd
...@@ -16,6 +16,7 @@ ...@@ -16,6 +16,7 @@
#include "base/strings/string_number_conversions.h" #include "base/strings/string_number_conversions.h"
#include "base/strings/utf_string_conversions.h" #include "base/strings/utf_string_conversions.h"
#include "base/threading/scoped_blocking_call.h" #include "base/threading/scoped_blocking_call.h"
#include "base/threading/scoped_thread_priority.h"
#include "base/threading/thread_id_name_manager.h" #include "base/threading/thread_id_name_manager.h"
#include "base/threading/thread_restrictions.h" #include "base/threading/thread_restrictions.h"
#include "base/time/time_override.h" #include "base/time/time_override.h"
...@@ -115,6 +116,13 @@ DWORD __stdcall ThreadFunc(void* params) { ...@@ -115,6 +116,13 @@ DWORD __stdcall ThreadFunc(void* params) {
PlatformThread::CurrentId()); PlatformThread::CurrentId());
} }
// Ensure thread priority is at least NORMAL before initiating thread
// destruction. Thread destruction on Windows holds the LdrLock while
// performing TLS destruction which causes hangs if performed at background
// priority (priority inversion) (see: http://crbug.com/1096203).
if (PlatformThread::GetCurrentThreadPriority() < ThreadPriority::NORMAL)
PlatformThread::SetCurrentThreadPriority(ThreadPriority::NORMAL);
return 0; return 0;
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment