[tracing] Implement Add/RemoveTraceStateObserver for default platform.

src/libplatform/tracing/tracing-controller.cc

Index: src/libplatform/tracing/tracing-controller.cc
diff --git a/src/libplatform/tracing/tracing-controller.cc b/src/libplatform/tracing/tracing-controller.cc
index e9a21725e289fd565391084a0b933a00c82df407..822c396fc6f605d260c77b5ae4b1048ddc991b9d 100644
--- a/src/libplatform/tracing/tracing-controller.cc
+++ b/src/libplatform/tracing/tracing-controller.cc
@@ -38,8 +38,13 @@ const int g_num_builtin_categories = 4;
 // Skip default categories.
 v8::base::AtomicWord g_category_index = g_num_builtin_categories;
 
+TracingController::TracingController() {}
+
+TracingController::~TracingController() {}
+
 void TracingController::Initialize(TraceBuffer* trace_buffer) {
   trace_buffer_.reset(trace_buffer);
+  mutex_.reset(new base::Mutex());
 }
 
 uint64_t TracingController::AddTraceEvent(
@@ -93,11 +98,27 @@ void TracingController::StartTracing(TraceConfig* trace_config) {
   trace_config_.reset(trace_config);
   mode_ = RECORDING_MODE;
   UpdateCategoryGroupEnabledFlags();
+  std::unordered_set<Platform::TraceStateObserver*> observers_copy;
+  {
+    base::LockGuard<base::Mutex> lock(mutex_.get());
+    observers_copy = observers_;

[fmeawad, 2016/09/27 18:05:49]

nit: I would argue that the copy constructor has the same overhead as the
iterator below. So I would put the iterator inside the lock

[caseq, 2016/09/27 18:09:04]

No. This is about not invoking callbacks from under a lock.

+  }
+  for (auto o : observers_copy) {
+    o->OnTraceEnabled();
+  }
 }
 
 void TracingController::StopTracing() {
   mode_ = DISABLED;
   UpdateCategoryGroupEnabledFlags();
+  std::unordered_set<Platform::TraceStateObserver*> observers_copy;
+  {
+    base::LockGuard<base::Mutex> lock(mutex_.get());
+    observers_copy = observers_;
+  }
+  for (auto o : observers_copy) {
+    o->OnTraceDisabled();

[caseq, 2016/09/27 17:50:22]

I'm a bit concerned about this being potentially called from
DefaultPlatform::~DefaultPlatform(), from under a lock and with parts of the
default platform already dead. Should we just ignore/clear the observers within
the destructor?

[fmeawad, 2016/09/27 18:05:49]

We need to disable the observer on stop tracing, if we are collecting cpu
profile, we should stop and not wait until the ~ is called.

[alph, 2016/09/27 19:10:53]

I moved StopTracing to the very beginning of platform destructor outside of
lock.

+  }
   trace_buffer_->Flush();
 }
 
@@ -172,6 +193,19 @@ const uint8_t* TracingController::GetCategoryGroupEnabledInternal(
   return category_group_enabled;
 }
 
+void TracingController::AddTraceStateObserver(
+    Platform::TraceStateObserver* observer) {
+  base::LockGuard<base::Mutex> lock(mutex_.get());
+  observers_.insert(observer);
+}
+
+void TracingController::RemoveTraceStateObserver(
+    Platform::TraceStateObserver* observer) {
+  base::LockGuard<base::Mutex> lock(mutex_.get());
+  DCHECK(observers_.find(observer) != observers_.end());
+  observers_.erase(observer);
+}
+
 }  // namespace tracing
 }  // namespace platform
 }  // namespace v8