|
|
Created:
6 years, 10 months ago by Cait (Slow) Modified:
6 years, 10 months ago CC:
chromium-reviews, tfarina, caitkp+watch_chromium.org, robertshield, csharp Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBreakpad coverage for chrome_elf start up
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252870
Patch Set 1 #
Total comments: 8
Patch Set 2 : More breakpad, less base #
Total comments: 10
Patch Set 3 : Address comments and catch crashes in blacklist intercept code #
Total comments: 8
Patch Set 4 : Reg key checks #
Total comments: 14
Patch Set 5 : Remove base dependency and clean up logic a bit #Patch Set 6 : Missed some nits #
Total comments: 33
Patch Set 7 : Use Environment Vars to get program dir #
Total comments: 56
Patch Set 8 : Tests and Greg's comments #
Total comments: 58
Patch Set 9 : Add policy checks and headlessness #
Total comments: 48
Patch Set 10 : More clean up #
Total comments: 10
Patch Set 11 : #
Total comments: 2
Patch Set 12 : rebase and last nits #Patch Set 13 : Fix tests on chromium builds #
Messages
Total messages: 31 (0 generated)
Siggi -- PTAL and let me know if I'm on the right track for the exception handling. Robert & Chris: FYI.
looks good so far :) https://codereview.chromium.org/154653002/diff/1/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/1/chrome_elf/breakpad.cc#newco... chrome_elf/breakpad.cc:141: // TODO(caitkp): Is terminating the right thing to do here? It's six of on, half a dozen of the other. If you continue the search here while inside DllMain, the loader will catch the exception. If this is at DLL_ATTACH time, the process will fail to run. https://codereview.chromium.org/154653002/diff/1/chrome_elf/breakpad.cc#newco... chrome_elf/breakpad.cc:179: return BreakpadWin::OnDumpRequested(exinfo); As we discussed, you may want to filter out debug break and single step exceptions here - have you tested this under debugger? https://codereview.chromium.org/154653002/diff/1/chrome_elf/chrome_elf_main.cc File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/154653002/diff/1/chrome_elf/chrome_elf_main.c... chrome_elf/chrome_elf_main.cc:19: // TODO (caitkp): Do we need this? If so, how do we make it play nice with A single function cannot have objects that require unwinding and __try/__except handlers. If you need this, you need to move the __try/__except into another function - a file static or anonymous namespace function is fine. https://codereview.chromium.org/154653002/diff/1/chrome_elf/chrome_elf_main.c... chrome_elf/chrome_elf_main.cc:33: // TODO(caitkp): We should really de-initialize our breakpad client here, to I don't know that this is true. As-is, you're also getting breakpad coverage for the initialization of all of chrome.exe's dependent DLLs.
+Robert, PTAL at breakpad.cc/h to make sure I didn't remove anything vital while gutting all the base dependencies. https://codereview.chromium.org/154653002/diff/1/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/1/chrome_elf/breakpad.cc#newco... chrome_elf/breakpad.cc:141: // TODO(caitkp): Is terminating the right thing to do here? On 2014/02/05 13:58:33, Sigurður Ásgeirsson wrote: > It's six of on, half a dozen of the other. If you continue the search here while > inside DllMain, the loader will catch the exception. > If this is at DLL_ATTACH time, the process will fail to run. Done -- I'll grab the dump and let the loader take it from there then. https://codereview.chromium.org/154653002/diff/1/chrome_elf/breakpad.cc#newco... chrome_elf/breakpad.cc:179: return BreakpadWin::OnDumpRequested(exinfo); On 2014/02/05 13:58:33, Sigurður Ásgeirsson wrote: > As we discussed, you may want to filter out debug break and single step > exceptions here - have you tested this under debugger? I tested under windbg and it seemed fine -- no harm in adding filters though. https://codereview.chromium.org/154653002/diff/1/chrome_elf/chrome_elf_main.cc File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/154653002/diff/1/chrome_elf/chrome_elf_main.c... chrome_elf/chrome_elf_main.cc:19: // TODO (caitkp): Do we need this? If so, how do we make it play nice with On 2014/02/05 13:58:33, Sigurður Ásgeirsson wrote: > A single function cannot have objects that require unwinding and __try/__except > handlers. If you need this, you need to move the __try/__except into another > function - a file static or anonymous namespace function is fine. Not needed anymore as our breakpad client no longer uses LazyInstance https://codereview.chromium.org/154653002/diff/1/chrome_elf/chrome_elf_main.c... chrome_elf/chrome_elf_main.cc:33: // TODO(caitkp): We should really de-initialize our breakpad client here, to On 2014/02/05 13:58:33, Sigurður Ásgeirsson wrote: > I don't know that this is true. > As-is, you're also getting breakpad coverage for the initialization of all of > chrome.exe's dependent DLLs. Did some very unscientific research here, and it seems that as long as everyone is using the same pipe, chrome's breakpad client will just replace ours when it starts. In fact, if not shut down properly on the previous run, Chrome currently starts it's breakpad client twice (once on initial launch, once on relaunch), so I think Siggi is correct that no clean-up is needed here. Unfortunately, we don't get the coverage of other dependent DLLs, unless we wrap them in try/excepts as we have done for elf.
https://codereview.chromium.org/154653002/diff/70001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/70001/chrome_elf/breakpad.cc#n... chrome_elf/breakpad.cc:73: // Get the alternate dump directory. We use the temp path. are crashes uploaded from the temp path as well? https://codereview.chromium.org/154653002/diff/70001/chrome_elf/breakpad.cc#n... chrome_elf/breakpad.cc:76: wchar_t temp_directory[MAX_PATH + 1] = { 0 }; = {} https://codereview.chromium.org/154653002/diff/70001/chrome_elf/breakpad.h File chrome_elf/breakpad.h (right): https://codereview.chromium.org/154653002/diff/70001/chrome_elf/breakpad.h#ne... chrome_elf/breakpad.h:23: int GenerateCrashDump(EXCEPTION_POINTERS* exinfo); comments on this and the global var below? https://codereview.chromium.org/154653002/diff/70001/chrome_elf/chrome_elf_ma... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/154653002/diff/70001/chrome_elf/chrome_elf_ma... chrome_elf/chrome_elf_main.cc:22: *((unsigned int*)0) = 0xDEAD; Don't think we want to leave this here? https://codereview.chromium.org/154653002/diff/70001/chrome_elf/chrome_elf_ma... chrome_elf/chrome_elf_main.cc:32: // ensure that we don't block Chrome's when it tries to connect in win_main. Did you mention that this might be ok as is?
PTAL. This addresses the comments in the previous patch, and adds breakpad support in blacklist intercept code. https://codereview.chromium.org/154653002/diff/70001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/70001/chrome_elf/breakpad.cc#n... chrome_elf/breakpad.cc:73: // Get the alternate dump directory. We use the temp path. On 2014/02/07 20:03:08, robertshield wrote: > are crashes uploaded from the temp path as well? I am not sure...it seems that the temp path is where the crash dumps are written to initially, then they somehow end up in the AppData\Local\Google\Chrome\User Data\Crash Reports\ dir (or uploaded). https://codereview.chromium.org/154653002/diff/70001/chrome_elf/breakpad.cc#n... chrome_elf/breakpad.cc:76: wchar_t temp_directory[MAX_PATH + 1] = { 0 }; On 2014/02/07 20:03:08, robertshield wrote: > = {} Done. https://codereview.chromium.org/154653002/diff/70001/chrome_elf/breakpad.h File chrome_elf/breakpad.h (right): https://codereview.chromium.org/154653002/diff/70001/chrome_elf/breakpad.h#ne... chrome_elf/breakpad.h:23: int GenerateCrashDump(EXCEPTION_POINTERS* exinfo); On 2014/02/07 20:03:08, robertshield wrote: > comments on this and the global var below? Done. https://codereview.chromium.org/154653002/diff/70001/chrome_elf/chrome_elf_ma... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/154653002/diff/70001/chrome_elf/chrome_elf_ma... chrome_elf/chrome_elf_main.cc:22: *((unsigned int*)0) = 0xDEAD; On 2014/02/07 20:03:08, robertshield wrote: >I suppose it might be a tad problematic... https://codereview.chromium.org/154653002/diff/70001/chrome_elf/chrome_elf_ma... chrome_elf/chrome_elf_main.cc:32: // ensure that we don't block Chrome's when it tries to connect in win_main. On 2014/02/07 20:03:08, robertshield wrote: > Did you mention that this might be ok as is? Done.
https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist.gypi File chrome_elf/blacklist.gypi (right): https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist.gy... chrome_elf/blacklist.gypi:26: # TODO(caitkp): Why do we need this here? possibly for the test target (blacklist_test_main_dll) that also uses blacklist.lib? https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist.cc (right): https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:303: nit: remove blank line https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist_interceptions.cc (right): https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist_interceptions.cc:212: nit: extra blank line? https://codereview.chromium.org/154653002/diff/140001/chrome_elf/breakpad.h File chrome_elf/breakpad.h (right): https://codereview.chromium.org/154653002/diff/140001/chrome_elf/breakpad.h#n... chrome_elf/breakpad.h:13: // that the user has agreed to crash dump reporting. How are we ensuring that the user has opted in to crash reporting?
https://codereview.chromium.org/154653002/diff/140001/chrome_elf/breakpad.h File chrome_elf/breakpad.h (right): https://codereview.chromium.org/154653002/diff/140001/chrome_elf/breakpad.h#n... chrome_elf/breakpad.h:13: // that the user has agreed to crash dump reporting. On 2014/02/10 18:43:14, robertshield wrote: > How are we ensuring that the user has opted in to crash reporting? Hrm..we currently don't. The remoting code I based this off of seems to check a RegKey before setting up crash reporting -- should we be doing something similar?
+Greg to confirm RegKey-checking logic is correct (chrome_elf_util.cc is where said logic lives). Thanks!
Looks awesome! A few comments, also there may have been some comments left unaddressed from my last round (couldn't see the responses to all of them). https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:8: #include "base/win/registry.h" To double check: this doesn't add any new dependencies to elf right? https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:29: base::win::RegKey key; nit: declare |key| next to where it is used. https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:34: .append(guid); This would be one less copy: base::string16 full_key_path(key_path); key_path.append(1, L'\\'); key_path.append(guid); https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:37: full_key_path.c_str(), KEY_QUERY_VALUE); could shorten the last few lines to: return key.Open(system_install? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER, full_key_path.c_str(), KEY_QUERY_VALUE) == ERROR_SUCCESS && key.ReadValueDW(value_to_read, value_out) == ERROR_SUCCESS; without significant loss in readability https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:54: bool found = wcsncmp(exe_path, L"C:\\Program Files", 15) == 0; We can't use shell apis to get the thing the system thinks is the program files path here can we? If not, perhaps use ::GetEnvironmentVariable("PROGRAMFILES") or ::GetEnvironmentVariable("PROGRAMFILES(X86)")? https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:71: bool is_canary = IsCanary(exe_path); is_canary is only used once, could drop the temp var and make the call below https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.h (right): https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.h:12: bool IsCanary(LPWSTR exe_path); use "const wchar_t*" here and below instead.
PTAL -- I've clean up the registry checking logic a bit, reinvented a few wheels, and switched to Greg's suggested approach for checking multi-installs. Thanks! https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist.gypi File chrome_elf/blacklist.gypi (right): https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist.gy... chrome_elf/blacklist.gypi:26: # TODO(caitkp): Why do we need this here? On 2014/02/10 18:43:14, robertshield wrote: > possibly for the test target (blacklist_test_main_dll) that also uses > blacklist.lib? Fixed by fwd declaring the ExceptionHandler. https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist.cc (right): https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist.cc:303: On 2014/02/10 18:43:14, robertshield wrote: > nit: remove blank line Done. https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist_interceptions.cc (right): https://codereview.chromium.org/154653002/diff/140001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist_interceptions.cc:212: On 2014/02/10 18:43:14, robertshield wrote: > nit: extra blank line? Done. https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:8: #include "base/win/registry.h" On 2014/02/11 22:26:58, robertshield wrote: > To double check: this doesn't add any new dependencies to elf right? It does :( I've removed it, and reinvented a few wheels instead. https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:29: base::win::RegKey key; On 2014/02/11 22:26:58, robertshield wrote: > nit: declare |key| next to where it is used. Done. https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:34: .append(guid); On 2014/02/11 22:26:58, robertshield wrote: > This would be one less copy: > > base::string16 full_key_path(key_path); > key_path.append(1, L'\\'); > key_path.append(guid); Done. https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:54: bool found = wcsncmp(exe_path, L"C:\\Program Files", 15) == 0; On 2014/02/11 22:26:58, robertshield wrote: > We can't use shell apis to get the thing the system thinks is the program files > path here can we? > > If not, perhaps use ::GetEnvironmentVariable("PROGRAMFILES") or > ::GetEnvironmentVariable("PROGRAMFILES(X86)")? Unfortunately this check will be one of the first things that happens in chrome_elf's DLLMain(), so Shell apis can't be used yet.. https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:71: bool is_canary = IsCanary(exe_path); On 2014/02/11 22:26:58, robertshield wrote: > is_canary is only used once, could drop the temp var and make the call below Done. https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.h (right): https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.h:12: bool IsCanary(LPWSTR exe_path); On 2014/02/11 22:26:58, robertshield wrote: > use "const wchar_t*" here and below instead. Done.
https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/240001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:54: bool found = wcsncmp(exe_path, L"C:\\Program Files", 15) == 0; On 2014/02/12 19:15:41, Cait Phillips wrote: > On 2014/02/11 22:26:58, robertshield wrote: > > We can't use shell apis to get the thing the system thinks is the program > files > > path here can we? > > > > If not, perhaps use ::GetEnvironmentVariable("PROGRAMFILES") or > > ::GetEnvironmentVariable("PROGRAMFILES(X86)")? > Unfortunately this check will be one of the first things that happens in > chrome_elf's DLLMain(), so Shell apis can't be used yet.. GetEnvironmentVariable is a kernel32 function. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/320001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:6: // handler. This implementation is based on Chrome crash reporting code. nit: Chrome's https://codereview.chromium.org/154653002/diff/320001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:17: google_breakpad::ExceptionHandler* g_elf_breakpad; = NULL; https://codereview.chromium.org/154653002/diff/320001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:38: // L"\\\\.\\pipe\\GoogleCrashServices\\S-1-5-18-x64"; can this comment and the one below with the SID be removed? https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:25: LONG ReadValue(HKEY key, const wchar_t* name, void* data, DWORD* data_size, This function really just directly wraps ::RegQueryValueEx. Why not use that directly inline instead and do away with this function? https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:28: reinterpret_cast<LPBYTE>(data), data_size); LPBYTE -> BYTE* https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:33: const wchar_t* guid, const wchar_t* value_to_read, nit: line up the indent https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:35: HKEY h_key; = NULL; https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:48: wchar_t raw_value[kMaxStringLength]; = {} https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:53: value_out = raw_value; Can't return a buffer allocated on the stack in an out pointer like this, it will have gone away when the caller tries to use it. I think you want to pass a base::string16* as the out parameter, it will make things easier. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:63: HKEY h_key; = NULL; https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:79: DWORD buffer_size = sizeof(DWORD); unused? https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:89: wchar_t* found = wcsstr(exe_path, L"Google\\Chrome SxS"); return wcsstr(exe_path, L"Google\\Chrome SxS") != NULL; https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:94: bool found = wcsncmp(exe_path, L"C:\\Program Files", 15) == 0; this can be shortened too, though I still think we should check the env var using the kernel32 API GetEnvironmentVariable https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:99: wchar_t* args = L""; this is setting args to a static string, which isn't what we want. Use: base::string16 args here instead. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:106: return !!found; return wcsstr(args, L"--multi-install") != NULL; https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:128: nit: drop this blank line https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:132: return out_value == 1; indent is off here.
Still todo: sort out what pipe we should be using. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/320001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:6: // handler. This implementation is based on Chrome crash reporting code. On 2014/02/12 21:12:37, robertshield wrote: > nit: Chrome's Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:17: google_breakpad::ExceptionHandler* g_elf_breakpad; On 2014/02/12 21:12:37, robertshield wrote: > = NULL; Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:25: LONG ReadValue(HKEY key, const wchar_t* name, void* data, DWORD* data_size, On 2014/02/12 21:12:37, robertshield wrote: > This function really just directly wraps ::RegQueryValueEx. Why not use that > directly inline instead and do away with this function? Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:28: reinterpret_cast<LPBYTE>(data), data_size); On 2014/02/12 21:12:37, robertshield wrote: > LPBYTE -> BYTE* Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:33: const wchar_t* guid, const wchar_t* value_to_read, On 2014/02/12 21:12:37, robertshield wrote: > nit: line up the indent Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:35: HKEY h_key; On 2014/02/12 21:12:37, robertshield wrote: > = NULL; Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:48: wchar_t raw_value[kMaxStringLength]; On 2014/02/12 21:12:37, robertshield wrote: > = {} Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:53: value_out = raw_value; On 2014/02/12 21:12:37, robertshield wrote: > Can't return a buffer allocated on the stack in an out pointer like this, it > will have gone away when the caller tries to use it. I think you want to pass a > base::string16* as the out parameter, it will make things easier. Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:63: HKEY h_key; On 2014/02/12 21:12:37, robertshield wrote: > = NULL; Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:79: DWORD buffer_size = sizeof(DWORD); On 2014/02/12 21:12:37, robertshield wrote: > unused? Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:89: wchar_t* found = wcsstr(exe_path, L"Google\\Chrome SxS"); On 2014/02/12 21:12:37, robertshield wrote: > return wcsstr(exe_path, L"Google\\Chrome SxS") != NULL; Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:94: bool found = wcsncmp(exe_path, L"C:\\Program Files", 15) == 0; On 2014/02/12 21:12:37, robertshield wrote: > this can be shortened too, though I still think we should check the env var > using the kernel32 API GetEnvironmentVariable Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:99: wchar_t* args = L""; On 2014/02/12 21:12:37, robertshield wrote: > this is setting args to a static string, which isn't what we want. Use: > > base::string16 args here instead. Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:106: return !!found; On 2014/02/12 21:12:37, robertshield wrote: > return wcsstr(args, L"--multi-install") != NULL; Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:128: On 2014/02/12 21:12:37, robertshield wrote: > nit: drop this blank line Done. https://codereview.chromium.org/154653002/diff/320001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:132: return out_value == 1; On 2014/02/12 21:12:37, robertshield wrote: > indent is off here. Done.
some comments. a bunch of the files have the old chunk mismatch error, so i didn't look at those. overall looks pretty good. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. (c) https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:28: // TODO(caitkp): figure out what these should be. Current pipe works with local 32-bit system: \\.\pipe\GoogleCrashServices\S-1-5-18 32-bit user: \\.\pipe\GoogleCrashServices\<user SID> 64-bit system: \\.\pipe\GoogleCrashServices\S-1-5-18-x64 64-bit user: \\.\pipe\GoogleCrashServices\<user SID>-x64 https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:44: nit: remove extra newline https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:90: MiniDumpWithProcessThreadData | // Get PEB and TEB. nit: indent two more spaces https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.h File chrome_elf/breakpad.h (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.h#n... chrome_elf/breakpad.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. no "(c)" in new copyright headers https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.h#n... chrome_elf/breakpad.h:11: class ExceptionHandler; nit: don't indent here https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.h#n... chrome_elf/breakpad.h:14: // Initializes collection and upload of crash reports. The caller has to ensure it looks like the implementation checks for consent. please update comment. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.h#n... chrome_elf/breakpad.h:17: // Crash reporting has to be initialized as early as possible (e.g. the first nit: "e.g." -> "e.g.," https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:17: nit: remove extra newline https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:29: *value_out = base::string16(); value_out->clear(); https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:35: if (::RegOpenKeyEx(system_install? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER, nit: space before ? https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:36: full_key_path.c_str(), 0, KEY_QUERY_VALUE, &h_key) != ERROR_SUCCESS) { use the KEY_WOW64_32KEY flag here and on line 64 (chrome doesn't yet, but should). https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:44: LONG result = ::RegQueryValueEx(h_key, value_to_read, 0, &type, this isn't guaranteed to return a null-terminated string. see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/m... for an implementation that enforces null-termination. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:50: return ERROR_SUCCESS == result; chromium style puts the constant after the variable being tested: return result == ERROR_SUCCESS; https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:56: HKEY h_key = NULL;; nit: remove extra ; https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:61: full_key_path.append(guid.c_str()); remove .c_str() https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:63: if (::RegOpenKeyEx(system_install? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER, nit: space before ? https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:75: return ERROR_SUCCESS == result; see above https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:81: return wcsstr(exe_path, L"Google\\Chrome SxS") != NULL; i think chrome looks for "Chrome SxS\\Application" rather than "Google\\Chrome SxS", no? https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:86: DWORD ret = ::GetEnvironmentVariable(L"PROGRAMFILES", program_dir, MAX_PATH); MAX_PATH -> arraysize(program_dir) here and below https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:87: if (ret!=0 && ret<=MAX_PATH && wcsncmp(exe_path, program_dir, ret) == 0) whitespace around operators simplify zero tests ret doesn't include the terminating null if (ret && ret < MAX_PATH && !wcsncmp(exe_path, program_dir, ret)) https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:99: if (ERROR_SUCCESS != ReadKeyValueString(IsSystemInstall(exe_path), ReadKeyValueString returns bool: if (!ReadKeyValueString(...)) https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:104: return wcsstr(args.c_str(), L"--multi-install") != NULL; return args.find(L"--multi-install") != base::string16::npos; https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:112: if (!GetModuleFileNameW(NULL, exe_path, MAX_PATH)) MAX_PATH -> arraysize(exe_path) https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:121: app_guid = IsMultiInstall(exe_path)? kAppGuidGoogleBinaries : nit: space before ? https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.h (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.h:8: #include <windows.h> unneeded https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:14: } test the other helper functions, too? base/test/test_reg_util_win.h has the RegistryOverrideManager, which is useful for testing functions that diddle in the registry.
getting there... https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/02/13 03:58:26, grt wrote: > (c) Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:28: // TODO(caitkp): figure out what these should be. Current pipe works with local On 2014/02/13 03:58:26, grt wrote: > 32-bit system: \\.\pipe\GoogleCrashServices\S-1-5-18 > 32-bit user: \\.\pipe\GoogleCrashServices\<user SID> > 64-bit system: \\.\pipe\GoogleCrashServices\S-1-5-18-x64 > 64-bit user: \\.\pipe\GoogleCrashServices\<user SID>-x64 Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:44: On 2014/02/13 03:58:26, grt wrote: > nit: remove extra newline Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:90: MiniDumpWithProcessThreadData | // Get PEB and TEB. On 2014/02/13 03:58:26, grt wrote: > nit: indent two more spaces Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.h File chrome_elf/breakpad.h (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.h#n... chrome_elf/breakpad.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/02/13 03:58:26, grt wrote: > no "(c)" in new copyright headers Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.h#n... chrome_elf/breakpad.h:11: class ExceptionHandler; On 2014/02/13 03:58:26, grt wrote: > nit: don't indent here Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.h#n... chrome_elf/breakpad.h:14: // Initializes collection and upload of crash reports. The caller has to ensure On 2014/02/13 03:58:26, grt wrote: > it looks like the implementation checks for consent. please update comment. Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/breakpad.h#n... chrome_elf/breakpad.h:17: // Crash reporting has to be initialized as early as possible (e.g. the first On 2014/02/13 03:58:26, grt wrote: > nit: "e.g." -> "e.g.," Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:17: On 2014/02/13 03:58:26, grt wrote: > nit: remove extra newline Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:29: *value_out = base::string16(); On 2014/02/13 03:58:26, grt wrote: > value_out->clear(); Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:35: if (::RegOpenKeyEx(system_install? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER, On 2014/02/13 03:58:26, grt wrote: > nit: space before ? Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:36: full_key_path.c_str(), 0, KEY_QUERY_VALUE, &h_key) != ERROR_SUCCESS) { On 2014/02/13 03:58:26, grt wrote: > use the KEY_WOW64_32KEY flag here and on line 64 (chrome doesn't yet, but > should). Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:44: LONG result = ::RegQueryValueEx(h_key, value_to_read, 0, &type, On 2014/02/13 03:58:26, grt wrote: > this isn't guaranteed to return a null-terminated string. see > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/installer/m... > for an implementation that enforces null-termination. Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:50: return ERROR_SUCCESS == result; On 2014/02/13 03:58:26, grt wrote: > chromium style puts the constant after the variable being tested: > return result == ERROR_SUCCESS; Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:56: HKEY h_key = NULL;; On 2014/02/13 03:58:26, grt wrote: > nit: remove extra ; Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:61: full_key_path.append(guid.c_str()); On 2014/02/13 03:58:26, grt wrote: > remove .c_str() Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:63: if (::RegOpenKeyEx(system_install? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER, On 2014/02/13 03:58:26, grt wrote: > nit: space before ? Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:75: return ERROR_SUCCESS == result; On 2014/02/13 03:58:26, grt wrote: > see above Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:81: return wcsstr(exe_path, L"Google\\Chrome SxS") != NULL; On 2014/02/13 03:58:26, grt wrote: > i think chrome looks for "Chrome SxS\\Application" rather than "Google\\Chrome > SxS", no? Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:86: DWORD ret = ::GetEnvironmentVariable(L"PROGRAMFILES", program_dir, MAX_PATH); On 2014/02/13 03:58:26, grt wrote: > MAX_PATH -> arraysize(program_dir) here and below Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:87: if (ret!=0 && ret<=MAX_PATH && wcsncmp(exe_path, program_dir, ret) == 0) On 2014/02/13 03:58:26, grt wrote: > whitespace around operators > simplify zero tests > ret doesn't include the terminating null > if (ret && ret < MAX_PATH && !wcsncmp(exe_path, program_dir, ret)) Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:99: if (ERROR_SUCCESS != ReadKeyValueString(IsSystemInstall(exe_path), On 2014/02/13 03:58:26, grt wrote: > ReadKeyValueString returns bool: > if (!ReadKeyValueString(...)) Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:104: return wcsstr(args.c_str(), L"--multi-install") != NULL; On 2014/02/13 03:58:26, grt wrote: > return args.find(L"--multi-install") != base::string16::npos; Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:112: if (!GetModuleFileNameW(NULL, exe_path, MAX_PATH)) On 2014/02/13 03:58:26, grt wrote: > MAX_PATH -> arraysize(exe_path) Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:121: app_guid = IsMultiInstall(exe_path)? kAppGuidGoogleBinaries : On 2014/02/13 03:58:26, grt wrote: > nit: space before ? Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.h (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.h:8: #include <windows.h> On 2014/02/13 03:58:26, grt wrote: > unneeded Done. https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:14: } On 2014/02/13 03:58:26, grt wrote: > test the other helper functions, too? base/test/test_reg_util_win.h has the > RegistryOverrideManager, which is useful for testing functions that diddle in > the registry. Done
https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:50: return ERROR_SUCCESS == result; On 2014/02/14 01:17:02, Cait Phillips wrote: > On 2014/02/13 03:58:26, grt wrote: > > chromium style puts the constant after the variable being tested: > > return result == ERROR_SUCCESS; > > Done. Really? ;-) https://codereview.chromium.org/154653002/diff/380001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:75: return ERROR_SUCCESS == result; On 2014/02/14 01:17:02, Cait Phillips wrote: > On 2014/02/13 03:58:26, grt wrote: > > see above > > Done. More doing? https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:10: #include <string> not needed? https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:23: const wchar_t kBreakpadVersionDefault[] = L"0.1.0.0"; unused https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:89: // Build the pipe name. It can be either: either -> one of https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:94: base::string16 user_sid = kSystemPrincipalSid; shouldn't this connect to the crash_service on the bots (\\.\pipe\ChromeCrashServices) the same way chrome does? also, i hate to rain on the parade, but what about policy control over crash reporting: * https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/chrome_... * https://code.google.com/p/chromium/codesearch#chromium/src/components/breakpa... https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:96: if (!IsSystemInstall(exe_path)) { if (!A && !B) return; https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:8: #include <windows.h> usually windows.h is included first. is there a reason it can't be here? https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:39: full_key_path.c_str(), 0, KEY_QUERY_VALUE | KEY_WOW64_32KEY, nit: align with open paren above https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:52: if (type != REG_SZ) { since an odd number of bytes doesn't make sense, how about (type != REG_SZ || (size & 1) != 0) https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:82: full_key_path.c_str(), 0, KEY_QUERY_VALUE | KEY_WOW64_32KEY, align with open paren above https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:90: reinterpret_cast<BYTE*>(value_out), &size); align with open paren above https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:121: kAppGuidGoogleChrome, kUninstallArgumentsField, &args)) { align with open paren above https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:142: if (system_install && ReadKeyValueDW(system_install, if (system_install && ReadKeyValueDW(system_install, kRegPathClientStateMedium, app_guid, kRegValueUsageStats, &out_value)) { https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:148: return ReadKeyValueDW(system_install, kRegPathClientState, return ReadKeyValueDW(system_install, kRegPathClientState, app_guid, kRegValueUsageStats, &out_value) && out_value == 1; https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:156: if (!::OpenProcessToken(::GetCurrentProcess(), TOKEN_QUERY, &token)) can you use base::win::ScopedHandle for the token? if not, you need to be sure to CloseHandle(token) before each return following line 158. if you can't use ScopedHandle, then you could possibly make lines 163 onward something like: base::string16 user_sid; if (::GetTokenInformation(...) && user->User.Sid && ::ConvertSidToStringSid(...)) { user_sid = sid_string; ::LocalFree(...); } CloseHandle(token); return user_sid; } it's probably a fine idea to make this return the string and leave it up to the caller to check whether or not it's empty for success/failure. this is more in line with chromium's approach to returning strings by value. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:160: BYTE user_bytes[sizeof(TOKEN_USER) + SECURITY_MAX_SID_SIZE] = {}; BYTE user_bytes[size] = {}; https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:161: TOKEN_USER* user = reinterpret_cast<TOKEN_USER*>(&user_bytes); &user_bytes will be a pointer to the array rather than a pointer to its first element. use either user_bytes (which will be automatically converted to a pointer to the first element) or &user_bytes[0] (which explicitly is a pointer to the first element). chromium style would be to use just |user_bytes|. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.h (right): https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.h:20: bool AreUsageStatsEnabled(const wchar_t* exe_path); docuemnt https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.h:22: bool GetUserSidString(base::string16* user_sid); document (and make it return the base::string16) https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:38: virtual void SetUp() OVERRIDE { SetUp should be protected, too https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:40: L"chrome_elf_test_local"); indentation https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:55: key.Create(is_system? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER, space after ? https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:56: BuildKey(kRegPathClientState, kAppGuidGoogleChrome).c_str(), align with the open paren of Create( https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:57: KEY_ALL_ACCESS); only request the access you need: KEY_SET_VALUE https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:64: key.Close(); RegKey's dtor does this for you https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:70: key.Create(is_system? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER, use RegKey's ctor rather than calling Create here and above. this whole function could become a 1-liner across a few lines: ASSERT_EQ(ERROR_SUCCESS, base::win::RegKey(...).WriteValue(...)); https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:71: BuildKey(path, guid).c_str(), align with the open paren above https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:72: KEY_ALL_ACCESS); KEY_SET_VALUE https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:74: key.Close(); remove https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:116: TEST_F(ChromeElfUtilTest, UsageStatsTest_Canary) { i think this is a good case for a parameterized test (https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...). for example: // Parameterized test with paramters: // 1: product: "canary" or "google" // 2: install level: "user" or "system" // 3: install mode: "single" or "multi" class ChromeElfUtilTest : public testing::TestWithParam<tuple<const char*, const char*, const char*> > { protected: virtual void SetUp() OVERRIDE { const char* app; const char* level; const char* mode; std::tie(app, level, mode) = GetParam(); bool is_canary = (std::string(app) == "canary"); system_level_ = (std::string(level) != "user"); multi_install_ = (std::string(mode) != "single"); if (is_canary) { ASSERT_FALSE(system_level_); ASSERT_FALSE(multi_install_); app_guid_ = kAppGuidCanary; chrome_path_ = kCanaryExePath; } else { app_guid_ = kAppGuidGoogleChrome; chrome_path_ = (system_level_ ? kChromeSystemExePath : kChromeUserExePath); } if (multi_install_) SetMultiInstallStateInRegistry(...); } void SetUsageState(DWORD value, bool state_medium) { // set |value| in ClientState or ClientStateMedium according to state_medium } const wchar_t* app_guid_; const wchar_t* chrome_path_; bool system_level_; bool multi_install_; }; TEST_P(ChromeElfUtilTest, UsageStatsAbsent) { EXPECT_FALSE(AreUsageStatsEnabled(chrome_path_)); } TEST_P(ChromeElfUtilTest, UsageStatsZero) { SetUsageStat(0, false); EXPECT_FALSE(AreUsageStatsEnabled(chrome_path_)); } TEST_P(ChromeElfUtilTest, UsageStatsOne) { SetUsageStat(1, false); EXPECT_TRUE(AreUsageStatsEnabled(chrome_path_)); if (is_canary) { EXPECT_FALSE(AreUsageStatsEnabled(kChromeUserExePath)); EXPECT_FALSE(AreUsageStatsEnabled(kChromeSystemExePath)); } else if (system_level_) { EXPECT_FALSE(AreUsageStatsEnabled(kCanaryExePath)); EXPECT_FALSE(AreUsageStatsEnabled(kChromeUserExePath)); } else { EXPECT_FALSE(AreUsageStatsEnabled(kCanaryExePath)); EXPECT_FALSE(AreUsageStatsEnabled(kChromeSystemExePath)); } } TEST_P(ChromeElfUtilTest, UsageStatsZeroInStateMedium) { if (!system_level_) return; SetUsageStat(0, true); EXPECT_FALSE(AreUsageStatsEnabled(chrome_path_)); } TEST_P(ChromeElfUtilTest, UsageStatsOneInStateMedium) { if (!system_level_) return; SetUsageStat(1, true); EXPECT_TRUE(AreUsageStatsEnabled(chrome_path_)); if (is_canary) { EXPECT_FALSE(AreUsageStatsEnabled(kChromeUserExePath)); EXPECT_FALSE(AreUsageStatsEnabled(kChromeSystemExePath)); } else if (system_level_) { EXPECT_FALSE(AreUsageStatsEnabled(kCanaryExePath)); EXPECT_FALSE(AreUsageStatsEnabled(kChromeUserExePath)); } else { EXPECT_FALSE(AreUsageStatsEnabled(kCanaryExePath)); EXPECT_FALSE(AreUsageStatsEnabled(kChromeSystemExePath)); } } INSTANTIATE_TEST_CASE_P(Canary, ChromeElfUtilTest, Combine(Values("canary"), Values("user"), Values("single"))); INSTANTIATE_TEST_CASE_P(GoogleChrome, ChromeElfUtilTest, Combine(Values("google"), Values("user", "system"), Values("single", "multi")));
https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:10: #include <string> On 2014/02/14 16:37:33, grt wrote: > not needed? Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:23: const wchar_t kBreakpadVersionDefault[] = L"0.1.0.0"; On 2014/02/14 16:37:33, grt wrote: > unused Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:89: // Build the pipe name. It can be either: On 2014/02/14 16:37:33, grt wrote: > either -> one of Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:94: base::string16 user_sid = kSystemPrincipalSid; On 2014/02/14 16:37:33, grt wrote: > shouldn't this connect to the crash_service on the bots > (\\.\pipe\ChromeCrashServices) the same way chrome does? > > also, i hate to rain on the parade, but what about policy control over crash > reporting: > * > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/app/chrome_... > * > https://code.google.com/p/chromium/codesearch#chromium/src/components/breakpa... Done -- if breakpad is enabled by policy, do we still check the other registry keys? https://codereview.chromium.org/154653002/diff/730001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:96: if (!IsSystemInstall(exe_path)) { On 2014/02/14 16:37:33, grt wrote: > if (!A && !B) > return; Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:8: #include <windows.h> On 2014/02/14 16:37:33, grt wrote: > usually windows.h is included first. is there a reason it can't be here? Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:39: full_key_path.c_str(), 0, KEY_QUERY_VALUE | KEY_WOW64_32KEY, On 2014/02/14 16:37:33, grt wrote: > nit: align with open paren above Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:52: if (type != REG_SZ) { On 2014/02/14 16:37:33, grt wrote: > since an odd number of bytes doesn't make sense, how about (type != REG_SZ || > (size & 1) != 0) Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:82: full_key_path.c_str(), 0, KEY_QUERY_VALUE | KEY_WOW64_32KEY, On 2014/02/14 16:37:33, grt wrote: > align with open paren above Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:90: reinterpret_cast<BYTE*>(value_out), &size); On 2014/02/14 16:37:33, grt wrote: > align with open paren above Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:121: kAppGuidGoogleChrome, kUninstallArgumentsField, &args)) { On 2014/02/14 16:37:33, grt wrote: > align with open paren above Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:142: if (system_install && ReadKeyValueDW(system_install, On 2014/02/14 16:37:33, grt wrote: > if (system_install && > ReadKeyValueDW(system_install, kRegPathClientStateMedium, app_guid, > kRegValueUsageStats, &out_value)) { Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:148: return ReadKeyValueDW(system_install, kRegPathClientState, On 2014/02/14 16:37:33, grt wrote: > return ReadKeyValueDW(system_install, kRegPathClientState, app_guid, > kRegValueUsageStats, &out_value) && out_value == 1; Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:156: if (!::OpenProcessToken(::GetCurrentProcess(), TOKEN_QUERY, &token)) On 2014/02/14 16:37:33, grt wrote: > can you use base::win::ScopedHandle for the token? if not, you need to be sure > to CloseHandle(token) before each return following line 158. if you can't use > ScopedHandle, then you could possibly make lines 163 onward something like: > > base::string16 user_sid; > if (::GetTokenInformation(...) && > user->User.Sid && > ::ConvertSidToStringSid(...)) { > user_sid = sid_string; > ::LocalFree(...); > } > CloseHandle(token); > return user_sid; > } > > it's probably a fine idea to make this return the string and leave it up to the > caller to check whether or not it's empty for success/failure. this is more in > line with chromium's approach to returning strings by value. Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:160: BYTE user_bytes[sizeof(TOKEN_USER) + SECURITY_MAX_SID_SIZE] = {}; On 2014/02/14 16:37:33, grt wrote: > BYTE user_bytes[size] = {}; Compiler gets upset: d:\chrome_git2\src\chrome_elf\breakpad.cc(61) : error C2057: expected constant e xpression d:\chrome_git2\src\chrome_elf\breakpad.cc(61) : error C2466: cannot allocate an array of constant size 0 https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:161: TOKEN_USER* user = reinterpret_cast<TOKEN_USER*>(&user_bytes); On 2014/02/14 16:37:33, grt wrote: > &user_bytes will be a pointer to the array rather than a pointer to its first > element. use either user_bytes (which will be automatically converted to a > pointer to the first element) or &user_bytes[0] (which explicitly is a pointer > to the first element). chromium style would be to use just |user_bytes|. Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.h (right): https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.h:20: bool AreUsageStatsEnabled(const wchar_t* exe_path); On 2014/02/14 16:37:33, grt wrote: > docuemnt Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.h:22: bool GetUserSidString(base::string16* user_sid); On 2014/02/14 16:37:33, grt wrote: > document (and make it return the base::string16) Moved into breakpad.cc anonymous namespace. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:38: virtual void SetUp() OVERRIDE { On 2014/02/14 16:37:33, grt wrote: > SetUp should be protected, too Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:40: L"chrome_elf_test_local"); On 2014/02/14 16:37:33, grt wrote: > indentation Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:55: key.Create(is_system? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER, On 2014/02/14 16:37:33, grt wrote: > space after ? Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:56: BuildKey(kRegPathClientState, kAppGuidGoogleChrome).c_str(), On 2014/02/14 16:37:33, grt wrote: > align with the open paren of Create( Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:57: KEY_ALL_ACCESS); On 2014/02/14 16:37:33, grt wrote: > only request the access you need: KEY_SET_VALUE Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:64: key.Close(); On 2014/02/14 16:37:33, grt wrote: > RegKey's dtor does this for you Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:70: key.Create(is_system? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER, On 2014/02/14 16:37:33, grt wrote: > use RegKey's ctor rather than calling Create here and above. this whole function > could become a 1-liner across a few lines: > ASSERT_EQ(ERROR_SUCCESS, > base::win::RegKey(...).WriteValue(...)); Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:71: BuildKey(path, guid).c_str(), On 2014/02/14 16:37:33, grt wrote: > align with the open paren above Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:72: KEY_ALL_ACCESS); On 2014/02/14 16:37:33, grt wrote: > KEY_SET_VALUE Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:74: key.Close(); On 2014/02/14 16:37:33, grt wrote: > remove Done. https://codereview.chromium.org/154653002/diff/730001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:116: TEST_F(ChromeElfUtilTest, UsageStatsTest_Canary) { On 2014/02/14 16:37:33, grt wrote: > i think this is a good case for a parameterized test > (https://code.google.com/p/googletest/wiki/AdvancedGuide#Value_Parameterized_T...). > for example: > > // Parameterized test with paramters: > // 1: product: "canary" or "google" > // 2: install level: "user" or "system" > // 3: install mode: "single" or "multi" > class ChromeElfUtilTest : > public testing::TestWithParam<tuple<const char*, > const char*, > const char*> > { > protected: > virtual void SetUp() OVERRIDE { > const char* app; > const char* level; > const char* mode; > std::tie(app, level, mode) = GetParam(); > bool is_canary = (std::string(app) == "canary"); > system_level_ = (std::string(level) != "user"); > multi_install_ = (std::string(mode) != "single"); > if (is_canary) { > ASSERT_FALSE(system_level_); > ASSERT_FALSE(multi_install_); > app_guid_ = kAppGuidCanary; > chrome_path_ = kCanaryExePath; > } else { > app_guid_ = kAppGuidGoogleChrome; > chrome_path_ = (system_level_ ? kChromeSystemExePath : > kChromeUserExePath); > } > if (multi_install_) > SetMultiInstallStateInRegistry(...); > } > > void SetUsageState(DWORD value, bool state_medium) { > // set |value| in ClientState or ClientStateMedium according to state_medium > } > > const wchar_t* app_guid_; > const wchar_t* chrome_path_; > bool system_level_; > bool multi_install_; > }; > > TEST_P(ChromeElfUtilTest, UsageStatsAbsent) { > EXPECT_FALSE(AreUsageStatsEnabled(chrome_path_)); > } > > TEST_P(ChromeElfUtilTest, UsageStatsZero) { > SetUsageStat(0, false); > EXPECT_FALSE(AreUsageStatsEnabled(chrome_path_)); > } > > TEST_P(ChromeElfUtilTest, UsageStatsOne) { > SetUsageStat(1, false); > EXPECT_TRUE(AreUsageStatsEnabled(chrome_path_)); > if (is_canary) { > EXPECT_FALSE(AreUsageStatsEnabled(kChromeUserExePath)); > EXPECT_FALSE(AreUsageStatsEnabled(kChromeSystemExePath)); > } else if (system_level_) { > EXPECT_FALSE(AreUsageStatsEnabled(kCanaryExePath)); > EXPECT_FALSE(AreUsageStatsEnabled(kChromeUserExePath)); > } else { > EXPECT_FALSE(AreUsageStatsEnabled(kCanaryExePath)); > EXPECT_FALSE(AreUsageStatsEnabled(kChromeSystemExePath)); > } > } > > TEST_P(ChromeElfUtilTest, UsageStatsZeroInStateMedium) { > if (!system_level_) > return; > SetUsageStat(0, true); > EXPECT_FALSE(AreUsageStatsEnabled(chrome_path_)); > } > > TEST_P(ChromeElfUtilTest, UsageStatsOneInStateMedium) { > if (!system_level_) > return; > SetUsageStat(1, true); > EXPECT_TRUE(AreUsageStatsEnabled(chrome_path_)); > if (is_canary) { > EXPECT_FALSE(AreUsageStatsEnabled(kChromeUserExePath)); > EXPECT_FALSE(AreUsageStatsEnabled(kChromeSystemExePath)); > } else if (system_level_) { > EXPECT_FALSE(AreUsageStatsEnabled(kCanaryExePath)); > EXPECT_FALSE(AreUsageStatsEnabled(kChromeUserExePath)); > } else { > EXPECT_FALSE(AreUsageStatsEnabled(kCanaryExePath)); > EXPECT_FALSE(AreUsageStatsEnabled(kChromeSystemExePath)); > } > } > > INSTANTIATE_TEST_CASE_P(Canary, ChromeElfUtilTest, > Combine(Values("canary"), > Values("user"), > Values("single"))); > INSTANTIATE_TEST_CASE_P(GoogleChrome, ChromeElfUtilTest, > Combine(Values("google"), > Values("user", "system"), > Values("single", "multi"))); Done -- wow! I had no idea these existed. They make things much less tedious.
looking good https://codereview.chromium.org/154653002/diff/870001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist_interceptions.cc (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist_interceptions.cc:221: GetNtDllExportByName("NtQuerySection")); nit: either 4-space indent here, or move the reinterpret_cast down one line so that all three assignments are formatted identically. i prefer the latter https://codereview.chromium.org/154653002/diff/870001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist_interceptions.cc:230: g_nt_unmap_view_of_section_func; nit: either 4-space indent, or wrap the whole thing in braces so that the continuation is aligned with the open paren https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:66: user->User.Sid && ::ConvertSidToStringSid(user->User.Sid, &sid_string)) { nit: i find this a little easier to read with ::ConvertSidToStringSid on the next line: if (A && B && C) { https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:77: bool has_env_var = ret != 0; if (ret != 0) return true; https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:80: bool has_flag = (command_line && wcsstr(command_line, kNoErrorDialogs)); i'm a little worried that this will match things that the Chrome code won't since it's doing a pure substring search rather than checking for a switch with the given name. please add a comment making that clear, but explaining that it's probably okay since there are no other switches defined that contain the string "noerrdialogs". return command_line && wcsstr(command_line, kNoErrorDialogs); https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:103: if (!AreUsageStatsEnabled(exe_path)) policy trumps usagestats, so only check this if !use_policy down below. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:126: bool enabled; nit: initialize to something. also, consider renaming to enabled_by_policy https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:131: } else { consider: } else if (use_policy ? enabled : AreUsageStatsEnabled(exe_path)) { } else { // Either reporting is disabled by policy or the user has not given consent. return; } https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf.gyp File chrome_elf/chrome_elf.gyp (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf.g... chrome_elf/chrome_elf.gyp:116: 'target_name': 'chrome_elf_breakpad', why is this its own target? https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_m... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_m... chrome_elf/chrome_elf_main.cc:19: InitializeCrashReporting(); is there anything that should be shut down during DETACH? https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:71: *value_out = raw_value; only make this assignment when result == ERROR_SUCCESS. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:95: DWORD size = sizeof(DWORD); sizeof(*value_out) https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:102: return result == ERROR_SUCCESS; result == ERROR_SUCCESS && size == sizeof(*value_out) https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:171: HKEY h_key = NULL; don't prefix this with h_ (and elsewhere in this file) https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:173: BYTE* value_bytes = reinterpret_cast<BYTE*>(value); value -> &value https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:174: DWORD size = sizeof(DWORD); sizeof(value) https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:177: if (::RegOpenKeyEx(HKEY_LOCAL_MACHINE, kRegPathChromePolicy, 0, be sure to close the key regardless as to whether the query succeeds or fails: if (::RegOpenKeyEx(...)...) { if (::RegQueryValueEx(...)...) { *breakpad_enabled = value != 0; } ::RegCloseKey(...);; } also, only consider |value| meaningful if size == sizeof(value) https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:178: KEY_READ | KEY_WOW64_32KEY, &h_key) == ERROR_SUCCESS && KEY_WOW64_32KEY is not called for here. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:178: KEY_READ | KEY_WOW64_32KEY, &h_key) == ERROR_SUCCESS && KEY_READ -> KEY_QUERY_VALUE https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:198: nit: remove extra newlines at end of file https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:84: system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER, use system_install_ member rather than passing it as an argument? https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:114: TEST_P(ChromeElfUtilTest, CanaryTest) { if this test doesn't use anything in the ChromeElfUtilTest fixture, don't bother using the fixture (i.e., use TEST rather than TEST_P). same comment for SystemInstallTest below. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:171: if (is_canary_) { this will never be true since canary is never at system level https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:174: } else if (system_level_) { this will always be true as a result of line 168
PTAL -- thanks for yet another round of comments, Greg :) https://codereview.chromium.org/154653002/diff/870001/chrome_elf/blacklist/bl... File chrome_elf/blacklist/blacklist_interceptions.cc (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist_interceptions.cc:221: GetNtDllExportByName("NtQuerySection")); On 2014/02/17 20:49:39, grt wrote: > nit: either 4-space indent here, or move the reinterpret_cast down one line so > that all three assignments are formatted identically. i prefer the latter Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/blacklist/bl... chrome_elf/blacklist/blacklist_interceptions.cc:230: g_nt_unmap_view_of_section_func; On 2014/02/17 20:49:39, grt wrote: > nit: either 4-space indent, or wrap the whole thing in braces so that the > continuation is aligned with the open paren Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:66: user->User.Sid && ::ConvertSidToStringSid(user->User.Sid, &sid_string)) { On 2014/02/17 20:49:39, grt wrote: > nit: i find this a little easier to read with ::ConvertSidToStringSid on the > next line: > if (A && > B && > C) { Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:77: bool has_env_var = ret != 0; On 2014/02/17 20:49:39, grt wrote: > if (ret != 0) > return true; Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:77: bool has_env_var = ret != 0; On 2014/02/17 20:49:39, grt wrote: > if (ret != 0) > return true; Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:80: bool has_flag = (command_line && wcsstr(command_line, kNoErrorDialogs)); On 2014/02/17 20:49:39, grt wrote: > i'm a little worried that this will match things that the Chrome code won't > since it's doing a pure substring search rather than checking for a switch with > the given name. please add a comment making that clear, but explaining that it's > probably okay since there are no other switches defined that contain the string > "noerrdialogs". > return command_line && wcsstr(command_line, kNoErrorDialogs); Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:103: if (!AreUsageStatsEnabled(exe_path)) On 2014/02/17 20:49:39, grt wrote: > policy trumps usagestats, so only check this if !use_policy down below. Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:126: bool enabled; On 2014/02/17 20:49:39, grt wrote: > nit: initialize to something. also, consider renaming to enabled_by_policy Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:131: } else { On 2014/02/17 20:49:39, grt wrote: > consider: > } else if (use_policy ? enabled : AreUsageStatsEnabled(exe_path)) { > } else { > // Either reporting is disabled by policy or the user has not given consent. > return; > } Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf.gyp File chrome_elf/chrome_elf.gyp (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf.g... chrome_elf/chrome_elf.gyp:116: 'target_name': 'chrome_elf_breakpad', On 2014/02/17 20:49:39, grt wrote: > why is this its own target? This is because it needs to be used by the blacklist target too (in order to catch crashes in the blacklist interception code. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_m... File chrome_elf/chrome_elf_main.cc (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_m... chrome_elf/chrome_elf_main.cc:19: InitializeCrashReporting(); On 2014/02/17 20:49:39, grt wrote: > is there anything that should be shut down during DETACH? Not that I know of. When chrome starts, I believe their client their client will just replace ours. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:71: *value_out = raw_value; On 2014/02/17 20:49:39, grt wrote: > only make this assignment when result == ERROR_SUCCESS. Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:95: DWORD size = sizeof(DWORD); On 2014/02/17 20:49:39, grt wrote: > sizeof(*value_out) Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:102: return result == ERROR_SUCCESS; On 2014/02/17 20:49:39, grt wrote: > result == ERROR_SUCCESS && size == sizeof(*value_out) Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:173: BYTE* value_bytes = reinterpret_cast<BYTE*>(value); On 2014/02/17 20:49:39, grt wrote: > value -> &value Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:174: DWORD size = sizeof(DWORD); On 2014/02/17 20:49:39, grt wrote: > sizeof(value) Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:177: if (::RegOpenKeyEx(HKEY_LOCAL_MACHINE, kRegPathChromePolicy, 0, On 2014/02/17 20:49:39, grt wrote: > be sure to close the key regardless as to whether the query succeeds or fails: > if (::RegOpenKeyEx(...)...) { > if (::RegQueryValueEx(...)...) { > *breakpad_enabled = value != 0; > } > ::RegCloseKey(...);; > } > > also, only consider |value| meaningful if size == sizeof(value) Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:178: KEY_READ | KEY_WOW64_32KEY, &h_key) == ERROR_SUCCESS && On 2014/02/17 20:49:39, grt wrote: > KEY_READ -> KEY_QUERY_VALUE Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:178: KEY_READ | KEY_WOW64_32KEY, &h_key) == ERROR_SUCCESS && On 2014/02/17 20:49:39, grt wrote: > KEY_WOW64_32KEY is not called for here. Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:198: On 2014/02/17 20:49:39, grt wrote: > nit: remove extra newlines at end of file Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:84: system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER, On 2014/02/17 20:49:39, grt wrote: > use system_install_ member rather than passing it as an argument? Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:114: TEST_P(ChromeElfUtilTest, CanaryTest) { On 2014/02/17 20:49:39, grt wrote: > if this test doesn't use anything in the ChromeElfUtilTest fixture, don't bother > using the fixture (i.e., use TEST rather than TEST_P). same comment for > SystemInstallTest below. Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:171: if (is_canary_) { On 2014/02/17 20:49:39, grt wrote: > this will never be true since canary is never at system level Done. https://codereview.chromium.org/154653002/diff/870001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:174: } else if (system_level_) { On 2014/02/17 20:49:39, grt wrote: > this will always be true as a result of line 168 Done.
https://codereview.chromium.org/154653002/diff/980001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/980001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:144: base::string16 pipe_name = kGoogleUpdatePipeName; nix base::string16 so that pipe_name defined on line 126 is used. https://codereview.chromium.org/154653002/diff/980001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/980001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:181: value_bytes, &size) == ERROR_SUCCESS) { nit: indentation https://codereview.chromium.org/154653002/diff/980001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:191: value_bytes, &size) == ERROR_SUCCESS) { nit: indentation https://codereview.chromium.org/154653002/diff/980001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/154653002/diff/980001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:30: L"C:\\Users\\user\\AppData\\Local\\Google\\Chrome SxS\\Application\\chrome.exe"; nit: break this string up so that the line isn't too long: const wchar_t kCanaryExePath[] = L"C:\\Users\\user\\AppData\\Local\\Google\\Chrome SxS\\Application" L"\\chrome.exe"; https://codereview.chromium.org/154653002/diff/980001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:114: TEST(ChromeElfUtilTest, CanaryTest) { please move this above the fixture class so they're not intermingled with the test functions that use the fixture. thanks.
https://codereview.chromium.org/154653002/diff/980001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/980001/chrome_elf/breakpad.cc#... chrome_elf/breakpad.cc:144: base::string16 pipe_name = kGoogleUpdatePipeName; On 2014/02/19 15:55:36, grt wrote: > nix base::string16 so that pipe_name defined on line 126 is used. Done. https://codereview.chromium.org/154653002/diff/980001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/980001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:181: value_bytes, &size) == ERROR_SUCCESS) { On 2014/02/19 15:55:36, grt wrote: > nit: indentation Done. https://codereview.chromium.org/154653002/diff/980001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util.cc:191: value_bytes, &size) == ERROR_SUCCESS) { On 2014/02/19 15:55:36, grt wrote: > nit: indentation Done. https://codereview.chromium.org/154653002/diff/980001/chrome_elf/chrome_elf_u... File chrome_elf/chrome_elf_util_unittest.cc (right): https://codereview.chromium.org/154653002/diff/980001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:30: L"C:\\Users\\user\\AppData\\Local\\Google\\Chrome SxS\\Application\\chrome.exe"; On 2014/02/19 15:55:36, grt wrote: > nit: break this string up so that the line isn't too long: > const wchar_t kCanaryExePath[] = > L"C:\\Users\\user\\AppData\\Local\\Google\\Chrome SxS\\Application" > L"\\chrome.exe"; Done. https://codereview.chromium.org/154653002/diff/980001/chrome_elf/chrome_elf_u... chrome_elf/chrome_elf_util_unittest.cc:114: TEST(ChromeElfUtilTest, CanaryTest) { On 2014/02/19 15:55:36, grt wrote: > please move this above the fixture class so they're not intermingled with the > test functions that use the fixture. thanks. Done.
lgtm!
Awesome! LGTM too. https://codereview.chromium.org/154653002/diff/1050001/chrome_elf/breakpad.cc File chrome_elf/breakpad.cc (right): https://codereview.chromium.org/154653002/diff/1050001/chrome_elf/breakpad.cc... chrome_elf/breakpad.cc:64: wchar_t* sid_string; nit: = NULL https://codereview.chromium.org/154653002/diff/1050001/chrome_elf/chrome_elf_... File chrome_elf/chrome_elf_util.cc (right): https://codereview.chromium.org/154653002/diff/1050001/chrome_elf/chrome_elf_... chrome_elf/chrome_elf_util.cc:63: } else if (raw_value[size/sizeof(wchar_t) - 1] != L'\0') { nit: spaces around /
+mark@ for breakpad/ owners approval of DEPS change (chrome_elf now depends on breakpad/src/client). background: this CL implements a bare-bones breakpad client which will give us data about any crashes that occur while chrome_elf is starting up (i.e. before any of chrome's breakpad support is up and running). Thanks!
LGTM
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/154653002/1130001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) chrome_elf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by caitkp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/154653002/1370001
Message was sent while issue was closed.
Change committed as 252870 |