Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(330)

Issue 1352713002: Get logging to chrome_debug.log working again on Windows Vista+. (Closed)

Created:
5 years, 3 months ago by ananta
Modified:
5 years, 3 months ago
CC:
chromium-reviews, darin-cc_chromium.org, jam, rickyz+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Get logging to chrome_debug.log working again on Windows Vista+. Logging to the chrome_debug.log file is supposed to work across the browser, renderer, gpu process, etc. However currently it only works in the browser. Reason being the chrome_debug.log file lives in the same directory as chrome.exe and the fact that the GPU and renderer processes are launched in low integrity on Vista+. In low integrity mode you have write access to folders and files which are marked as being accessible from low and no write up, everywhere else. Fix is to not initialize the logging system during the presandbox warmup on Windows and do it during sandbox warmup. We also add a filesystem policy to allow access to the log file from the renderer and GPU process. The other change here is to remove the mutex and SetFilePointer code in the logging subsystem on Windows. These exist for atomically appending to the log file. Windows provides that functionality via the FILE_APPEND_DATA access mask. BUG=532660 Committed: https://crrev.com/61762fb871f414a2987b635aa249e3a9c57a4290 Cr-Commit-Position: refs/heads/master@{#349561}

Patch Set 1 #

Total comments: 7

Patch Set 2 : Address review comments #

Total comments: 8

Patch Set 3 : Review comments second round #

Total comments: 3

Patch Set 4 : Remove stray comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+69 lines, -56 lines) Patch
M base/logging.h View 1 chunk +3 lines, -0 lines 0 comments Download
M base/logging.cc View 1 2 3 12 chunks +37 lines, -49 lines 0 comments Download
M chrome/app/chrome_main_delegate.cc View 1 3 chunks +17 lines, -7 lines 0 comments Download
M content/common/sandbox_win.cc View 1 2 2 chunks +12 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 20 (5 generated)
ananta
5 years, 3 months ago (2015-09-16 23:05:24 UTC) #2
scottmg
https://codereview.chromium.org/1352713002/diff/1/base/logging.cc File base/logging.cc (right): https://codereview.chromium.org/1352713002/diff/1/base/logging.cc#newcode274 base/logging.cc:274: g_log_file = CreateFile(g_log_file_name->c_str(), FILE_APPEND_DATA, do we need FILE_APPEND_DATA | ...
5 years, 3 months ago (2015-09-16 23:30:47 UTC) #3
scottmg
also +jam just as fyi since we mucked with logging a bunch recently, in case ...
5 years, 3 months ago (2015-09-16 23:34:58 UTC) #5
ananta
https://codereview.chromium.org/1352713002/diff/1/base/logging.cc File base/logging.cc (right): https://codereview.chromium.org/1352713002/diff/1/base/logging.cc#newcode274 base/logging.cc:274: g_log_file = CreateFile(g_log_file_name->c_str(), FILE_APPEND_DATA, On 2015/09/16 23:30:47, scottmg wrote: ...
5 years, 3 months ago (2015-09-16 23:49:38 UTC) #6
scottmg
otherwise, lgtm (but i don't OWNERS anything here) https://codereview.chromium.org/1352713002/diff/20001/base/logging.cc File base/logging.cc (right): https://codereview.chromium.org/1352713002/diff/20001/base/logging.cc#newcode69 base/logging.cc:69: #if ...
5 years, 3 months ago (2015-09-16 23:56:51 UTC) #7
ananta
https://codereview.chromium.org/1352713002/diff/20001/base/logging.cc File base/logging.cc (right): https://codereview.chromium.org/1352713002/diff/20001/base/logging.cc#newcode69 base/logging.cc:69: #if defined(OS_WIN) On 2015/09/16 23:56:51, scottmg wrote: > put ...
5 years, 3 months ago (2015-09-17 00:06:00 UTC) #8
ananta
+rvargas for owners
5 years, 3 months ago (2015-09-17 00:08:56 UTC) #10
scottmg
https://codereview.chromium.org/1352713002/diff/40001/base/logging.cc File base/logging.cc (right): https://codereview.chromium.org/1352713002/diff/40001/base/logging.cc#newcode278 base/logging.cc:278: // https://msdn.microsoft.com/en-us/library/windows/desktop/aa364399(v=vs.85).aspx nit; (you could delete the "(v=vs.85)" from ...
5 years, 3 months ago (2015-09-17 00:17:40 UTC) #11
jam
lgtm
5 years, 3 months ago (2015-09-17 14:47:34 UTC) #12
jschuh
lgtm on the sandbox change.
5 years, 3 months ago (2015-09-17 15:55:36 UTC) #13
rvargas (doing something else)
lgtm https://codereview.chromium.org/1352713002/diff/40001/base/logging.cc File base/logging.cc (right): https://codereview.chromium.org/1352713002/diff/40001/base/logging.cc#newcode362 base/logging.cc:362: // We don't need a lock on Windows ...
5 years, 3 months ago (2015-09-17 18:51:58 UTC) #14
ananta
https://codereview.chromium.org/1352713002/diff/40001/base/logging.cc File base/logging.cc (right): https://codereview.chromium.org/1352713002/diff/40001/base/logging.cc#newcode362 base/logging.cc:362: // We don't need a lock on Windows for ...
5 years, 3 months ago (2015-09-17 20:34:59 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1352713002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1352713002/60001
5 years, 3 months ago (2015-09-17 20:36:01 UTC) #18
commit-bot: I haz the power
Committed patchset #4 (id:60001)
5 years, 3 months ago (2015-09-18 01:00:16 UTC) #19
commit-bot: I haz the power
5 years, 3 months ago (2015-09-18 01:01:00 UTC) #20
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/61762fb871f414a2987b635aa249e3a9c57a4290
Cr-Commit-Position: refs/heads/master@{#349561}

Powered by Google App Engine
This is Rietveld 408576698