• Peter Marshall's avatar
    [tracing] Fix races in TracingController implementation · 51e80efd
    Peter Marshall authored
    The default TracingController (used by d8 and Node) has some concurrency
    issues. The new test flushes these out, when a second thread logs trace
    events while the main thread calls StopTracing().
    
    - Use an acquire load in UpdateCategoryGroupEnabledFlags() because this
      was racing with GetCategoryGroupEnabled() where a new category is
      added in the slow path. g_category_groups is append-only, but
      reads/writes to g_category_index need to be correctly ordered so that
      new categories are added and only then is the change to the index
      visible. The relaxed load ignored this and caused unsynchronized
      read/write.
    - Use a relaxed load in ~ScopedTracer() to access category_group_enabled
      as this previously used a non-atomic operation which caused a race
      with UpdateCategoryGroupEnabledFlag() which does a relaxed store.
    - Replace TracingController::mode_ with an atomic bool as read/writes to
      mode_ were not synchronized and caused TSAN errors. It only has two
      states and it doesn't seem like we will extend this so just convert it
      to bool.
    - Take the lock around calling trace_object->Initialize in
      AddTraceEvent(), and around trace_buffer_->Flush() in StopTracing().
      These two raced previously as the underlying TraceBufferRingBuffer
      passes out pointers to TraceObjects in a synchronized way, but the
      caller (AddTraceEvent) then writes into the object without
      synchronization. This leads to races when Flush() is called, at which
      time TraceBufferRingBuffer assumes that all the pointers it handed out
      are to valid, initialized TraceObjects - which is not true because
      AddTraceEvent may still be calling Initialize on them. This could be
      the cause of issues in Node.js where the last line of tracing/logging
      sometimes gets cut off. This is kind of a band-aid solution - access
      to the TraceObjects handed out by the ring buffer really needs proper
      synchronization which at this point would require redesign. It's quite
      likely we will replace this with Perfetto in the near future so not
      much point investing in this code right now.
    - Enable TracingCpuProfiler test which was flaky due to these bugs.
    
    Bug: v8:8821
    Change-Id: I141296800c6906ac0e7f3f21dd16d861b07dae62
    Reviewed-on: https://chromium-review.googlesource.com/c/1477283
    Commit-Queue: Peter Marshall <petermarshall@chromium.org>
    Reviewed-by: 's avatarYang Guo <yangguo@chromium.org>
    Reviewed-by: 's avatarAli Ijaz Sheikh <ofrobots@google.com>
    Cr-Commit-Position: refs/heads/master@{#59752}
    51e80efd
Name
Last commit
Last update
..
libplatform Loading commit data...
APIDesign.md Loading commit data...
DEPS Loading commit data...
OWNERS Loading commit data...
v8-inspector-protocol.h Loading commit data...
v8-inspector.h Loading commit data...
v8-internal.h Loading commit data...
v8-platform.h Loading commit data...
v8-profiler.h Loading commit data...
v8-testing.h Loading commit data...
v8-util.h Loading commit data...
v8-value-serializer-version.h Loading commit data...
v8-version-string.h Loading commit data...
v8-version.h Loading commit data...
v8-wasm-trap-handler-posix.h Loading commit data...
v8-wasm-trap-handler-win.h Loading commit data...
v8.h Loading commit data...
v8config.h Loading commit data...