Issue metadata
Sign in to add a comment
|
MemoryTracingIntegrationTest fails on TSAN |
||||||||||||||||||||||||
Issue descriptionTrying to enable services_unittests with TSAN reveals some (test only?) races: https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_tsan_rel_ng/256937 [ RUN ] MemoryTracingIntegrationTest.GenerationChangeDoesntReenterMDM ================== WARNING: ThreadSanitizer: data race (pid=9676) Write of size 8 at 0x000008f746d8 by main thread: #0 OnExit base/memory/singleton.h:259:15 (services_unittests+0x369ead8) #1 Delete base/trace_event/trace_log.cc:57 (services_unittests+0x369ead8) #2 base::trace_event::TraceLog::DeleteForTesting() base/trace_event/trace_log.cc:1539 (services_unittests+0x369ead8) #3 memory_instrumentation::MemoryTracingIntegrationTest::TearDown() services/resource_coordinator/public/cpp/memory_instrumentation/tracing_integration_unittest.cc:162:5 (services_unittests+0x16b9310) #4 testing::Test::Run() third_party/googletest/src/googletest/src/gtest.cc (services_unittests+0x1e3f73a) ... Previous atomic read of size 8 at 0x000008f746d8 by thread T1: #0 __tsan_atomic64_load /b/build/slave/linux_upload_clang/build/src/third_party/llvm/compiler-rt/lib/tsan/rtl/tsan_interface_atomic.cc:540:3 (services_unittests+0x1321c70) #1 load buildtools/third_party/libc++/trunk/include/atomic:922:17 (services_unittests+0x36963d4) #2 Acquire_Load base/atomicops_internals_portable.h:217 (services_unittests+0x36963d4) #3 GetOrCreateLazyPointer<base::trace_event::TraceLog> base/lazy_instance_helpers.h:77 (services_unittests+0x36963d4) #4 base::Singleton<base::trace_event::TraceLog, base::LeakySingletonTraits<base::trace_event::TraceLog>, base::trace_event::TraceLog>::get() base/memory/singleton.h:243 (services_unittests+0x36963d4) #5 GetInstance base/trace_event/trace_log.cc:342:10 (services_unittests+0x36972df) #6 base::trace_event::TraceLog::GetCategoryGroupEnabled(char const*) base/trace_event/trace_log.cc:425 (services_unittests+0x36972df) #7 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) base/debug/task_annotator.cc:32:3 (services_unittests+0x35b424c) ... Thread T1 'IPC thread' (tid=9678, running) created by main thread ...
,
Feb 23 2018
So indeed this is a testing-only race. I think that there is no real race (there should be no threads active at that point) but what happens is that TSan observes: - a thread (that ends before the test reaches TearDown()) accessing the instance pointer in GetOrCreateLazyPointer with a load-acquire - clearing up the instance pointer (in TearDown()) with a non-barriered write so it says "at some point you will have a race here" I was almost tempted to file a suppression. But then I realized that years ago, young myself implemented this DeleteForTesting as a gross hack in base/singleton.h . At very least I can clear it up now that we have base::NoDestructor. https://chromium-review.googlesource.com/#/c/chromium/src/+/935841
,
Feb 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/919402657fd592a1c1388905279a300d6448e08d commit 919402657fd592a1c1388905279a300d6448e08d Author: Primiano Tucci <primiano@chromium.org> Date: Mon Feb 26 23:06:32 2018 tracing: Singleton -> NoDestructor and fix TSan test violation This CL cleans up the hack put in place in base::Singleton to reset the TraceLog instance in tests, in favour of a least worst hack to reset the instance within trace_log.cc . Other than cleaning up singleton.h, this also removes a TSan violation caused by calling Singleton::OnExit() (see bug). The violation was reported, I believe, for the following reason: - A thread (that ends before the test reaches TearDown()) accesses the Singleton instance pointer in GetOrCreateLazyPointer with a load-acquire. - TearDown() clears up the Singleton instance pointer with a non-barriered write. This is enough to make TSan conclude "this will be racy" even if the race doesn't happen (because of causality in the TearDown(), all threads created by the tests should be gone once there). This CL fixes the problem by switching TraceLog's singleton to a base::NoDestructor. This should also make TraceLog::GetInstance slightly faster in production builds as it removes one layer of pointer dereferencing (Singleton's instance is heap-based, base::NoDestructor is bss-based). The ResetForTesting method is implemented by simply invoking the destructor, clearing up the static storage and re-placement-new-ing the object. This keeps the pointer returned by GetInstance() unchanged, but still achieves the reset needed by tests. Bug: 815234 TBR: rouslan@chromium.org Change-Id: I4c0dfc33ea45e64f36234da2006ece96905914b9 Reviewed-on: https://chromium-review.googlesource.com/935841 Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org> Reviewed-by: Lei Zhang <thestig@chromium.org> Commit-Queue: Primiano Tucci <primiano@chromium.org> Cr-Commit-Position: refs/heads/master@{#539307} [modify] https://crrev.com/919402657fd592a1c1388905279a300d6448e08d/base/memory/singleton.h [modify] https://crrev.com/919402657fd592a1c1388905279a300d6448e08d/base/trace_event/memory_dump_manager_unittest.cc [modify] https://crrev.com/919402657fd592a1c1388905279a300d6448e08d/base/trace_event/trace_event_unittest.cc [modify] https://crrev.com/919402657fd592a1c1388905279a300d6448e08d/base/trace_event/trace_log.cc [modify] https://crrev.com/919402657fd592a1c1388905279a300d6448e08d/base/trace_event/trace_log.h [modify] https://crrev.com/919402657fd592a1c1388905279a300d6448e08d/components/payments/content/service_worker_payment_app_factory.h [modify] https://crrev.com/919402657fd592a1c1388905279a300d6448e08d/services/resource_coordinator/public/cpp/memory_instrumentation/tracing_integration_unittest.cc
,
Feb 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4b4bfc5848baa03343cba8c470df845968e42559 commit 4b4bfc5848baa03343cba8c470df845968e42559 Author: Mostyn Bramley-Moore <mostynb@vewd.com> Date: Tue Feb 27 13:08:58 2018 [jumbo] followup after 'tracing: Singleton -> NoDestructor and fix TSan test violation' There are multiple g_instance_for_testing globals after https://chromium-review.googlesource.com/c/chromium/src/+/935841 - let's rename one of them to avoid jumbo build breakage. TBR=primiano@chromium.org Bug: 815234 ,775547 Change-Id: I184578b694e59dad8b06643db199f32820fd5887 Reviewed-on: https://chromium-review.googlesource.com/938821 Commit-Queue: Mostyn Bramley-Moore <mostynb@vewd.com> Reviewed-by: Mostyn Bramley-Moore <mostynb@vewd.com> Cr-Commit-Position: refs/heads/master@{#539424} [modify] https://crrev.com/4b4bfc5848baa03343cba8c470df845968e42559/base/trace_event/trace_log.cc
,
Feb 27 2018
https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_tsan_rel_ng seems green now |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by thestig@chromium.org
, Feb 23 2018