LoggingLock::Init() should append a non-empty string to

base/logging.cc

 // Copyright (c) 2012 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 193 matching lines @@
   }
 
   static void Init(LogLockingState lock_log, const PathChar* new_log_file) {
     if (initialized)
       return;
     lock_log_file = lock_log;
     if (lock_log_file == LOCK_LOG_FILE) {
 #if defined(OS_WIN)
       if (!log_mutex) {
         std::wstring safe_name;
-        if (new_log_file)
+        if (!new_log_file) {
+          safe_name = GetDefaultLogFile();
+        } else if (new_log_file[0]) {

[brettw, 2012/09/11 19:20:54]

Should we just be asserting that people don't give us empty non-null strings?
Otherwise there should be a comment about why this is necessary.

[wtc, 2012/09/11 21:44:46]

We can't assert that because in Release builds, Chrome
passes an empty non-null string to indicate it is not using
a log file.

Another solution is for Chrome to pass the L"<NotLoggingToFile>"
string to this function under that condition. But it seems
better to handle that in this function.

I will add a comment to explain this. I can also add
      CHECK(!safe_name.empty());
before the t.append(safe_name) call on line 224.

[wtc, 2012/09/11 22:36:50]

For this function (LoggingLock::Init), any non-empty string
will do, because it only uses the |new_log_file| argument
to construct the mutex name. By using L"<NotLoggingToFile>"
I am trying to make the mutex name more informative.
This function can certainly use GetDefaultLogFile() in the
mutex name.

Note that there are two default log file names -- a default
for base/logging.cc itself, and a default for Chrome.  To
get the default log file name for Chrome requires access to
the user profile. jschuh made a change to avoid getting the
default log file name, so that we don't access the profile
when we are not logging to a file. After his change, Chrome
passes a non-null empty string in that case.

I have to admit after a lot of code browsing I am not sure
I fully understand this code. I did find some code in the
source tree that passes new_log_file=NULL to this function.
So it seems that new_log_file=NULL means "use the default
log file name for base".

[brettw, 2012/09/11 22:43:31]

Let's add a comment here that if the client sent non-null but an empty string,
we shouldn't bother to compute the filename, but can't provide an empty mutex
name. I'd also like to specify this behavior in the header. It doesn't even
mention what NULL means, and it should also mention that the empty string may be
used if logging to a file isn't specified.

[wtc, 2012/09/12 20:29:57]

I will add some comments to base/logging.h to document my
understanding of the log locking code.

Since a change to base/logging.h will cause many files to
recompile, I will do that in a separate CL, after the M23
branch point.

           safe_name = new_log_file;
-        else
-          safe_name = GetDefaultLogFile();
+        } else {
+          safe_name = L"<NotLoggingToFile>";
+        }

[wtc, 2012/09/11 04:17:49]

I am assuming |new_log_file| may have three cases.

1. new_log_file == NULL: this case means we want to use the
default log file name ("debug.log" in the application
directory).

2. new_log_file is a non-empty string.

3. new_log_file is an empty string: this case means we are
not logging to a file.

Is this correct?

         // \ is not a legal character in mutex names so we replace \ with /
         std::replace(safe_name.begin(), safe_name.end(), '\\', '/');
         std::wstring t(L"Global\\");
         t.append(safe_name);
         log_mutex = ::CreateMutex(NULL, FALSE, t.c_str());
 
         if (log_mutex == NULL) {
 #if DEBUG
           // Keep the error code for debugging
           int error = GetLastError();  // NOLINT
@@ skipping 623 matching lines @@
 }
 
 // This was defined at the beginning of this file.
 #undef write
 
 }  // namespace logging
 
 std::ostream& operator<<(std::ostream& out, const wchar_t* wstr) {
   return out << WideToUTF8(std::wstring(wstr));
 }