SetLogger should delete previously set custom logger. (#853)

As specified in the doc comment for SetLogger, "the logger becomes the
property of the logging module and should not be deleted by the caller".

Not only should the LogDestination delete a custom logger in its
destructor, but it should also delete a previous logger when another
logger is passed to SetLogger().

Co-authored-by: Sergiu Deitsch <sergiud@users.noreply.github.com>

src/logging.cc

@@ -594,6 +594,10 @@ class LogDestination {
 
   static LogDestination* log_destination(LogSeverity severity);
 
+  base::Logger* GetLoggerImpl() const { return logger_; }
+  void SetLoggerImpl(base::Logger* logger);
+  void ResetLoggerImpl() { SetLoggerImpl(&fileobject_); }
+
   LogFileObject fileobject_;
   base::Logger* logger_;      // Either &fileobject_, or wrapper around it
 
@@ -643,10 +647,20 @@ LogDestination::LogDestination(LogSeverity severity,
 }
 
 LogDestination::~LogDestination() {
+  ResetLoggerImpl();
+}
+
+void LogDestination::SetLoggerImpl(base::Logger* logger) {
+  if (logger_ == logger) {
+    // Prevent releasing currently held sink on reset
+    return;
+  }
+
   if (logger_ && logger_ != &fileobject_) {
     // Delete user-specified logger set via SetLogger().
     delete logger_;
   }
+  logger_ = logger;
 }
 
 inline void LogDestination::FlushLogFilesUnsafe(int min_severity) {
@@ -2021,12 +2035,12 @@ void LogMessage::SendToSyslogAndLog() {
 
 base::Logger* base::GetLogger(LogSeverity severity) {
   MutexLock l(&log_mutex);
-  return LogDestination::log_destination(severity)->logger_;
+  return LogDestination::log_destination(severity)->GetLoggerImpl();
 }
 
 void base::SetLogger(LogSeverity severity, base::Logger* logger) {
   MutexLock l(&log_mutex);
-  LogDestination::log_destination(severity)->logger_ = logger;
+  LogDestination::log_destination(severity)->SetLoggerImpl(logger);
 }
 
 // L < log_mutex.  Acquires and releases mutex_.

src/logging_custom_prefix_unittest.cc

@@ -842,6 +842,11 @@ static void TestExtension() {
 struct MyLogger : public base::Logger {
   string data;
 
+  explicit MyLogger(bool* set_on_destruction)
+      : set_on_destruction_(set_on_destruction) {}
+
+  ~MyLogger() { *set_on_destruction_ = true; }
+
   virtual void Write(bool /* should_flush */,
                      time_t /* timestamp */,
                      const char* message,
@@ -852,19 +857,25 @@ struct MyLogger : public base::Logger {
   virtual void Flush() { }
 
   virtual uint32 LogSize() { return data.length(); }
+
+ private:
+  bool* set_on_destruction_;
 };
 
 static void TestWrapper() {
   fprintf(stderr, "==== Test log wrapper\n");
 
-  MyLogger my_logger;
+  bool custom_logger_deleted = false;
+  MyLogger* my_logger = new MyLogger(&custom_logger_deleted);
   base::Logger* old_logger = base::GetLogger(GLOG_INFO);
-  base::SetLogger(GLOG_INFO, &my_logger);
+  base::SetLogger(GLOG_INFO, my_logger);
   LOG(INFO) << "Send to wrapped logger";
+  CHECK(strstr(my_logger->data.c_str(), "Send to wrapped logger") != NULL);
   FlushLogFiles(GLOG_INFO);
-  base::SetLogger(GLOG_INFO, old_logger);
 
-  CHECK(strstr(my_logger.data.c_str(), "Send to wrapped logger") != NULL);
+  EXPECT_FALSE(custom_logger_deleted);
+  base::SetLogger(GLOG_INFO, old_logger);
+  EXPECT_TRUE(custom_logger_deleted);
 }
 
 static void TestErrno() {

src/logging_unittest.cc

@@ -853,6 +853,11 @@ static void TestExtension() {
 struct MyLogger : public base::Logger {
   string data;
 
+  explicit MyLogger(bool* set_on_destruction)
+      : set_on_destruction_(set_on_destruction) {}
+
+  ~MyLogger() { *set_on_destruction_ = true; }
+
   virtual void Write(bool /* should_flush */,
                      time_t /* timestamp */,
                      const char* message,
@@ -863,19 +868,25 @@ struct MyLogger : public base::Logger {
   virtual void Flush() { }
 
   virtual uint32 LogSize() { return data.length(); }
+
+ private:
+  bool* set_on_destruction_;
 };
 
 static void TestWrapper() {
   fprintf(stderr, "==== Test log wrapper\n");
 
-  MyLogger my_logger;
+  bool custom_logger_deleted = false;
+  MyLogger* my_logger = new MyLogger(&custom_logger_deleted);
   base::Logger* old_logger = base::GetLogger(GLOG_INFO);
-  base::SetLogger(GLOG_INFO, &my_logger);
+  base::SetLogger(GLOG_INFO, my_logger);
   LOG(INFO) << "Send to wrapped logger";
+  CHECK(strstr(my_logger->data.c_str(), "Send to wrapped logger") != NULL);
   FlushLogFiles(GLOG_INFO);
-  base::SetLogger(GLOG_INFO, old_logger);
 
-  CHECK(strstr(my_logger.data.c_str(), "Send to wrapped logger") != NULL);
+  EXPECT_FALSE(custom_logger_deleted);
+  base::SetLogger(GLOG_INFO, old_logger);
+  EXPECT_TRUE(custom_logger_deleted);
 }
 
 static void TestErrno() {