Protect TraceLog::enabled_state_observer_list_ with a dedicated lock

base/trace_event/trace_event_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/trace_event/trace_event.h"
 
 #include <math.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 1118 matching lines @@
   TraceLog::GetInstance()->SetEnabled(TraceConfig(kRecordAllCategoryFilter, ""),
                                       TraceLog::RECORDING_MODE);
   EXPECT_TRUE(TraceLog::GetInstance()->IsEnabled());
 
   TraceLog::GetInstance()->SetDisabled();
   EXPECT_FALSE(TraceLog::GetInstance()->IsEnabled());
 
   TraceLog::GetInstance()->RemoveEnabledStateObserver(&observer);
 }
 
-// Tests that a state observer can remove itself during a callback.
-class SelfRemovingEnabledStateObserver
-    : public TraceLog::EnabledStateObserver {
- public:
-  SelfRemovingEnabledStateObserver() {}
-  ~SelfRemovingEnabledStateObserver() override {}
-
-  // TraceLog::EnabledStateObserver overrides:
-  void OnTraceLogEnabled() override {}
-
-  void OnTraceLogDisabled() override {
-    TraceLog::GetInstance()->RemoveEnabledStateObserver(this);
-  }
-};
-
-TEST_F(TraceEventTestFixture, SelfRemovingObserver) {

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

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]

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.

-  ASSERT_EQ(0u, TraceLog::GetInstance()->GetObserverCountForTest());
-
-  SelfRemovingEnabledStateObserver observer;
-  TraceLog::GetInstance()->AddEnabledStateObserver(&observer);
-  EXPECT_EQ(1u, TraceLog::GetInstance()->GetObserverCountForTest());
-
-  TraceLog::GetInstance()->SetEnabled(TraceConfig(kRecordAllCategoryFilter, ""),
-                                      TraceLog::RECORDING_MODE);
-  TraceLog::GetInstance()->SetDisabled();
-  // The observer removed itself on disable.
-  EXPECT_EQ(0u, TraceLog::GetInstance()->GetObserverCountForTest());
-}
-
 bool IsNewTrace() {
   bool is_new_trace;
   TRACE_EVENT_IS_NEW_TRACE(&is_new_trace);
   return is_new_trace;
 }
 
 TEST_F(TraceEventTestFixture, NewTraceRecording) {
   ASSERT_FALSE(IsNewTrace());
   TraceLog::GetInstance()->SetEnabled(TraceConfig(kRecordAllCategoryFilter, ""),
                                       TraceLog::RECORDING_MODE);
@@ skipping 1916 matching lines @@
 }
 
 TEST_F(TraceEventTestFixture, SyntheticDelayConfigurationToString) {
   const char filter[] = "DELAY(test.Delay;16;oneshot)";
   TraceConfig config(filter, "");
   EXPECT_EQ(filter, config.ToCategoryFilterString());
 }
 
 }  // namespace trace_event
 }  // namespace base