TLS crashes on Win8 when base::Singleton is used |
||||
Issue description
OS: Win8
The following code crashes on some Win8 machines if you comment the line that calls PlatformThreadLocalStorage::OnThreadExit().
#include <stdio.h>
#include "base/at_exit.h"
#include "base/memory/singleton.h"
#include "base/threading/thread_local_storage.h"
class C {
public:
// Return the singleton instance which will get destroyed by the AtExitMgr.
static C* GetInstance() {
return base::Singleton<C>::get();
}
void f() {
printf("C::f");
}
};
int main(int argc, char** argv) {
base::AtExitManager exit_manager;
C* c = C::GetInstance();
c->f();
// Comment the next line to reproduce crash.
base::internal::PlatformThreadLocalStorage::OnThreadExit();
return 0;
}
,
Mar 23 2017
We were only able to observe and reproduce the issue on some of the Win8 bots. And the issue only happens in Debug mode.
,
Mar 23 2017
The "crash" is more that NtTerminateProcess(-1, 0); is called during shutdown with the following stack: ntdll!NtTerminateProcess+0xc ntdll!RtlpWaitOnCriticalSection+0x58 ntdll!RtlpEnterCriticalSectionContended+0x148 ntdll!RtlEnterCriticalSection+0x43 unit_test!__acrt_lock+0x15 unit_test!_free_dbg+0x39 unit_test!operator delete+0xe unit_test!operator delete[]+0xc unit_test!`anonymous namespace'::OnThreadExitInternal+0x122 unit_test!base::internal::PlatformThreadLocalStorage::OnThreadExit+0x3e unit_test!OnThreadExit+0x14 ntdll!LdrxCallInitRoutine+0x16 ntdll!LdrpCallInitRoutine+0x60 ntdll!LdrpCallTlsInitializers+0x86 ntdll!LdrShutdownProcess+0x1fa ntdll!RtlExitUserProcess+0x5f KERNEL32!ExitProcessImplementation+0x12 unit_test!exit_or_terminate_process+0x38 unit_test!common_exit+0x19d unit_test!exit+0x12 unit_test!__scrt_common_main_seh+0x168 unit_test!__scrt_common_main+0xd unit_test!mainCRTStartup+0x8 KERNEL32!BaseThreadInitThunk+0xe ntdll!__RtlUserThreadStart+0x72 ntdll!_RtlUserThreadStart+0x1b This normally goes unnoticed, unless the process exit code matters.
,
Mar 23 2017
FWIU this is because we try to free TLS slots on shutdown after the CRT is uninitialized. Freeing TLS slots on shutdown is useless, but it is useful on short-lived threads, can we tell the difference? @robliao as TLS expert in Chromium.
,
Mar 23 2017
and it's specifically the line delete[] tls_data; from https://codesearch.chromium.org/chromium/src/base/threading/thread_local_storage.cc?l=182 that causes this behaviour. Hypothesis is that delete[] is being called after the CRT's allocator has shut down. I'm not sure why it is important to delete |tls_data| here? The process is about to shutdown, seems like leaking it shouldn't have bad side effects.
,
Mar 23 2017
The OnThreadExit handler here: https://codesearch.chromium.org/chromium/src/base/threading/thread_local_storage_win.cc?l=66 has a |reserved| parameter, which Raymond Chen suggests might be useful: https://blogs.msdn.microsoft.com/oldnewthing/20120105-00/?p=8683. Well, ok he's talking about DllMain, but maybe lpReserved also communicates "process is exiting" to the TLS destructor.
,
Mar 23 2017
> I'm not sure why it is important to delete |tls_data| here? The process is about to shutdown, seems like leaking it shouldn't have bad side effects. The main benefit here is for tests. We'll get charged for a leak upon termination.
,
Mar 23 2017
That and we do want to clean up tls_data on normal thread terminations (so we don't accumulate them).
,
Mar 23 2017
Re. tests: I don't think we'll get charged with a leak upon termination (leak detection happens before threads exit IIRC). I think main purpose is short-lived worker threads, but on shutdown we shouldn't bother if we can find a way to tell.
,
Aug 15 2017
This sounds vaguely like what we ran into in https://bugs.chromium.org/p/boringssl/issues/detail?id=45 way back.
,
Aug 15 2017
To that end, I believe the fix would not be to check the |reserved| parameter or anything, but just ignore DLL_PROCESS_DETACH. Only release thread-locals on DLL_THREAD_DETACH. |
||||
►
Sign in to add a comment |
||||
Comment 1 by scottmg@chromium.org
, Mar 23 2017