Threading issue with TraceLog::EnabledStateObserver |
|||
Issue descriptionTraceLog stores a vector of raw pointers of EnabledStateObserver, and invokes their callbacks synchronously when TraceLog::IsEnabled() state changes. The invocation is intentionally not lock-guarded, allowing callbacks to use TRACE_EVENT macros [1]. This allows simultaneous access to an observer from multiple threads when TraceLog and the observer are managed by different threads, and can cause threading issues. For example, when the callback of an observer is being run, another thread may run the destructor of the observer, resulting use-after-free (see discussion in [2]). We should introduce a new EnabledStateObserver interface that, when TraceLog::IsEnabled() state changes, the callbacks are run asynchronously as tasks posted back to the thread managing the observer. This ensures that the observer is accessed by a unique thread and eliminates threading issues. [1] https://codereview.chromium.org/16829002 [2] https://codereview.chromium.org/1916843003
,
May 11 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/69156980cae08c198a4102dde2a7275f66c4c0c0 commit 69156980cae08c198a4102dde2a7275f66c4c0c0 Author: xiaochengh <xiaochengh@chromium.org> Date: Wed May 11 11:04:14 2016 Introduce TraceLog::AsyncEnabledStateObserver This CL introduces AsyncEnabledStateObserver as a complement of the potentially thread-unsafe EnabledStateObserver class. The callbacks of the new class are called asynchronously as separate tasks, eliminating the possibility of threading issues when TraceLog and observers are managed in different threads. In follow-up CLs, some subclasses of EnabledStateObserver will be migrated to AsyncEnabledStateObserver instead. BUG= 610629 TEST=will be added in a follow-up CL Review-Url: https://codereview.chromium.org/1956323002 Cr-Commit-Position: refs/heads/master@{#392895} [modify] https://crrev.com/69156980cae08c198a4102dde2a7275f66c4c0c0/base/trace_event/trace_log.cc [modify] https://crrev.com/69156980cae08c198a4102dde2a7275f66c4c0c0/base/trace_event/trace_log.h
,
May 12 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/31ea0222e9a5124bd55a38ce9bec31a9623bb393 commit 31ea0222e9a5124bd55a38ce9bec31a9623bb393 Author: xiaochengh <xiaochengh@chromium.org> Date: Thu May 12 17:59:35 2016 Change the base class of BlameContext into AsyncEnabledStateObserver This is a follow-up CL of https://codereview.chromium.org/1956323002. BUG= 610629 Review-Url: https://codereview.chromium.org/1956333002 Cr-Commit-Position: refs/heads/master@{#393297} [modify] https://crrev.com/31ea0222e9a5124bd55a38ce9bec31a9623bb393/base/trace_event/blame_context.cc [modify] https://crrev.com/31ea0222e9a5124bd55a38ce9bec31a9623bb393/base/trace_event/blame_context.h [modify] https://crrev.com/31ea0222e9a5124bd55a38ce9bec31a9623bb393/base/trace_event/blame_context_unittest.cc [modify] https://crrev.com/31ea0222e9a5124bd55a38ce9bec31a9623bb393/components/scheduler/base/task_queue_manager_unittest.cc
,
May 24 2016
,
May 26 2016
I think that after #3 and https://codereview.chromium.org/1975363002/ this issue is fixed. Will reopen if I find something else. |
|||
►
Sign in to add a comment |
|||
Comment 1 by primiano@chromium.org
, May 10 2016Components: -Platform>DevTools>Tracing Internals>Tracing
Labels: -Restrict-View-Google
+bunch of tracing folks. -RVG as there doesn't seem to be anything secret here The TL;DR problem here is that, with the current API, the observers have to necessarily be indefinitely lived, or it's impossible to avoid races between OnTraceLog{Dis/En}abled and the dtor. This is a bummer for some clients like the BlameContext and the v8 profiler that are not long lived singletons. After some discussion w/ xiaochengh@ and skyostil@ looks like the saner thing to do is to post the observer threads on the clients thread using WeakPtr.