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

Don't lock the log file unless we are logging to a file.

R=jschuh@chromium.org,davemoore@chromium.org,brettw@chromium.org
BUG=146406
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156750

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

Sorry to ask three code reviewers. Here is why I ask
you to review this CL.

jschuh: you may have introduced this regression.

davemoore: you wrote the LoggingLock code originally.

brettw: you are an owner of base.

http://crbug.com/146406 has more info on this bug.  Thanks.

https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc
File base/logging.cc (right):

https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc#newcod...
base/logging.cc:220: }

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?

[jschuh, 2012-09-11 07:24:24]

On 2012/09/11 04:17:49, wtc wrote:
> Sorry to ask three code reviewers. Here is why I ask
> you to review this CL.
> 
> jschuh: you may have introduced this regression.
> 
> davemoore: you wrote the LoggingLock code originally.
> 
> brettw: you are an owner of base.
> 
> http://crbug.com/146406 has more info on this bug.  Thanks.
> 
> https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc
> File base/logging.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc#newcod...
> base/logging.cc:220: }
> 
> 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?

lgtm. An empty log path means we're not logging to a file and don't need the
ALPC hole, but that doesn't preclude console or other logging targets.

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

https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc
File base/logging.cc (right):

https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc#newcod...
base/logging.cc:216: } else if (new_log_file[0]) {
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]

https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc
File base/logging.cc (right):

https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc#newcod...
base/logging.cc:216: } else if (new_log_file[0]) {

On 2012/09/11 19:20:54, brettw wrote:
> Should we just be asserting that people don't give us empty non-null strings?

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.

[brettw, 2012-09-11 21:54:59]

Is there a problem with passing the default log file name for the mutex name in
the release case?

I'm not sure it was on purpose that Chrome specifically passes the empty string
instead of null to InitLogging. It looks like we just have the file name in a
std::string so it was more convenient.

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

https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc
File base/logging.cc (right):

https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc#newcod...
base/logging.cc:216: } else if (new_log_file[0]) {

On 2012/09/11, brettw wrote:
> Is there a problem with passing the default log file
> name for the mutex name in the release case?

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]

https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc
File base/logging.cc (right):

https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc#newcod...
base/logging.cc:216: } else if (new_log_file[0]) {
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:27:09]

jschuh,brettw: please review patch set 2.

After studying the log locking code, I came up with a different
solution.

I verified that we don't need the log file lock in the Release
build of Chrome.

I included base/logging.cc temporarily in this CL just to
provide context for code review. I will remove base/logging.cc
from the CL before I commit this.

https://chromiumcodereview.appspot.com/10909164/diff/8001/base/logging.cc
File base/logging.cc (right):

https://chromiumcodereview.appspot.com/10909164/diff/8001/base/logging.cc#new...
base/logging.cc:623: LoggingLock logging_lock;

This is the only place where we write to a log file.

Note that we won't get here in a Release build of Chrome
because logging_destination is LOG_NONE. So LoggingLock
does NOT need to lock the log file if logging_destination
is LOG_NONE or LOG_ONLY_TO_SYSTEM_DEBUG_LOG.

https://chromiumcodereview.appspot.com/10909164/diff/8001/chrome/common/loggi...
File chrome/common/logging_chrome.cc (right):

https://chromiumcodereview.appspot.com/10909164/diff/8001/chrome/common/loggi...
chrome/common/logging_chrome.cc:296: log_locking_state = DONT_LOCK_LOG_FILE;

LogLockingState has four possible values:

enum LoggingDestination { LOG_NONE,
                          LOG_ONLY_TO_FILE,
                          LOG_ONLY_TO_SYSTEM_DEBUG_LOG,
                          LOG_TO_BOTH_FILE_AND_SYSTEM_DEBUG_LOG };

So if we get here, logging_dest must be either LOG_NONE or
LOG_ONLY_TO_SYSTEM_DEBUG_LOG.

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

https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc
File base/logging.cc (right):

https://chromiumcodereview.appspot.com/10909164/diff/1/base/logging.cc#newcod...
base/logging.cc:216: } else if (new_log_file[0]) {

On 2012/09/11 22:43:31, brettw wrote:
> 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.

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.

[brettw, 2012-09-13 19:47:08]

lgtm

[DaveMoore, 2012-09-13 20:22:46]

lgtm

[commit-bot: I haz the power, 2012-09-14 03:39:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/10909164/10004

[commit-bot: I haz the power, 2012-09-14 05:44:01]

Change committed as 156750