implement SetWatchEvent and WaitForEvent for trace-based-tests

implement SetWatchEvent and WaitForEvent for trace-based-tests.

Up until now, tracing-based tests have been required to either run a trace for some amount of time or use existing notification mechanisms to return control to the test.

This change adds a 'watch event' feature to trace_event_impl which can be used by tests to wait for an event to occur. This mechanism could replace the use of test-only notifications which is frowned upon as well as mock classes for catching events. Trace events also have the huge advantage of working on all chrome processes as opposed to the browser-only notification service.

BUG=139939
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154552

[jbates, 2012-08-05 20:43:08]

ptal

[jbates, 2012-08-07 23:32:14]

ptal

[ccameron, 2012-08-21 01:06:34]

http://codereview.chromium.org/10837082/diff/8001/base/debug/trace_event_impl.cc
File base/debug/trace_event_impl.cc (right):

http://codereview.chromium.org/10837082/diff/8001/base/debug/trace_event_impl...
base/debug/trace_event_impl.cc:333: ++trace_log_->notification_thread_count_;
[RW-lock evangelism]: With a separate reader-writer lock for the callback, we
could discard this count-tracking.

http://codereview.chromium.org/10837082/diff/8001/base/debug/trace_event_impl...
base/debug/trace_event_impl.cc:339: if (notification_) {
[RW-lock evangelism]: With a separate reader-writer lock for the callback, we
would do AutoReaderLock lock(trace_log_->callback_lock_)

http://codereview.chromium.org/10837082/diff/8001/base/debug/trace_event_impl...
base/debug/trace_event_impl.cc:583: AutoLock lock(lock_);
[RW-lock evangelism]: With a separate reader-writer lock for the callback, this
would be AutoWriterLock lock(callback_lock_)

http://codereview.chromium.org/10837082/diff/8001/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

http://codereview.chromium.org/10837082/diff/8001/base/debug/trace_event_impl...
base/debug/trace_event_impl.h:160: enum Notification {
This isn't treated as a bit mask (but rather as a standard enum) in
TraceControllerImpl::OnTraceNotification.  Probably easier to not use as a mask
with multiple events.

http://codereview.chromium.org/10837082/diff/8001/base/debug/trace_event_impl...
base/debug/trace_event_impl.h:281: // NOTE: If |num_occurrences| events have
already occurred, the notification
I have a preference for not supporting this semantic (fire if these events
happened in the past), but it's mostly gut-based.  We should discuss how this
impacts use-cases offline.

http://codereview.chromium.org/10837082/diff/8001/base/debug/trace_event_impl...
base/debug/trace_event_impl.h:344: NotificationCallback notification_callback_;
I see no concurrency issues with this implementation.

That said, this seems to be a hand-rolled read-writer lock.  Perhaps we should
add that sort of structure to base (with LockForRead in SendNotificationIfAny,
and LockForWrite in SetNotificationCallback)?

That way, maintainers will be less likely to break this -- the mental load over
verifying there were no issues was high, and my certainty-of-correctness has a
half-life of ~2 minutes.  In fact, it may be worth considering just creating a
separate ordinary lock (and not exposing concurrent callbacks from multiple
threads) just to simplify the code.

http://codereview.chromium.org/10837082/diff/8001/chrome/test/base/tracing.cc
File chrome/test/base/tracing.cc (right):

http://codereview.chromium.org/10837082/diff/8001/chrome/test/base/tracing.cc...
chrome/test/base/tracing.cc:50: is_waiting_on_watch_ = true;
Race: we will use is_waiting_on_watch to see if it is safe to access
message_loop_runner_ (resulting in NULL pointer access).  I'd advocate making
these updates and checks atomic by putting them in a mutex.

http://codereview.chromium.org/10837082/diff/8001/chrome/test/base/tracing.cc...
chrome/test/base/tracing.cc:100: if (is_waiting_on_watch_)
Race: is_waiting_in_watch_ can be true while message_loop_runner_ is still NULL
(see WaitForWatchEvent's assignment).

http://codereview.chromium.org/10837082/diff/8001/content/browser/trace_contr...
File content/browser/trace_controller_impl.cc (right):

http://codereview.chromium.org/10837082/diff/8001/content/browser/trace_contr...
content/browser/trace_controller_impl.cc:383: switch
(static_cast<base::debug::TraceLog::Notification>(notification)) {
The definition of ...::Notification says that the incoming int is a bit mask,
but we're treating it as an enum here (probably easier as an enum).

[jbates, 2012-08-23 22:45:28]

Latest CL adds a browsertest for the new tracing::WaitForWatchEvent API.

I'd like to hold off on implementing support for multiple Waits in a single
Begin/EndTracing scope, because it adds a number of complications. tldr: Using a
sequence number as you suggested would work, but the NotificationHelper and
notification callback would need to include an extra parameter for the sequence
number and that would only be used by one of the two possible events. If someone
ends up asking for that feature though, it's certainly possible to implement in
the future.

https://chromiumcodereview.appspot.com/10837082/diff/8001/base/debug/trace_ev...
File base/debug/trace_event_impl.h (right):

https://chromiumcodereview.appspot.com/10837082/diff/8001/base/debug/trace_ev...
base/debug/trace_event_impl.h:160: enum Notification {
On 2012/08/21 01:06:34, ccameron1 wrote:
> This isn't treated as a bit mask (but rather as a standard enum) in
> TraceControllerImpl::OnTraceNotification.  Probably easier to not use as a
mask
> with multiple events.

Good catch, TraceControllerImpl::OnTraceNotification was wrong -- I updated it
to check for both flags because it's possible for them both to happen in one
notification.

https://chromiumcodereview.appspot.com/10837082/diff/8001/base/debug/trace_ev...
base/debug/trace_event_impl.h:281: // NOTE: If |num_occurrences| events have
already occurred, the notification
On 2012/08/21 01:06:34, ccameron1 wrote:
> I have a preference for not supporting this semantic (fire if these events
> happened in the past), but it's mostly gut-based.  We should discuss how this
> impacts use-cases offline.

This is important to catch the events that occur at process start before the
process hooks up to IPC and is told to start tracing (such as GPU process start
during the trace). It could be a bool option here, but with the current
requirement that there is one watch per Begin/End tracing, it seems good to keep
it simple until we have a feature request for more advanced use cases.

https://chromiumcodereview.appspot.com/10837082/diff/8001/base/debug/trace_ev...
base/debug/trace_event_impl.h:344: NotificationCallback notification_callback_;
On 2012/08/21 01:06:34, ccameron1 wrote:
> I see no concurrency issues with this implementation.
> 
> That said, this seems to be a hand-rolled read-writer lock.  Perhaps we should
> add that sort of structure to base (with LockForRead in SendNotificationIfAny,
> and LockForWrite in SetNotificationCallback)?
> 
> That way, maintainers will be less likely to break this -- the mental load
over
> verifying there were no issues was high, and my certainty-of-correctness has a
> half-life of ~2 minutes.  In fact, it may be worth considering just creating a
> separate ordinary lock (and not exposing concurrent callbacks from multiple
> threads) just to simplify the code.

As discussed, added comments around the NotificationHelper to explain that it's
like a reader-writer lock.

https://chromiumcodereview.appspot.com/10837082/diff/8001/chrome/test/base/tr...
File chrome/test/base/tracing.cc (right):

https://chromiumcodereview.appspot.com/10837082/diff/8001/chrome/test/base/tr...
chrome/test/base/tracing.cc:50: is_waiting_on_watch_ = true;
On 2012/08/21 01:06:34, ccameron1 wrote:
> Race: we will use is_waiting_on_watch to see if it is safe to access
> message_loop_runner_ (resulting in NULL pointer access).  I'd advocate making
> these updates and checks atomic by putting them in a mutex.

As discussed, the missing detail is that these methods are all called on the UI
thread. Added some DCHECKs and comments to make that more clear.

https://chromiumcodereview.appspot.com/10837082/diff/8001/content/browser/tra...
File content/browser/trace_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/10837082/diff/8001/content/browser/tra...
content/browser/trace_controller_impl.cc:383: switch
(static_cast<base::debug::TraceLog::Notification>(notification)) {
On 2012/08/21 01:06:34, ccameron1 wrote:
> The definition of ...::Notification says that the incoming int is a bit mask,
> but we're treating it as an enum here (probably easier as an enum).

Good catch, fixed this code to check for both flags.

[jbates, 2012-08-24 18:54:51]

ccameron: lg?

[ccameron, 2012-08-24 20:00:18]

On 2012/08/24 18:54:51, jbates wrote:
> ccameron: lg?

lgtm

[jbates, 2012-08-24 20:23:45]

brettw: */OWNERS

[brettw, 2012-08-29 06:54:36]

overloaded, please find another reviewer.

[jbates, 2012-08-29 15:50:36]

Owners, ptal.

jar: base/OWNERS
sky: chrome/test/OWNERS
piman: content/OWNERS

[sky, 2012-08-29 16:05:09]

http://codereview.chromium.org/10837082/diff/18002/chrome/test/base/tracing.cc
File chrome/test/base/tracing.cc (right):

http://codereview.chromium.org/10837082/diff/18002/chrome/test/base/tracing.c...
chrome/test/base/tracing.cc:27: , watch_notification_count_(0) {}
, on previous line.

http://codereview.chromium.org/10837082/diff/18002/chrome/test/base/tracing.c...
chrome/test/base/tracing.cc:106: virtual void OnEventWatchNotification() {
OVERRIDE

http://codereview.chromium.org/10837082/diff/18002/chrome/test/base/tracing.c...
chrome/test/base/tracing.cc:112: message_loop_runner_->Quit();
Is it possible the timer is still running?

http://codereview.chromium.org/10837082/diff/18002/chrome/test/base/tracing.h
File chrome/test/base/tracing.h (right):

http://codereview.chromium.org/10837082/diff/18002/chrome/test/base/tracing.h...
chrome/test/base/tracing.h:34: const char* category_name,
Can all the args be const std::string& ? Simpler from a lifetime perspectifve.

[piman, 2012-08-29 16:50:07]

LGTM+nit

http://codereview.chromium.org/10837082/diff/18002/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

http://codereview.chromium.org/10837082/diff/18002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:229: // time. After calling
SetNotificationCallback(NotificationCallback()) to
nit: please note (because it's not obvious) that the callback can occur on any
thread that adds a trace.

http://codereview.chromium.org/10837082/diff/18002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:342: // Number of threads that are have a copy of
the notification callback.
nit: "that are have" typo?

[jbates, 2012-08-29 21:14:12]

http://codereview.chromium.org/10837082/diff/18002/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

http://codereview.chromium.org/10837082/diff/18002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:229: // time. After calling
SetNotificationCallback(NotificationCallback()) to
On 2012/08/29 16:50:07, piman wrote:
> nit: please note (because it's not obvious) that the callback can occur on any
> thread that adds a trace.

Done.

http://codereview.chromium.org/10837082/diff/18002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:342: // Number of threads that are have a copy of
the notification callback.
On 2012/08/29 16:50:07, piman wrote:
> nit: "that are have" typo?

Done. Changed to "are belong to".

http://codereview.chromium.org/10837082/diff/18002/chrome/test/base/tracing.cc
File chrome/test/base/tracing.cc (right):

http://codereview.chromium.org/10837082/diff/18002/chrome/test/base/tracing.c...
chrome/test/base/tracing.cc:27: , watch_notification_count_(0) {}
On 2012/08/29 16:05:09, sky wrote:
> , on previous line.

Done.

http://codereview.chromium.org/10837082/diff/18002/chrome/test/base/tracing.c...
chrome/test/base/tracing.cc:106: virtual void OnEventWatchNotification() {
On 2012/08/29 16:05:09, sky wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/10837082/diff/18002/chrome/test/base/tracing.c...
chrome/test/base/tracing.cc:112: message_loop_runner_->Quit();
On 2012/08/29 16:05:09, sky wrote:
> Is it possible the timer is still running?

It's stopped on line 110.

http://codereview.chromium.org/10837082/diff/18002/chrome/test/base/tracing.h
File chrome/test/base/tracing.h (right):

http://codereview.chromium.org/10837082/diff/18002/chrome/test/base/tracing.h...
chrome/test/base/tracing.h:34: const char* category_name,
On 2012/08/29 16:05:09, sky wrote:
> Can all the args be const std::string& ? Simpler from a lifetime perspectifve.

Done.

[piman, 2012-08-29 21:27:34]

All your base LGTM

[sky, 2012-08-29 21:40:46]

LGTM

[jar (doing other things), 2012-08-29 22:38:19]

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
File base/debug/trace_event_impl.cc (right):

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:323: , notification_(0) {
nit: comma on end of previous line.

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:341: if (notification_) {
nit: early return would mean you don't need to indent the rest (or read it to
see what the else clause isn't).

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:361: , notification_condition_(&lock_)
nit: comma on end of lines  (...though this was a cute idea.... it isn't Chrome
style :-/ ).

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:231: // is guaranteed that the old callback will
no longer be called. Warning: it
How do you guarantee that the "old callback will no longer be called?"  What
happens if the call has started(?), but the calling thread pre-empts for an
arbitrary length of time, during which time we call SetNotificationCallback(). 
Couldn't' this result is future "calls" to the old callback, as the stuck-thread
releases?

What is it that this comment is trying to say, and how is it actionable?  IMO,
you can't ever teardown infrastructure critical to the "old" callback.

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:316: inline NotificationHelper(TraceLog*
trace_log);
nit: explicit.

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:320: inline void AddNotificationWhileLocked(int
notification);
What is the distinction betwene "AddNotification" and "SendNotification"?

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:339: Lock lock_;
Can you clarify what is protected by this lock?

[jbates, 2012-08-29 23:12:06]

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
File base/debug/trace_event_impl.cc (right):

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:323: , notification_(0) {
On 2012/08/29 22:38:19, jar wrote:
> nit: comma on end of previous line.

Done.

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:341: if (notification_) {
On 2012/08/29 22:38:19, jar wrote:
> nit: early return would mean you don't need to indent the rest (or read it to
> see what the else clause isn't).

Done.

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:361: , notification_condition_(&lock_)
On 2012/08/29 22:38:19, jar wrote:
> nit: comma on end of lines  (...though this was a cute idea.... it isn't
Chrome
> style :-/ ).

Done. Oops, I must have been infected by WebKit style recently.

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:231: // is guaranteed that the old callback will
no longer be called. Warning: it
On 2012/08/29 22:38:19, jar wrote:
> How do you guarantee that the "old callback will no longer be called?"  What
> happens if the call has started(?), but the calling thread pre-empts for an
> arbitrary length of time, during which time we call SetNotificationCallback().

> Couldn't' this result is future "calls" to the old callback, as the
stuck-thread
> releases?
> 
> What is it that this comment is trying to say, and how is it actionable?  IMO,
> you can't ever teardown infrastructure critical to the "old" callback.

After SetNotificationCallback returns, the calling thread can then delete the
target of any previously set callback. (Added that to the comments.) The
synchronization code in SetNotificationCallback should guarantee that.

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:316: inline NotificationHelper(TraceLog*
trace_log);
On 2012/08/29 22:38:19, jar wrote:
> nit: explicit.

Done.

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:320: inline void AddNotificationWhileLocked(int
notification);
On 2012/08/29 22:38:19, jar wrote:
> What is the distinction betwene "AddNotification" and "SendNotification"?

Updated comments.

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:339: Lock lock_;
On 2012/08/29 22:38:19, jar wrote:
> Can you clarify what is protected by this lock?

Done.

[jar (doing other things), 2012-08-29 23:35:45]

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:231: // is guaranteed that the old callback will
no longer be called. Warning: it
Is the callback made under the protection of a lock (note is is generally a bad
idea to protect code, rather than data, via a lock)?

How do you preclude a call to the old callback??

Is this all run on one thread?

On 2012/08/29 23:12:06, jbates wrote:
> On 2012/08/29 22:38:19, jar wrote:
> > How do you guarantee that the "old callback will no longer be called?"  What
> > happens if the call has started(?), but the calling thread pre-empts for an
> > arbitrary length of time, during which time we call
SetNotificationCallback().
> 
> > Couldn't' this result is future "calls" to the old callback, as the
> stuck-thread
> > releases?
> > 
> > What is it that this comment is trying to say, and how is it actionable? 
IMO,
> > you can't ever teardown infrastructure critical to the "old" callback.
> 
> After SetNotificationCallback returns, the calling thread can then delete the
> target of any previously set callback. (Added that to the comments.) The
> synchronization code in SetNotificationCallback should guarantee that.

http://codereview.chromium.org/10837082/diff/27005/base/debug/trace_event_imp...
File base/debug/trace_event_impl.cc (right):

http://codereview.chromium.org/10837082/diff/27005/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:322: trace_log_(trace_log),
not quite: When you can't fit all initializers on the definition line.....
(which is this case):

Colon for initializers is indented 4, followed by a space, followed by the first
initializer.
Additional initializers are indented in line with the first initializer.

Name()

http://codereview.chromium.org/10837082/diff/27005/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:361: enabled_(false),
Nit: fix format for initializer (as described above).

[jbates, 2012-08-30 00:55:14]

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_impl.h
File base/debug/trace_event_impl.h (right):

http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
base/debug/trace_event_impl.h:231: // is guaranteed that the old callback will
no longer be called. Warning: it
On 2012/08/29 23:35:45, jar wrote:
> Is the callback made under the protection of a lock (note is is generally a
bad
> idea to protect code, rather than data, via a lock)?
> 
> How do you preclude a call to the old callback??
> 
> Is this all run on one thread?
> 
> On 2012/08/29 23:12:06, jbates wrote:
> > On 2012/08/29 22:38:19, jar wrote:
> > > How do you guarantee that the "old callback will no longer be called?" 
What
> > > happens if the call has started(?), but the calling thread pre-empts for
an
> > > arbitrary length of time, during which time we call
> SetNotificationCallback().
> > 
> > > Couldn't' this result is future "calls" to the old callback, as the
> > stuck-thread
> > > releases?
> > > 
> > > What is it that this comment is trying to say, and how is it actionable? 
> IMO,
> > > you can't ever teardown infrastructure critical to the "old" callback.
> > 
> > After SetNotificationCallback returns, the calling thread can then delete
the
> > target of any previously set callback. (Added that to the comments.) The
> > synchronization code in SetNotificationCallback should guarantee that.
> 

As discussed, you were correct in suspecting this code, since there was a
possible deadlock if the SetNotificationCallback method was called during a
notification callback. I updated the comments for SetNotificationCallback with a
warning that you must not call it during a notification callback.

Normally this design would not be necessary because callbacks have ref-counting
protection. Unfortunately, one of the targets of this callback is a Singleton
(TraceControllerImpl), which can not be ref-counted. So instead the
TraceControllerImpl specifies base::Unretained in its call to
SetNotificationCallback. To safely support that use case, this cond-var is my
best idea so far. Technically we could get really crazy and store a vector of
thread-ids that are holding copies of the callback, and then check if the caller
to SetNotification is in that vector, but since there are no use cases for this
it seems more effective to document the requirement on SetNotificationCallback.
If it ever does happen, it will be very clear: there will be a deadlock 100% of
the time and the stack will clearly show the user code that broke this rule.

http://codereview.chromium.org/10837082/diff/27005/base/debug/trace_event_imp...
File base/debug/trace_event_impl.cc (right):

http://codereview.chromium.org/10837082/diff/27005/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:322: trace_log_(trace_log),
On 2012/08/29 23:35:45, jar wrote:
> not quite: When you can't fit all initializers on the definition line.....
> (which is this case):
> 
> Colon for initializers is indented 4, followed by a space, followed by the
first
> initializer.
> Additional initializers are indented in line with the first initializer.
> 
> Name()

Done.

http://codereview.chromium.org/10837082/diff/27005/base/debug/trace_event_imp...
base/debug/trace_event_impl.cc:361: enabled_(false),
On 2012/08/29 23:35:45, jar wrote:
> Nit: fix format for initializer (as described above).

Done.

[jbates, 2012-08-30 20:22:31]

On 2012/08/30 00:55:14, jbates wrote:
>
http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_impl.h
> File base/debug/trace_event_impl.h (right):
> 
>
http://codereview.chromium.org/10837082/diff/27002/base/debug/trace_event_imp...
> base/debug/trace_event_impl.h:231: // is guaranteed that the old callback will
> no longer be called. Warning: it
> On 2012/08/29 23:35:45, jar wrote:
> > Is the callback made under the protection of a lock (note is is generally a
> bad
> > idea to protect code, rather than data, via a lock)?
> > 
> > How do you preclude a call to the old callback??
> > 
> > Is this all run on one thread?
> > 
> > On 2012/08/29 23:12:06, jbates wrote:
> > > On 2012/08/29 22:38:19, jar wrote:
> > > > How do you guarantee that the "old callback will no longer be called?" 
> What
> > > > happens if the call has started(?), but the calling thread pre-empts for
> an
> > > > arbitrary length of time, during which time we call
> > SetNotificationCallback().
> > > 
> > > > Couldn't' this result is future "calls" to the old callback, as the
> > > stuck-thread
> > > > releases?
> > > > 
> > > > What is it that this comment is trying to say, and how is it actionable?

> > IMO,
> > > > you can't ever teardown infrastructure critical to the "old" callback.
> > > 
> > > After SetNotificationCallback returns, the calling thread can then delete
> the
> > > target of any previously set callback. (Added that to the comments.) The
> > > synchronization code in SetNotificationCallback should guarantee that.
> > 
> 
> As discussed, you were correct in suspecting this code, since there was a
> possible deadlock if the SetNotificationCallback method was called during a
> notification callback. I updated the comments for SetNotificationCallback with
a
> warning that you must not call it during a notification callback.
> 
> Normally this design would not be necessary because callbacks have
ref-counting
> protection. Unfortunately, one of the targets of this callback is a Singleton
> (TraceControllerImpl), which can not be ref-counted.

As discussed offline with Jim, I've changed TraceControllerImpl to a leaky
LazyInstance so that it can leave itself as the NotificationCallback target
without risking use-after-free. Now we no longer need any fancy recursive
cond-var workarounds in TraceLog!

> So instead the
> TraceControllerImpl specifies base::Unretained in its call to
> SetNotificationCallback. To safely support that use case, this cond-var is my
> best idea so far. Technically we could get really crazy and store a vector of
> thread-ids that are holding copies of the callback, and then check if the
caller
> to SetNotification is in that vector, but since there are no use cases for
this
> it seems more effective to document the requirement on
SetNotificationCallback.
> If it ever does happen, it will be very clear: there will be a deadlock 100%
of
> the time and the stack will clearly show the user code that broke this rule.
> 
>
http://codereview.chromium.org/10837082/diff/27005/base/debug/trace_event_imp...
> File base/debug/trace_event_impl.cc (right):
> 
>
http://codereview.chromium.org/10837082/diff/27005/base/debug/trace_event_imp...
> base/debug/trace_event_impl.cc:322: trace_log_(trace_log),
> On 2012/08/29 23:35:45, jar wrote:
> > not quite: When you can't fit all initializers on the definition line.....
> > (which is this case):
> > 
> > Colon for initializers is indented 4, followed by a space, followed by the
> first
> > initializer.
> > Additional initializers are indented in line with the first initializer.
> > 
> > Name()
> 
> Done.
> 
>
http://codereview.chromium.org/10837082/diff/27005/base/debug/trace_event_imp...
> base/debug/trace_event_impl.cc:361: enabled_(false),
> On 2012/08/29 23:35:45, jar wrote:
> > Nit: fix format for initializer (as described above).
> 
> Done.

[jar (doing other things), 2012-08-30 23:20:16]

Base changes LGTM

[commit-bot: I haz the power, 2012-08-30 23:22:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/10837082/50001

[commit-bot: I haz the power, 2012-08-30 23:22:59]

Failed to apply patch for content/renderer/render_view_impl.cc:
While running patch -p1 --forward --force;
patching file content/renderer/render_view_impl.cc
Hunk #1 FAILED at 13.
Hunk #2 succeeded at 1204 (offset 41 lines).
1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/render_view_impl.cc.rej

[commit-bot: I haz the power, 2012-08-31 21:44:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jbates@chromium.org/10837082/49004

[commit-bot: I haz the power, 2012-09-01 00:52:21]

Change committed as 154552