Moving ScopedMockLog from net/test to base/test.

I want to reuse ScopedMockLog from unittests under src/remoting/host.
To do that, I am moving this class from net/test to base/test.

When doing the move, I also wanted to ensure thread-safety in the case when
logging is happening on a thread different from where StartCapturingLogs or
StopCapturingLogs are called.  Having proper locks (and memory barriers implied
by the locks) should ensure that 1) LogMessageHandler won't see a half-way
executed StartCapturingLogs or StopCapturingLogs and 2) that a log write
in-progress won't get a rug pulled from underneath by destroying of gMock's
structures embedded in ScopedMockLog's Log mock method.

BUG=
TEST=net_unittests

Review URL: https://codereview.chromium.org/966423003

Cr-Commit-Position: refs/heads/master@{#319105}

base/base.gyp

@@ -938,6 +938,8 @@
         'test/mock_chrome_application_mac.mm',
         'test/mock_devices_changed_observer.cc',
         'test/mock_devices_changed_observer.h',
+        'test/mock_log.cc',
+        'test/mock_log.h',
         'test/multiprocess_test.cc',
         'test/multiprocess_test.h',
         'test/multiprocess_test_android.cc',

base/test/BUILD.gn

@@ -36,6 +36,8 @@ source_set("test_support") {
     "mock_chrome_application_mac.mm",
     "mock_devices_changed_observer.cc",
     "mock_devices_changed_observer.h",
+    "mock_log.cc",
+    "mock_log.h",
     "multiprocess_test.cc",
     "multiprocess_test.h",
     "multiprocess_test_android.cc",

net/test/scoped_mock_log.cc → base/test/mock_log.cc

-// Copyright 2014 The Chromium Authors. All rights reserved.
+// Copyright 2015 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 "net/test/scoped_mock_log.h"
+#include "base/test/mock_log.h"
 
-#include "base/logging.h"
-
-namespace net {
+namespace base {
 namespace test {
 
 // static
-ScopedMockLog* ScopedMockLog::g_instance_ = NULL;
+MockLog* MockLog::g_instance_ = nullptr;
+Lock MockLog::g_lock;
 
-ScopedMockLog::ScopedMockLog() : is_capturing_logs_(false) {}
+MockLog::MockLog() : is_capturing_logs_(false) {
+}
 
-ScopedMockLog::~ScopedMockLog() {
+MockLog::~MockLog() {
   if (is_capturing_logs_) {
     StopCapturingLogs();
   }
 }
 
-void ScopedMockLog::StartCapturingLogs() {
+void MockLog::StartCapturingLogs() {
+  AutoLock scoped_lock(g_lock);
+
   // We don't use CHECK(), which can generate a new LOG message, and
-  // thus can confuse ScopedMockLog objects or other registered
+  // thus can confuse MockLog objects or other registered
   // LogSinks.
   RAW_CHECK(!is_capturing_logs_);
   RAW_CHECK(!g_instance_);
@@ -33,26 +35,34 @@ void ScopedMockLog::StartCapturingLogs() {
   logging::SetLogMessageHandler(LogMessageHandler);
 }
 
-void ScopedMockLog::StopCapturingLogs() {
+void MockLog::StopCapturingLogs() {
+  AutoLock scoped_lock(g_lock);
+
   // We don't use CHECK(), which can generate a new LOG message, and
-  // thus can confuse ScopedMockLog objects or other registered
+  // thus can confuse MockLog objects or other registered
   // LogSinks.
   RAW_CHECK(is_capturing_logs_);
   RAW_CHECK(g_instance_ == this);
 
   is_capturing_logs_ = false;
   logging::SetLogMessageHandler(previous_handler_);
-  g_instance_ = NULL;
+  g_instance_ = nullptr;
 }
 
 // static
-bool ScopedMockLog::LogMessageHandler(int severity,
+bool MockLog::LogMessageHandler(int severity,
                                 const char* file,
                                 int line,
                                 size_t message_start,
                                 const std::string& str) {
+  // gMock guarantees thread-safety for calling a mocked method
+  // (https://code.google.com/p/googlemock/wiki/CookBook#Using_Google_Mock_and_Threads)
+  // but we also need to make sure that Start/StopCapturingLogs are synchronized
+  // with LogMessageHandler.
+  AutoLock scoped_lock(g_lock);
+
   return g_instance_->Log(severity, file, line, message_start, str);
 }
 
 }  // namespace test
-}  // namespace net
+}  // namespace base

net/test/scoped_mock_log.h → base/test/mock_log.h

-// Copyright 2014 The Chromium Authors. All rights reserved.
+// Copyright 2015 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.
 
-#ifndef NET_QUIC_TEST_TOOLS_SCOPED_MOCK_LOG_H_
-#define NET_QUIC_TEST_TOOLS_SCOPED_MOCK_LOG_H_
+#ifndef BASE_TEST_MOCK_LOG_H_
+#define BASE_TEST_MOCK_LOG_H_
+
+#include <string>
 
 #include "base/logging.h"
+#include "base/macros.h"
+#include "base/synchronization/lock.h"
 #include "testing/gmock/include/gmock/gmock.h"
-#include "testing/gtest/include/gtest/gtest.h"
 
-namespace net {
+namespace base {
 namespace test {
 
-// A ScopedMockLog object intercepts LOG() messages issued during its
-// lifespan.  Using this together with gMock, it's very easy to test
-// how a piece of code calls LOG().  The typical usage:
+// A MockLog object intercepts LOG() messages issued during its lifespan.  Using
+// this together with gMock, it's very easy to test how a piece of code calls
+// LOG().  The typical usage:
 //
 //   TEST(FooTest, LogsCorrectly) {
-//     ScopedMockLog log;
+//     MockLog log;
 //
 //     // We expect the WARNING "Something bad!" exactly twice.
 //     EXPECT_CALL(log, Log(WARNING, _, "Something bad!"))
@@ -31,51 +34,46 @@ namespace test {
 //     Foo();  // Exercises the code under test.
 //   }
 //
-// CAVEAT: base/logging does not allow a thread to call LOG() again
-// when it's already inside a LOG() call.  Doing so will cause a
-// deadlock.  Therefore, it's the user's responsibility to not call
-// LOG() in an action triggered by ScopedMockLog::Log().  You may call
-// RAW_LOG() instead.
-class ScopedMockLog {
+// CAVEAT: base/logging does not allow a thread to call LOG() again when it's
+// already inside a LOG() call.  Doing so will cause a deadlock.  Therefore,
+// it's the user's responsibility to not call LOG() in an action triggered by
+// MockLog::Log().  You may call RAW_LOG() instead.
+class MockLog {
  public:
-  // Creates a ScopedMockLog object that is not capturing logs.
-  // If it were to start to capture logs, it could be a problem if
-  // some other threads already exist and are logging, as the user
-  // hasn't had a chance to set up expectation on this object yet
-  // (calling a mock method before setting the expectation is
+  // Creates a MockLog object that is not capturing logs.  If it were to start
+  // to capture logs, it could be a problem if some other threads already exist
+  // and are logging, as the user hasn't had a chance to set up expectation on
+  // this object yet (calling a mock method before setting the expectation is
   // UNDEFINED behavior).
-  ScopedMockLog();
+  MockLog();
 
   // When the object is destructed, it stops intercepting logs.
-  ~ScopedMockLog();
+  ~MockLog();
 
   // Starts log capturing if the object isn't already doing so.
-  // Otherwise crashes.  Usually this method is called in the same
-  // thread that created this object.  It is the user's responsibility
-  // to not call this method if another thread may be calling it or
-  // StopCapturingLogs() at the same time.
+  // Otherwise crashes.
   void StartCapturingLogs();
 
-  // Stops log capturing if the object is capturing logs.  Otherwise
-  // crashes.  Usually this method is called in the same thread that
-  // created this object.  It is the user's responsibility to not call
-  // this method if another thread may be calling it or
-  // StartCapturingLogs() at the same time.
+  // Stops log capturing if the object is capturing logs.  Otherwise crashes.
   void StopCapturingLogs();
 
-  // Sets the Log Message Handler that gets passed every log message before
-  // it's sent to other log destinations (if any).
-  // Returns true to signal that it handled the message and the message
-  // should not be sent to other log destinations.
-  MOCK_METHOD5(Log, bool(int severity,
+  // Log method is invoked for every log message before it's sent to other log
+  // destinations (if any).  The method should return true to signal that it
+  // handled the message and the message should not be sent to other log
+  // destinations.
+  MOCK_METHOD5(Log,
+               bool(int severity,
                     const char* file,
                     int line,
                     size_t message_start,
                     const std::string& str));
 
  private:
-  // The currently active scoped mock log.
-  static ScopedMockLog* g_instance_;
+  // The currently active mock log.
+  static MockLog* g_instance_;
+
+  // Lock protecting access to g_instance_.
+  static Lock g_lock;
 
   // Static function which is set as the logging message handler.
   // Called once for each message.
@@ -88,11 +86,13 @@ class ScopedMockLog {
   // True if this object is currently capturing logs.
   bool is_capturing_logs_;
 
-  // The previous handler to restore when the ScopedMockLog is destroyed.
+  // The previous handler to restore when the MockLog is destroyed.
   logging::LogMessageHandlerFunction previous_handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockLog);
 };
 
 }  // namespace test
-}  // namespace net
+}  // namespace base
 
-#endif  // NET_QUIC_TEST_TOOLS_SCOPED_MOCK_LOG_H_
+#endif  // BASE_TEST_MOCK_LOG_H_

net/net.gypi

@@ -1678,8 +1678,6 @@
       'test/run_all_unittests.cc',
       'test/scoped_disable_exit_on_dfatal.cc',
       'test/scoped_disable_exit_on_dfatal.h',
-      'test/scoped_mock_log.cc',
-      'test/scoped_mock_log.h',
       'test/test_certificate_data.h',
       'tools/balsa/balsa_frame_test.cc',
       'tools/balsa/balsa_headers_test.cc',

net/quic/congestion_control/rtt_stats_test.cc

@@ -7,7 +7,7 @@
 #include <vector>
 
 #include "base/logging.h"
-#include "net/test/scoped_mock_log.h"
+#include "base/test/mock_log.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using logging::LOG_WARNING;
@@ -226,7 +226,7 @@ TEST_F(RttStatsTest, ExpireSmoothedMetrics) {
 
 TEST_F(RttStatsTest, UpdateRttWithBadSendDeltas) {
   // Make sure we ignore bad RTTs.
-  ScopedMockLog log;
+  base::test::MockLog log;
 
   QuicTime::Delta initial_rtt = QuicTime::Delta::FromMilliseconds(10);
   rtt_stats_.UpdateRtt(initial_rtt, QuicTime::Delta::Zero(), QuicTime::Zero());

net/test/gtest_util.h

@@ -7,8 +7,8 @@
 #ifndef NET_QUIC_TEST_TOOLS_GTEST_UTIL_H_
 #define NET_QUIC_TEST_TOOLS_GTEST_UTIL_H_
 
+#include "base/test/mock_log.h"
 #include "net/test/scoped_disable_exit_on_dfatal.h"
-#include "net/test/scoped_mock_log.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -20,7 +20,7 @@ namespace test {
 #define GTEST_DFATAL_(statement, matcher, fail)                        \
   GTEST_AMBIGUOUS_ELSE_BLOCKER_                                        \
   if (true) {                                                          \
-    ::net::test::ScopedMockLog gtest_log;                               \
+    ::base::test::MockLog gtest_log;                                   \
     ::net::test::ScopedDisableExitOnDFatal gtest_disable_exit;         \
     using ::testing::_;                                                \
     EXPECT_CALL(gtest_log, Log(_, _, _, _, _))                         \
@@ -35,8 +35,7 @@ namespace test {
       goto GTEST_CONCAT_TOKEN_(gtest_label_dfatal_, __LINE__);         \
     }                                                                  \
   } else                                                               \
-    GTEST_CONCAT_TOKEN_(gtest_label_dfatal_, __LINE__):                 \
-        fail("")
+  GTEST_CONCAT_TOKEN_(gtest_label_dfatal_, __LINE__) : fail("")
 
 // The EXPECT_DFATAL and ASSERT_DFATAL macros are lightweight
 // alternatives to EXPECT_DEBUG_DEATH and ASSERT_DEBUG_DEATH. They