implement SetWatchEvent and WaitForEvent for trace-based-tests

base/debug/trace_event_impl.h

Index: base/debug/trace_event_impl.h
diff --git a/base/debug/trace_event_impl.h b/base/debug/trace_event_impl.h
index ca711dbd489108478394dc901ab40a78438a1a29..b5cb671ecb1beff77082f7d0ac9c35ecd1d6b607 100644
--- a/base/debug/trace_event_impl.h
+++ b/base/debug/trace_event_impl.h
@@ -16,6 +16,7 @@
 #include "base/memory/ref_counted_memory.h"
 #include "base/observer_list.h"
 #include "base/string_util.h"
+#include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
 #include "base/timer.h"
 
@@ -89,6 +90,7 @@ class BASE_EXPORT TraceEvent {
     return parameter_copy_storage_.get();
   }
 
+  const unsigned char* category_enabled() const { return category_enabled_; }
   const char* name() const { return name_; }
 
  private:
@@ -154,6 +156,16 @@ class BASE_EXPORT TraceResultBuffer {
 
 class BASE_EXPORT TraceLog {
  public:
+  // Notification is a mask of one or more of the following events.
+  enum Notification {
+    // The trace buffer does not flush dynamically, so when it fills up,
+    // subsequent trace events will be dropped. This callback is generated when
+    // the trace buffer is full. The callback must be thread safe.
+    TRACE_BUFFER_FULL = 1 << 0,
+    // A subscribed trace-event occurred.
+    EVENT_WATCH_NOTIFICATION = 1 << 1
+  };
+
   static TraceLog* GetInstance();
 
   // Get set of known categories. This can change as new code paths are reached.
@@ -213,23 +225,22 @@ class BASE_EXPORT TraceLog {
 
   float GetBufferPercentFull() const;
 
-  // When enough events are collected, they are handed (in bulk) to
-  // the output callback. If no callback is set, the output will be
-  // silently dropped. The callback must be thread safe. The string format is
+  // Set the thread-safe notification callback. The callback can occur at any
+  // time and from any thread. After calling
+  // SetNotificationCallback(NotificationCallback()) to clear the callback, it
+  // is guaranteed that the old callback will no longer be called. Warning: it

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

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.

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

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.

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

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:

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

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.

+  // is possible for the old callback to be called during a call to
+  // SetNotificationCallback.
+  typedef base::Callback<void(int)> NotificationCallback;
+  void SetNotificationCallback(const NotificationCallback& cb);
+
+  // Flush all collected events to the given output callback. The callback will
+  // be called one or more times with IPC-bite-size chunks. The string format is
   // undefined. Use TraceResultBuffer to convert one or more trace strings to
   // JSON.
   typedef base::Callback<void(const scoped_refptr<base::RefCountedString>&)>
       OutputCallback;
-  void SetOutputCallback(const OutputCallback& cb);
-
-  // The trace buffer does not flush dynamically, so when it fills up,
-  // subsequent trace events will be dropped. This callback is generated when
-  // the trace buffer is full. The callback must be thread safe.
-  typedef base::Callback<void(void)> BufferFullCallback;
-  void SetBufferFullCallback(const BufferFullCallback& cb);
-
-  // Flushes all logged data to the callback.
-  void Flush();
+  void Flush(const OutputCallback& cb);
 
   // Called by TRACE_EVENT* macros, don't call this directly.
   static const unsigned char* GetCategoryEnabled(const char* name);
@@ -263,6 +274,16 @@ class BASE_EXPORT TraceLog {
                                const void* id,
                                const std::string& extra);
 
+  // For every matching event, a notification will be fired. NOTE: the
+  // notification will fire for each matching event that has already occurred
+  // since tracing was started (including before tracing if the process was
+  // started with tracing turned on).
+  void SetWatchEvent(const std::string& category_name,
+                     const std::string& event_name);
+  // Cancel the watch event. If tracing is enabled, this may race with the
+  // watch event notification firing.
+  void CancelWatchEvent();
+
   int process_id() const { return process_id_; }
 
   // Exposed for unittesting:
@@ -287,6 +308,26 @@ class BASE_EXPORT TraceLog {
   // by the Singleton class.
   friend struct StaticMemorySingletonTraits<TraceLog>;
 
+  // Helper class for managing notification_thread_count_ and running
+  // notification callbacks. This is very similar to a reader-writer lock, but
+  // shares the lock with TraceLog and manages the notification flags.
+  class NotificationHelper {
+   public:
+    inline NotificationHelper(TraceLog* trace_log);

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

nit: explicit.

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

Done.

+    inline ~NotificationHelper();
+
+    // Called only while TraceLog::lock_ is held.
+    inline void AddNotificationWhileLocked(int notification);

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

What is the distinction betwene "AddNotification" and "SendNotification"?

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

Updated comments.

+
+    // Called only while TraceLog::lock_ is NOT held.
+    inline void SendNotificationIfAny();
+
+   private:
+    TraceLog* trace_log_;
+    NotificationCallback callback_copy_;
+    int notification_;
+  };
+
   TraceLog();
   ~TraceLog();
   const unsigned char* GetCategoryEnabledInternal(const char* name);
@@ -297,8 +338,10 @@ class BASE_EXPORT TraceLog {
   // synchronization.
   Lock lock_;

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

Can you clarify what is protected by this lock?

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

Done.

   bool enabled_;
-  OutputCallback output_callback_;
-  BufferFullCallback buffer_full_callback_;
+  base::ConditionVariable notification_condition_;
+  NotificationCallback notification_callback_;
+  // Number of threads that have a copy of the notification callback.
+  int notification_thread_count_;
   std::vector<TraceEvent> logged_events_;
   std::vector<std::string> included_categories_;
   std::vector<std::string> excluded_categories_;
@@ -312,6 +355,10 @@ class BASE_EXPORT TraceLog {
 
   int process_id_;
 
+  // Allow tests to wake up when certain events occur.
+  const unsigned char* watch_category_;
+  std::string watch_event_name_;
+
   DISALLOW_COPY_AND_ASSIGN(TraceLog);
 };