Don't delete g_vlog_info

base/logging.cc

 // Copyright (c) 2011 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/logging.h"
 
 #if defined(OS_WIN)
 #include <io.h>
 #include <windows.h>
 typedef HANDLE FileHandle;
@@ skipping 46 matching lines @@
 #include "base/safe_strerror_posix.h"
 #endif
 
 #if defined(OS_ANDROID)
 #include <android/log.h>
 #endif
 
 namespace logging {
 
 DcheckState g_dcheck_state = DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS;
+
+VlogInfo* g_vlog_info_first = NULL;

[akalin, 2011/12/01 17:03:24]

static and/or anon namespace for these two? (I guess the other globals in this
file, too)

[stevenjb, 2011/12/01 19:09:38]

Done.

+VlogInfo* g_vlog_info_second = NULL;

[akalin, 2011/12/01 17:03:24]

might be clearer to just have "g_vlog_info" and "g_vlog_info_prev" instead of
three variables with two equal.

[stevenjb, 2011/12/01 19:09:38]

Done.

 VlogInfo* g_vlog_info = NULL;
 
 const char* const log_severity_names[LOG_NUM_SEVERITIES] = {
   "INFO", "WARNING", "ERROR", "ERROR_REPORT", "FATAL" };
 
 int min_log_level = 0;
 
 // The default set here for logging_destination will only be used if
 // InitLogging is not called.  On Windows, use a file next to the exe;
 // on POSIX platforms, where it may not even be possible to locate the
@@ skipping 273 matching lines @@
   return true;
 }
 
 bool BaseInitLoggingImpl(const PathChar* new_log_file,
                          LoggingDestination logging_dest,
                          LogLockingState lock_log,
                          OldFileDeletionState delete_old,
                          DcheckState dcheck_state) {
   CommandLine* command_line = CommandLine::ForCurrentProcess();
   g_dcheck_state = dcheck_state;
-  delete g_vlog_info;
-  g_vlog_info = NULL;
+
+  // NOTE: If g_vlog_info has already been initialized, it might be in use
+  // by another thread. Don't delete the old VLogInfo, just abandon it and
+  // create a second one. We track both to avoid memory leak warnings.
+  CHECK(g_vlog_info_second == NULL);

[akalin, 2011/12/01 17:03:24]

usual style is !g_vlog_info_second

+
+  VlogInfo* vlog_info = NULL;

[oshima, 2011/12/01 16:28:48]

how about 

DCHECK(!vlog_info_second);
vlog_info_second = vlog_info;

inside if block below?

[stevenjb, 2011/12/01 16:51:41]

I'm not sure I understand your suggestion. I put the CHECK above to do it as
early as possible, but I could move it into the if() block if that woud be more
clear. I used CHECK instead of DCHECK to prevent a leak from showing up, but I
could use DCHECK if we are ok with that. (My experience is that ChromeOS Debug
builds rarely get run outside of some tests).

On 2011/12/01 16:28:48, oshima wrote:

[stevenjb, 2011/12/01 19:09:38]

After chatting + fred's feedback, I think this will be simpler.
Kept it as a CHECK instead of a DCHECK however to prevent more subtle races or
memory failures.

   // Don't bother initializing g_vlog_info unless we use one of the
   // vlog switches.
   if (command_line->HasSwitch(switches::kV) ||
       command_line->HasSwitch(switches::kVModule)) {
-    g_vlog_info =
+    vlog_info =
         new VlogInfo(command_line->GetSwitchValueASCII(switches::kV),
                      command_line->GetSwitchValueASCII(switches::kVModule),
                      &min_log_level);
   }
 
+  if (!g_vlog_info_first) {

[akalin, 2011/12/01 17:03:24]

rewrite to use positive test, i.e. if (g_vlog_info_first).

+    g_vlog_info_first = vlog_info;
+    g_vlog_info = g_vlog_info_first;

[akalin, 2011/12/01 17:03:24]

i couldn't understand what these two lines were doing until I read them a couple
times.  what about:

g_vlog_info = v_log_info;
if (g_vlog_info_first) {
  g_vlog_info_second = v_log_info;
} else {
  g_vlog_info_first = v_log_info;
}

[stevenjb, 2011/12/01 19:09:38]

Re-factored with oshima's feedback also.

+  } else {
+    g_vlog_info_second = vlog_info;
+    g_vlog_info = g_vlog_info_second;
+  }
+
   LoggingLock::Init(lock_log, new_log_file);
 
   LoggingLock logging_lock;
 
   if (log_file) {
     // calling InitLogging twice or after some log call has already opened the
     // default log file will re-initialize to the new options
     CloseFile(log_file);
     log_file = NULL;
   }
@@ skipping 21 matching lines @@
 int GetMinLogLevel() {
   return min_log_level;
 }
 
 int GetVlogVerbosity() {
   return std::max(-1, LOG_INFO - GetMinLogLevel());
 }
 
 int GetVlogLevelHelper(const char* file, size_t N) {
   DCHECK_GT(N, 0U);
-  return g_vlog_info ?
-      g_vlog_info->GetVlogLevel(base::StringPiece(file, N - 1)) :
+  VlogInfo* vlog_info = g_vlog_info;
+  return vlog_info ?
+      vlog_info->GetVlogLevel(base::StringPiece(file, N - 1)) :
       GetVlogVerbosity();
 }
 
 void SetLogItems(bool enable_process_id, bool enable_thread_id,
                  bool enable_timestamp, bool enable_tickcount) {
   log_process_id = enable_process_id;
   log_thread_id = enable_thread_id;
   log_timestamp = enable_timestamp;
   log_tickcount = enable_tickcount;
 }
@@ skipping 420 matching lines @@
 
 // This was defined at the beginnig of this file.
 #undef write
 
 std::ostream& operator<<(std::ostream& o, const StringPiece& piece) {
   o.write(piece.data(), static_cast<std::streamsize>(piece.size()));
   return o;
 }
 
 }  // namespace base