Fix event tracing for windows test flakyness.

Assert on non-null properties to the static members of the trace controller.
Fix usage of same.
Add a PostEventsDisabled method to trace producer to which allows for a non-racy enable/disable test.

BUG=52388
BUG=59328
TEST=Unittests in the change.

Review URL: http://codereview.chromium.org/3800006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@62766 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 40 insertions, 13 deletions

diff --git a/base/event_trace_consumer_win_unittest.cc b/base/event_trace_consumer_win_unittest.cc
index 6c0a740..c964412 100644
--- a/base/event_trace_consumer_win_unittest.cc
+++ b/base/event_trace_consumer_win_unittest.cc
@@ -72,7 +72,8 @@ const wchar_t* const kTestSessionName = L"TestLogSession";
 class EtwTraceConsumerBaseTest: public testing::Test {
  public:
   virtual void SetUp() {
-    EtwTraceController::Stop(kTestSessionName, NULL);
+    EtwTraceProperties ignore;
+    EtwTraceController::Stop(kTestSessionName, &ignore);
   }
 };
 
@@ -245,14 +246,16 @@ class EtwTraceConsumerDataTest: public testing::Test {
   }
 
   virtual void SetUp() {
-    EtwTraceController::Stop(kTestSessionName, NULL);
+    EtwTraceProperties prop;
+    EtwTraceController::Stop(kTestSessionName, &prop);
     // Construct a temp file name.
     ASSERT_TRUE(file_util::CreateTemporaryFile(&temp_file_));
   }
 
   virtual void TearDown() {
     EXPECT_TRUE(file_util::Delete(temp_file_, false));
-    EtwTraceController::Stop(kTestSessionName, NULL);
+    EtwTraceProperties ignore;
+    EtwTraceController::Stop(kTestSessionName, &ignore);
   }
 
   HRESULT LogEventToTempSession(PEVENT_TRACE_HEADER header) {
diff --git a/base/event_trace_controller_win.cc b/base/event_trace_controller_win.cc
index cb15150..c46164b 100644
--- a/base/event_trace_controller_win.cc
+++ b/base/event_trace_controller_win.cc
@@ -97,6 +97,7 @@ HRESULT EtwTraceController::Flush(EtwTraceProperties* properties) {
 
 HRESULT EtwTraceController::Start(const wchar_t* session_name,
     EtwTraceProperties* properties, TRACEHANDLE* session_handle) {
+  DCHECK(properties != NULL);
   ULONG err = ::StartTrace(session_handle, session_name, properties->get());
   return HRESULT_FROM_WIN32(err);
 }
@@ -110,6 +111,7 @@ HRESULT EtwTraceController::Query(const wchar_t* session_name,
 
 HRESULT EtwTraceController::Update(const wchar_t* session_name,
     EtwTraceProperties* properties) {
+  DCHECK(properties != NULL);
   ULONG err = ::ControlTrace(NULL, session_name, properties->get(),
                              EVENT_TRACE_CONTROL_UPDATE);
   return HRESULT_FROM_WIN32(err);
@@ -117,6 +119,7 @@ HRESULT EtwTraceController::Update(const wchar_t* session_name,
 
 HRESULT EtwTraceController::Stop(const wchar_t* session_name,
     EtwTraceProperties* properties) {
+  DCHECK(properties != NULL);
   ULONG err = ::ControlTrace(NULL, session_name, properties->get(),
                              EVENT_TRACE_CONTROL_STOP);
   return HRESULT_FROM_WIN32(err);
@@ -124,6 +127,7 @@ HRESULT EtwTraceController::Stop(const wchar_t* session_name,
 
 HRESULT EtwTraceController::Flush(const wchar_t* session_name,
     EtwTraceProperties* properties) {
+  DCHECK(properties != NULL);
   ULONG err = ::ControlTrace(NULL, session_name, properties->get(),
                              EVENT_TRACE_CONTROL_FLUSH);
   return HRESULT_FROM_WIN32(err);
diff --git a/base/event_trace_controller_win_unittest.cc b/base/event_trace_controller_win_unittest.cc
index 55222a6..5777e58 100644
--- a/base/event_trace_controller_win_unittest.cc
+++ b/base/event_trace_controller_win_unittest.cc
@@ -42,7 +42,7 @@ class TestingProvider: public EtwTraceProvider {
   virtual void OnEventsEnabled() {
     ::SetEvent(callback_event_.Get());
   }
-  virtual void OnEventsDisabled() {
+  virtual void PostEventsDisabled() {
     ::SetEvent(callback_event_.Get());
   }
 
@@ -55,7 +55,8 @@ class TestingProvider: public EtwTraceProvider {
 
 TEST(EtwTraceTest, Cleanup) {
   // Clean up potential leftover sessions from previous unsuccessful runs.
-  EtwTraceController::Stop(kTestSessionName, NULL);
+  EtwTraceProperties ignore;
+  EtwTraceController::Stop(kTestSessionName, &ignore);
 }
 
 TEST(EtwTracePropertiesTest, Initialization) {
@@ -152,8 +153,7 @@ TEST(EtwTraceControllerTest, StartFileSession) {
   EXPECT_STREQ(L"", controller.session_name());
 }
 
-// Flaky, http://crbug.com/59328.
-TEST(EtwTraceControllerTest, FLAKY_EnableDisable) {
+TEST(EtwTraceControllerTest, EnableDisable) {
   TestingProvider provider(kTestProvider);
 
   EXPECT_EQ(ERROR_SUCCESS, provider.Register());
diff --git a/base/event_trace_provider_win.cc b/base/event_trace_provider_win.cc
index c7f5001..826a4bf 100644
--- a/base/event_trace_provider_win.cc
+++ b/base/event_trace_provider_win.cc
@@ -48,6 +48,8 @@ ULONG EtwTraceProvider::DisableEvents() {
   enable_flags_ = 0;
   session_handle_ = NULL;
 
+  PostEventsDisabled();
+
   return ERROR_SUCCESS;
 }
 
diff --git a/base/event_trace_provider_win.h b/base/event_trace_provider_win.h
index b0526b1..5c89d1a 100644
--- a/base/event_trace_provider_win.h
+++ b/base/event_trace_provider_win.h
@@ -126,13 +126,25 @@ class EtwTraceProvider {
   ULONG Log(EVENT_TRACE_HEADER* event);
 
  protected:
-  // These are called after events have been enabled or disabled.
-  // Override them if you want to do processing at the start or
-  // end of collection.
-  // Note: These may be called ETW's thread and they may be racy.
+  // Called after events have been enabled, override in subclasses
+  // to set up state or log at the start of a session.
+  // Note: This function may be called ETW's thread and may be racy,
+  //    bring your own locking if needed.
   virtual void OnEventsEnabled() {}
+
+  // Called just before events are disabled, override in subclasses
+  // to tear down state or log at the end of a session.
+  // Note: This function may be called ETW's thread and may be racy,
+  //    bring your own locking if needed.
   virtual void OnEventsDisabled() {}
 
+  // Called just after events have been disabled, override in subclasses
+  // to tear down state at the end of a session. At this point it's
+  // to late to log anything to the session.
+  // Note: This function may be called ETW's thread and may be racy,
+  //    bring your own locking if needed.
+  virtual void PostEventsDisabled() {}
+
  private:
   ULONG EnableEvents(PVOID buffer);
   ULONG DisableEvents();
diff --git a/base/trace_event_win_unittest.cc b/base/trace_event_win_unittest.cc
index a142af1..1335bed 100644
--- a/base/trace_event_win_unittest.cc
+++ b/base/trace_event_win_unittest.cc
@@ -79,6 +79,13 @@ class TraceEventTest: public testing::Test {
   void SetUp() {
     bool is_xp = base::win::GetVersion() < base::win::VERSION_VISTA;
 
+    if (is_xp) {
+      // Tear down any dangling session from an earlier failing test.
+      EtwTraceProperties ignore;
+
+      EtwTraceController::Stop(kTestSessionName, &ignore);
+    }
+
     // Resurrect and initialize the TraceLog singleton instance.
     // On Vista and better, we need the provider registered before we
     // start the private, in-proc session, but on XP we need the global
@@ -254,8 +261,7 @@ TEST_F(TraceEventTest, TraceLog) {
   PlayLog();
 }
 
-// Marked flaky per http://crbug.com/52388
-TEST_F(TraceEventTest, FLAKY_Macros) {
+TEST_F(TraceEventTest, Macros) {
   ExpectPlayLog();
 
   // The events should arrive in the same sequence as the expects.