Protect TraceLog::enabled_state_observer_list_ with a dedicated lock

Protect TraceLog::enabled_state_observer_list_ with a dedicated lock

TraceLog dispatches events to observers without protection of any lock,
which is a source of data race. However, we can't simply protect it with
TraceLog::lock_ because that leads to deadlock.

Hence, this patch introduces a dedicated lock to protect accesses to the
observer list without causing deadlock.

To prevent introducing new deadlocks, this patch also sets a policy that
any subclass of EnabledStateObserver must not try to access the TraceLog's
observer list from the OnTraceLogEnabled()/Disabled() callbacks. Unit test
TraceEventTestFixture.SelfRemovingObserver is removed due to violation of
the policy.

BUG=546021

[Xiaocheng, 2016-04-27 09:52:36]

Description was changed from

==========
Protect TraceLog::enabled_state_observer_list_ with a dedicated lock

TraceLog dispatches events to observers without protection of any lock,
which is a source of data race. However, we can't simply protect it with
TraceLog::lock_ because that leads to deadlock.

Hence, this patch introduces a dedicated lock to protect accesses to the
observer list without causing deadlock.

BUG=546021
==========

to

==========
Protect TraceLog::enabled_state_observer_list_ with a dedicated lock

TraceLog dispatches events to observers without protection of any lock,
which is a source of data race. However, we can't simply protect it with
TraceLog::lock_ because that leads to deadlock.

Hence, this patch introduces a dedicated lock to protect accesses to the
observer list without causing deadlock.

To prevent introducing new deadlocks, this patch also sets a policy that
any subclass of EnabledStateObserver must not try to access the TraceLog's
observer list from the OnTraceLogEnabled()/Disabled() callbacks. Unit test
TraceEventTestFixture.SelfRemovingObserver is removed due to violation of
the policy.

BUG=546021
==========

[Xiaocheng, 2016-04-27 12:18:28]

xiaochengh@chromium.org changed reviewers:
+ primiano@chromium.org, skyostil@chromium.org

[Xiaocheng, 2016-04-27 12:18:28]

This patch should be necessary if we want to fix any data race related to
EnabledStateObserver.

PTAL.

[Primiano Tucci (use gerrit), 2016-04-27 13:50:21]

Let me have a chat with skyostil@, and see if there is a  way to make
BlameContext safe.
I need to think more, but I'm not 100% sold on the fact that we need to
guarantee this from the tracing viewpoint. I think that so far the assumption
was that the trace log observer are long lived (IMHO the remove methods should
not have been there in the first place).
This adding a new excitement level of complexity to the tracing codebase.

https://codereview.chromium.org/1923193002/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_unittest.cc (left):

https://codereview.chromium.org/1923193002/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_unittest.cc:1154: TEST_F(TraceEventTestFixture,
SelfRemovingObserver) {
Hmm why are we removing this test? I assume it was introduced to capture some
use case no?

[Xiaocheng, 2016-04-27 14:18:13]

Thanks for the review.

It seems very difficult to get rid of the race that you found with dtor in a
different way: as long as we can enter the callback without holding a lock (or
sth similar), there is nothing to stop another thread from running the dtor at
the same time.

I guess this race happens not only for BlameContext but also other subclasses of
EnabledStateObserver. I'll verify tmr.

https://codereview.chromium.org/1923193002/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event_unittest.cc (left):

https://codereview.chromium.org/1923193002/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event_unittest.cc:1154: TEST_F(TraceEventTestFixture,
SelfRemovingObserver) {
On 2016/04/27 at 13:50:21, Primiano Tucci wrote:
> Hmm why are we removing this test? I assume it was introduced to capture some
use case no?

I haven't seen any real use case in the code base that needs to modifies
TraceLog::enabled_state_observer_list_ during the callback. Guess it's just a
corner case?

If we add a lock to protect the observer list when dispatching to them, this
test case has to be removed. Otherwise the call back would try to acquire the
same lock again (via RemoveEnabledStateObserver), which causes a deadlock.

[Sami, 2016-04-28 15:33:02]

We did some deep thinking with Primiano and ended up with this idea: Since the
current trace log observer interface is fundamentally thread-unsafe[1], we
should replace it with something better. One way to do this would be to replace
Add/RemoveObserver() with this:

    AddObserver(WeakPtr<TraceLogEnabledObserver> observer);
    RemoveObserver(WeakPtr<TraceLogEnabledObserver> observer);

This would keep a list of observers paired with ThreadTaskRunnerHandle::Get()
recorded in AddObserver().

When tracing is enabled or disabled, the trace log would walk the list of
observers and post tasks to each one. If the observer is deleted on another
thread while this is happening, the posted task will automatically turn into a
no-op.

Primiano will post a C++ version of the above :)

[1] Anything that is registered cannot be safely destroyed (outside the file
thread), because the callback might happen (on the file thread) while an
observer is being unregistered and/or destroyed.

[Primiano Tucci (use gerrit), 2016-04-28 15:34:34]

On 2016/04/28 15:33:02, Sami (slow -- travelling) wrote:
> Primiano will post a C++ version of the above :)

So this is what I was thinking. I need to leave the office in few minutes, so if
you are willing to give a shoot at that I'll be more than glad and happy to
review.
If you are busy don't worry, that's not really something you introduced, I can
look at this once back to LON (Tuesday next week)


// Guarantees that the OnTraceLogEnabled/Disabled callbacks are always posted
// on the task_runner on which the observer lives. This is to allow the observer
// to safely unregister in its dtor. 
class EnabledStateObserverThreadSafeProxy  : public EnabledStateObserver,
Singleton {
  // This singleton registers to the TraceLog and is an observer itself.

  struct RegisteredThreadSafeObserver {
    WeakPtr<EnabledStateObserver> observer;
    scoped_refptr<TaskRunner> task_runner;
  }

  vector<RegisteredThreadSafeObserver> observers;

  void RegisterThreadSafeObserver( WeakPtr<EnabledStateObserver> o) {
    Autolock
    observers.push_back({o, ThreadTaskRunnerHandle::get()})
  }

  // this is called from TraceLog
  void OnTraceLogEnabled() override {
    AutoLock;
    for (obsweakptr, task_runner in observers) {
      task_runner->PostTask(Bind(&EnabledStateObserver::OnTraceLogEnabled,
obsweakptr);
    }  
  }

  // same for disabled
}

[Xiaocheng, 2016-05-09 02:26:24]

On 2016/04/28 at 15:34:34, primiano wrote:
> On 2016/04/28 15:33:02, Sami (slow -- travelling) wrote:
> > Primiano will post a C++ version of the above :)
> 
> So this is what I was thinking. I need to leave the office in few minutes, so
if you are willing to give a shoot at that I'll be more than glad and happy to
review.

I'll take it since it's blocking my CL.

[Primiano Tucci (use gerrit), 2016-05-10 16:06:05]

Can this CL be closed in favor of crrev.com/1956323002 ? it just keep showing in
my review list.

[Xiaocheng, 2016-05-11 00:31:10]

On 2016/05/10 at 16:06:05, primiano wrote:
> Can this CL be closed in favor of crrev.com/1956323002 ? it just keep showing
in my review list.

Closed.