New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 815234 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug
Hotlist-MemoryInfra

Blocking:
issue 482593



Sign in to add a comment

MemoryTracingIntegrationTest fails on TSAN

Project Member Reported by thestig@chromium.org, Feb 23 2018

Issue description

Trying 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 ...
 
Blocking: 482593
Owner: primiano@chromium.org
Status: Started (was: Untriaged)
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

Project Member

Comment 3 by bugdroid1@chromium.org, 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

Project Member

Comment 4 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
https://ci.chromium.org/buildbot/tryserver.chromium.linux/linux_chromium_tsan_rel_ng seems green now

Sign in to add a comment