Fix up crash reporting in unit tests and the Chrome Frame helper processes:

Fix up crash reporting in unit tests and the Chrome Frame helper processes:

Bother to initialize crash reporting in chrome_frame_tests.exe.
Correctly construct the crash reporting pipe name for non-admin installs.

BUG=86846
TEST=Crash dumps are reported when running Chrome Frame tests in headless mode, user-level Chrome Frame helper processes connect to the crash service.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90255

[grt (UTC plus 2), 2011-06-22 14:15:01]

Wow, crash reporting that reports crashes!

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/chrome_frame.gyp
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/chrome_frame.gy...
chrome_frame/chrome_frame.gyp:405: '../breakpad/src',
would <(DEPTH) work here and elsewhere?

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/chrome_launcher...
File chrome_frame/chrome_launcher_main.cc (right):

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/chrome_launcher...
chrome_frame/chrome_launcher_main.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
2011 :-)

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
File chrome_frame/crash_server_init.cc (right):

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.cc:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
2011

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.cc:15: const wchar_t kFullMemoryCrashReport[] =
L"full-memory-crash-report";
Looks like this isn't used anymore.  Please remove.

Also, since --full-memory-crash-report no longer has meaning in
InitializeCrashReporting, should invokers of procs that link in this code be
modified to no longer use that switch?

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.cc:26: const DWORD kSize = sizeof(TOKEN_USER) +
SECURITY_MAX_SID_SIZE;
Google style guide says to use sizeof(varname) rather than (typename) wherever
possible.  That aside, consider the technique used in
http://msdn.microsoft.com/en-us/library/ee207402(v=VS.85).aspx since it doesn't
require sizeof computations or a cast.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.cc:52: GetModuleFileName(NULL, exe_path, MAX_PATH
* 2);
Will this not return the path to iexplore.exe when called from the context of
npchrome_frame.dll?  Is this desired?  If not, see base\base_paths_win.cc for
the technique to get the location of the dll.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.cc:52: GetModuleFileName(NULL, exe_path, MAX_PATH
* 2);
Rather than repeat MAX_PATH * 2, how about _countof?

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.cc:59: if (wcsstr(exe_path, program_files_path))
{
To be strictly correct, this should be:
  if (wcsstr(...) == exe_path)
although it will still do more work than needed if a match isn't found since all
of exe_path is searched for program_files_path.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
File chrome_frame/crash_server_init.h (right):

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.h:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
2011

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.h:20: extern const wchar_t
kFullMemoryCrashReport[];
This doesn't seem to be used anymore.  Remove it?

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.h:24: enum CrashReportingFlags {
I suggest CrashReportingMode.  Rationale: "flags" implies bits in a bitfield,
and a singular type name makes more sense in the context of a function
declaration since only one member of the type is used at a time.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.h:24: enum CrashReportingFlags {
Maybe add a comment here to explain what these mean?  Unless they're covered in
the comment for InitializeCrashReporting.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.h:35: // to delete this object which will shut
down the crash reporting machinery.
Please document flags/mode.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/test/run_all_un...
File chrome_frame/test/run_all_unittests.cc (right):

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/test/run_all_un...
chrome_frame/test/run_all_unittests.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
2011

[robertshield, 2011-06-23 02:22:51]

thanks for the review! ptal

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/chrome_frame.gyp
File chrome_frame/chrome_frame.gyp (right):

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/chrome_frame.gy...
chrome_frame/chrome_frame.gyp:405: '../breakpad/src',
On 2011/06/22 14:15:01, grt wrote:
> would <(DEPTH) work here and elsewhere?

done here. Unsure about changing it for the rest of the file. as the '../'
approach seems widespread in other .gyp files in the source tree.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/chrome_launcher...
File chrome_frame/chrome_launcher_main.cc (right):

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/chrome_launcher...
chrome_frame/chrome_launcher_main.cc:1: // Copyright (c) 2009 The Chromium
Authors. All rights reserved.
On 2011/06/22 14:15:01, grt wrote:
> 2011 :-)

Done.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
File chrome_frame/crash_server_init.cc (right):

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.cc:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
On 2011/06/22 14:15:01, grt wrote:
> 2011

Done.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.cc:15: const wchar_t kFullMemoryCrashReport[] =
L"full-memory-crash-report";
On 2011/06/22 14:15:01, grt wrote:
> Looks like this isn't used anymore.  Please remove.
> 
> Also, since --full-memory-crash-report no longer has meaning in
> InitializeCrashReporting, should invokers of procs that link in this code be
> modified to no longer use that switch?

The only place CF-related that it is used is by npchrome_frame when it invokes
chrome via chrome_launcher. Since chrome respects the flag, I won't remove it
from npchrome_frame.dll.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.cc:26: const DWORD kSize = sizeof(TOKEN_USER) +
SECURITY_MAX_SID_SIZE;
On 2011/06/22 14:15:01, grt wrote:
> Google style guide says to use sizeof(varname) rather than (typename) wherever
> possible.  That aside, consider the technique used in
> http://msdn.microsoft.com/en-us/library/ee207402%28v=VS.85%29.aspx since it
doesn't
> require sizeof computations or a cast.

Good idea, much nicer.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.cc:52: GetModuleFileName(NULL, exe_path, MAX_PATH
* 2);
On 2011/06/22 14:15:01, grt wrote:
> Will this not return the path to iexplore.exe when called from the context of
> npchrome_frame.dll?  Is this desired?  If not, see base\base_paths_win.cc for
> the technique to get the location of the dll.

This code is only linked into chrome_launcher.exe and chrome_frame_helper.exe
but the __ImageBase thing makes it a bit safer. Changed, thanks.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.cc:52: GetModuleFileName(NULL, exe_path, MAX_PATH
* 2);
On 2011/06/22 14:15:01, grt wrote:
> Rather than repeat MAX_PATH * 2, how about _countof?

Done.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.cc:59: if (wcsstr(exe_path, program_files_path))
{
On 2011/06/22 14:15:01, grt wrote:
> To be strictly correct, this should be:
>   if (wcsstr(...) == exe_path)
> although it will still do more work than needed if a match isn't found since
all
> of exe_path is searched for program_files_path.

Done.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
File chrome_frame/crash_server_init.h (right):

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.h:1: // Copyright (c) 2010 The Chromium Authors.
All rights reserved.
On 2011/06/22 14:15:01, grt wrote:
> 2011

Done.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.h:20: extern const wchar_t
kFullMemoryCrashReport[];
On 2011/06/22 14:15:01, grt wrote:
> This doesn't seem to be used anymore.  Remove it?

Done.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.h:24: enum CrashReportingFlags {
On 2011/06/22 14:15:01, grt wrote:
> Maybe add a comment here to explain what these mean?  Unless they're covered
in
> the comment for InitializeCrashReporting.

Done.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.h:24: enum CrashReportingFlags {
On 2011/06/22 14:15:01, grt wrote:
> I suggest CrashReportingMode.  Rationale: "flags" implies bits in a bitfield,
> and a singular type name makes more sense in the context of a function
> declaration since only one member of the type is used at a time.

Done.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/crash_server_in...
chrome_frame/crash_server_init.h:35: // to delete this object which will shut
down the crash reporting machinery.
On 2011/06/22 14:15:01, grt wrote:
> Please document flags/mode.

Documented above.

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/test/run_all_un...
File chrome_frame/test/run_all_unittests.cc (right):

http://codereview.chromium.org/7219007/diff/4003/chrome_frame/test/run_all_un...
chrome_frame/test/run_all_unittests.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2011/06/22 14:15:01, grt wrote:
> 2011

Done.

[grt (UTC plus 2), 2011-06-23 03:28:22]

LGTM once you address the two nits below.  Cheerio!

http://codereview.chromium.org/7219007/diff/10001/chrome_frame/crash_server_i...
File chrome_frame/crash_server_init.cc (right):

http://codereview.chromium.org/7219007/diff/10001/chrome_frame/crash_server_i...
chrome_frame/crash_server_init.cc:87: CrashReportingMode flags) {
flags -> mode

http://codereview.chromium.org/7219007/diff/10001/chrome_frame/crash_server_i...
File chrome_frame/crash_server_init.h (right):

http://codereview.chromium.org/7219007/diff/10001/chrome_frame/crash_server_i...
chrome_frame/crash_server_init.h:33: CrashReportingMode flags);
flags -> mode

[robertshield, 2011-06-23 13:52:08]

Thanks!

http://codereview.chromium.org/7219007/diff/10001/chrome_frame/crash_server_i...
File chrome_frame/crash_server_init.cc (right):

http://codereview.chromium.org/7219007/diff/10001/chrome_frame/crash_server_i...
chrome_frame/crash_server_init.cc:87: CrashReportingMode flags) {
On 2011/06/23 03:28:22, grt wrote:
> flags -> mode

Done.

http://codereview.chromium.org/7219007/diff/10001/chrome_frame/crash_server_i...
File chrome_frame/crash_server_init.h (right):

http://codereview.chromium.org/7219007/diff/10001/chrome_frame/crash_server_i...
chrome_frame/crash_server_init.h:33: CrashReportingMode flags);
On 2011/06/23 03:28:22, grt wrote:
> flags -> mode

Done.