Lock all access to logging data, in case it's reinitialized

Lock all access to logging data, in case it's reinitialized

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

[brettw, 2010-08-23 21:47:20]

LGTM, just some style nits.

http://codereview.chromium.org/3195014/diff/1002/3001
File base/logging.cc (right):

http://codereview.chromium.org/3195014/diff/1002/3001#newcode199
base/logging.cc:199: #elif defined(OS_POSIX)
If there's nothing, can you just delete this "elif"?

http://codereview.chromium.org/3195014/diff/1002/3001#newcode242
base/logging.cc:242: // When we don't use a lock, we are using a global mutex.
We need to do this
Can you put a blank line above this to separate the lock from the mutex?

http://codereview.chromium.org/3195014/diff/1002/3001#newcode261
base/logging.cc:261: // When we don't use a lock, we are using a global mutex.
We need to do this
Seems unnecessary to duplicate this comment from above.

http://codereview.chromium.org/3195014/diff/1002/3001#newcode343
base/logging.cc:343: logging_destination == LOG_ONLY_TO_SYSTEM_DEBUG_LOG) {
We only require {} on multi-line contents, and the rest of this file doesn't use
{}, so you shouldn't be adding it.

http://codereview.chromium.org/3195014/diff/1002/3001#newcode594
base/logging.cc:594: // been created do it now.. We do this on demand, but if
two threads try
Can you fix the double period/

[DaveMoore, 2010-08-27 21:07:04]

Could you please check this again? It failed the try servers w/out these
additional changes.

http://codereview.chromium.org/3195014/diff/1002/3001
File base/logging.cc (right):

http://codereview.chromium.org/3195014/diff/1002/3001#newcode199
base/logging.cc:199: #elif defined(OS_POSIX)
On 2010/08/23 21:47:20, brettw wrote:
> If there's nothing, can you just delete this "elif"?
> 

Done.

http://codereview.chromium.org/3195014/diff/1002/3001#newcode242
base/logging.cc:242: // When we don't use a lock, we are using a global mutex.
We need to do this
On 2010/08/23 21:47:20, brettw wrote:
> Can you put a blank line above this to separate the lock from the mutex?

Done.

http://codereview.chromium.org/3195014/diff/1002/3001#newcode261
base/logging.cc:261: // When we don't use a lock, we are using a global mutex.
We need to do this
On 2010/08/23 21:47:20, brettw wrote:
> Seems unnecessary to duplicate this comment from above.

Done.

http://codereview.chromium.org/3195014/diff/1002/3001#newcode343
base/logging.cc:343: logging_destination == LOG_ONLY_TO_SYSTEM_DEBUG_LOG) {
On 2010/08/23 21:47:20, brettw wrote:
> We only require {} on multi-line contents, and the rest of this file doesn't
use
> {}, so you shouldn't be adding it.

Done.

http://codereview.chromium.org/3195014/diff/1002/3001#newcode594
base/logging.cc:594: // been created do it now.. We do this on demand, but if
two threads try
On 2010/08/23 21:47:20, brettw wrote:
> Can you fix the double period/

Done.

[brettw, 2010-08-27 21:11:59]

http://codereview.chromium.org/3195014/diff/8001/9001
File base/logging.cc (right):

http://codereview.chromium.org/3195014/diff/8001/9001#newcode193
base/logging.cc:193: wchar_t module_name[MAX_PATH];
Since this is just duplicated from below, can this be moved into a helper
function?

[DaveMoore, 2010-08-31 20:05:48]

http://codereview.chromium.org/3195014/diff/8001/9001
File base/logging.cc (right):

http://codereview.chromium.org/3195014/diff/8001/9001#newcode193
base/logging.cc:193: wchar_t module_name[MAX_PATH];
On 2010/08/27 21:11:59, brettw wrote:
> Since this is just duplicated from below, can this be moved into a helper
> function?

Done.

[brettw, 2010-08-31 20:25:25]

LGTM

[stoyan, 2010-09-01 22:36:40]

http://codereview.chromium.org/3195014/diff/14001/15001
File base/logging.cc (right):

http://codereview.chromium.org/3195014/diff/14001/15001#newcode215
base/logging.cc:215: log_mutex = ::CreateMutex(NULL, FALSE, t.c_str());
Will not work if invoked from renderer (low integrity) since browser (medium
integrity) already took the name (and use the default security descriptor).
Generally speaking "Global\\" prefix is almost always not a good idea.

http://codereview.chromium.org/3195014/diff/14001/15001#newcode333
base/logging.cc:333: *new_log_file_name = new_log_file;
new_log_file may be NULL if logging_destination is LOG_ONLY_TO_SYSTEM_DEBUG_LOG.
I guess this is the reason why ChromeFrame tests crashed.