Don't delete g_vlog_info

base/logging.cc

Index: base/logging.cc
diff --git a/base/logging.cc b/base/logging.cc
index 27754af8406531c62ebf2eca7d8520c951c322e5..fd858285d4a99debcd05b8b7185f18919db624bf 100644
--- a/base/logging.cc
+++ b/base/logging.cc
@@ -64,6 +64,9 @@ typedef pthread_mutex_t* MutexHandle;
 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] = {
@@ -357,18 +360,31 @@ bool BaseInitLoggingImpl(const PathChar* new_log_file,
                          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;
@@ -410,8 +426,9 @@ int GetVlogVerbosity() {
 
 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();
 }