Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad-handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

Sample renderer crash/1aed2bc785e28995
Sample browser: crash/66c822815474a5d8

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073, 546288, 456193
Committed: https://crrev.com/aaa2ff656f9fcabcbdd9e9964e3b2bdc8f5102ed
Cr-Commit-Position: refs/heads/master@{#361742}

[scottmg, 2015-11-03 18:57:19]

Description was changed from

==========
XXX WIP: Crashpad Windows

BUG=546288
==========

to

==========
WIP: Crashpad Windows

BUG=546288
==========

[scottmg, 2015-11-06 19:49:59]

Patchset #2 (id:20001) has been deleted

[scottmg, 2015-11-06 19:50:05]

Patchset #2 (id:40001) has been deleted

[scottmg, 2015-11-06 19:50:10]

Patchset #2 (id:60001) has been deleted

[scottmg, 2015-11-06 19:50:15]

Patchset #2 (id:80001) has been deleted

[scottmg, 2015-11-06 19:50:22]

Patchset #3 (id:120001) has been deleted

[scottmg, 2015-11-06 19:50:28]

Patchset #2 (id:100001) has been deleted

[scottmg, 2015-11-06 19:50:33]

Patchset #3 (id:160001) has been deleted

[scottmg, 2015-11-06 19:50:38]

Patchset #4 (id:200001) has been deleted

[scottmg, 2015-11-06 19:50:42]

Patchset #3 (id:180001) has been deleted

[scottmg, 2015-11-06 20:51:50]

Description was changed from

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad_handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

See also http://crbug.com/427611/.

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288
==========

to

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad_handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

See also http://crbug.com/427611/.

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193
==========

[scottmg, 2015-11-06 20:51:51]

scottmg@chromium.org changed reviewers:
+ cpu@chromium.org, mark@chromium.org

[scottmg, 2015-11-06 22:12:28]

Description was changed from

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad_handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

See also http://crbug.com/427611/.

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193
==========

to

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad_handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193
==========

[scottmg, 2015-11-06 22:26:44]

Patchset #7 (id:300001) has been deleted

[cpu_(ooo_6.6-7.5), 2015-11-06 23:16:48]

https://codereview.chromium.org/1416133003/diff/280001/build/filename_rules.gypi
File build/filename_rules.gypi (right):

https://codereview.chromium.org/1416133003/diff/280001/build/filename_rules.g...
build/filename_rules.gypi:19: }],
you can probably push this change now outside of this CL?

https://codereview.chromium.org/1416133003/diff/280001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1416133003/diff/280001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:128: bool RunAsCrashpadHandlerIfSpecified(int*
rc) {
don't like the "IfSpecified"

https://codereview.chromium.org/1416133003/diff/280001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:136: // We're dropping args[i], so reserve one
less than num_args for the
is this not possible with base/command_line ?

[scottmg, 2015-11-07 00:39:43]

Thanks for taking a look.

I'm not overly confident that I'm emulating all of the things that breakpad was
doing, so if you have any high-level thoughts on that it would be very
appreciated.

https://codereview.chromium.org/1416133003/diff/280001/build/filename_rules.gypi
File build/filename_rules.gypi (right):

https://codereview.chromium.org/1416133003/diff/280001/build/filename_rules.g...
build/filename_rules.gypi:19: }],
On 2015/11/06 23:16:48, cpu wrote:
> you can probably push this change now outside of this CL?

Sure, https://codereview.chromium.org/1413883010.

https://codereview.chromium.org/1416133003/diff/280001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1416133003/diff/280001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:128: bool RunAsCrashpadHandlerIfSpecified(int*
rc) {
On 2015/11/06 23:16:48, cpu wrote:
> don't like the "IfSpecified"

Done.

https://codereview.chromium.org/1416133003/diff/280001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:136: // We're dropping args[i], so reserve one
less than num_args for the
On 2015/11/06 23:16:48, cpu wrote:
> is this not possible with base/command_line ?

I have trust issues with how much that code does. Switched.

https://codereview.chromium.org/1416133003/diff/280001/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1416133003/diff/280001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:92: // Returns the directory in which the
currently running executable resides.
This was copied verbatim from the breakpad integration code.

https://codereview.chromium.org/1416133003/diff/280001/components/crash/conte...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/1416133003/diff/280001/components/crash/conte...
components/crash/content/app/crashpad.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
This is basically unchanged from crashpad_mac.mm, but it doesn't find the base
unfortunately.

https://codereview.chromium.org/1416133003/diff/280001/components/crash/conte...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1416133003/diff/280001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:76: #ifdef _WIN64
This is mostly unchanged from breakpad integration code, except to use Crashpad
to write the dump rather than Breakpad of course.

[cpu_(ooo_6.6-7.5), 2015-11-07 01:53:18]

looks great, a few comments

https://codereview.chromium.org/1416133003/diff/120002/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1416133003/diff/120002/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:157: base::CommandLine::Init(0, NULL);
lets move this Init(0, NULL) inside the RunAsCrashpadHandler

also note line 151, it might mess with your cmdline processing so if you can
test with asan.

https://codereview.chromium.org/1416133003/diff/120002/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1416133003/diff/120002/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:124: if (base::win::IsMetroProcess())
mmm .. see line 86

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
components/crash/content/app/crashpad.cc:77: 
check that message start is less than string.size() ?

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
components/crash/content/app/crashpad.cc:104: // sanitization.
yeah... wheee! leaky layers!!!


leave as-is

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
components/crash/content/app/crashpad.cc:133: }
http://www.boost.org/doc/libs/1_55_0/doc/html/tribool/tutorial.html#idp225162440

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
components/crash/content/app/crashpad.cc:231:
std::sort(uploaded_reports->begin(), uploaded_reports->end(), sort_by_time);
you could use a lambda here ...

but whatever.

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
components/crash/content/app/crashpad_win.cc:126: // committed and reserved for
breakpad. What could possibly go wrong?
love it.

[scottmg, 2015-11-09 18:29:56]

Thanks

https://codereview.chromium.org/1416133003/diff/120002/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1416133003/diff/120002/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:157: base::CommandLine::Init(0, NULL);
On 2015/11/07 01:53:17, cpu wrote:
> lets move this Init(0, NULL) inside the RunAsCrashpadHandler
> 
> also note line 151, it might mess with your cmdline processing so if you can
> test with asan.

I think it's better out here, since it's also initializing the command line for
when it's not running as crashpad_handler (I moved this up from below.)

Maybe instead I'll move this back to where it was for the global
rest-of-the-program operation, but use CommandLine::FromString() inside
RunAsCrashpadHandler to have a local unmodified one for that function.

I'll see if I can figure out how to run asan on windows. I hope it doesn't
modify the return value of GetCommandLineW() though?

https://codereview.chromium.org/1416133003/diff/120002/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1416133003/diff/120002/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:124: if (base::win::IsMetroProcess())
On 2015/11/07 01:53:17, cpu wrote:
> mmm .. see line 86

Removed the local one.

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
components/crash/content/app/crashpad.cc:77: 
On 2015/11/07 01:53:17, cpu wrote:
> check that message start is less than string.size() ?

Done.

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
components/crash/content/app/crashpad.cc:133: }
On 2015/11/07 01:53:18, cpu wrote:
>
http://www.boost.org/doc/libs/1_55_0/doc/html/tribool/tutorial.html#idp225162440

I'll see if I can figure out how to deps-in boost.

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
components/crash/content/app/crashpad.cc:231:
std::sort(uploaded_reports->begin(), uploaded_reports->end(), sort_by_time);
On 2015/11/07 01:53:18, cpu wrote:
> you could use a lambda here ...
> 
> but whatever.

Done.

[Mark Mentovai, 2015-11-09 18:40:20]

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
components/crash/content/app/crashpad.cc:133: }
scottmg wrote:
> On 2015/11/07 01:53:18, cpu wrote:
> >
>
http://www.boost.org/doc/libs/1_55_0/doc/html/tribool/tutorial.html#idp225162440
> 
> I'll see if I can figure out how to deps-in boost.

Don’t know if tribool’s ever been considered.
https://google.github.io/styleguide/cppguide.html#Boost

[scottmg, 2015-11-09 18:58:47]

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/1416133003/diff/120002/components/crash/conte...
components/crash/content/app/crashpad.cc:133: }
On 2015/11/09 18:40:20, Mark Mentovai wrote:
> scottmg wrote:
> > On 2015/11/07 01:53:18, cpu wrote:
> > >
> >
>
http://www.boost.org/doc/libs/1_55_0/doc/html/tribool/tutorial.html#idp225162440
> > 
> > I'll see if I can figure out how to deps-in boost.
> 
> Don’t know if tribool’s ever been considered.
> https://google.github.io/styleguide/cppguide.html#Boost

Oh wow, I was just joking! I didn't know we used any of it. Wouldn't want
Crashpad to have a dependency anyway.

[Mark Mentovai, 2015-11-09 19:01:58]

Oh, in that case, ha!

[cpu_(ooo_6.6-7.5), 2015-11-10 03:06:51]

lgtm  sans the IsMetroProcess() question.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (left):

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:237: if (base::win::GetVersion() >=
base::win::VERSION_WIN10)
scary ..

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:144: : dll_(nullptr),
metro_mode_(base::win::IsMetroProcess()) {
note that IsProcessImmersive() does not exist in win7 or earlier.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:155: 
oh this brings back memories...   carry on ...

[Mark Mentovai, 2015-11-10 17:21:18]

https://codereview.chromium.org/1416133003/diff/350001/DEPS
File DEPS (right):

https://codereview.chromium.org/1416133003/diff/350001/DEPS#newcode130
DEPS:130: Var('chromium_git') + '/crashpad/crashpad.git' + '@' +
'9e4cd8f07b107edf9cbba5c5760967d5aeb8e2bd',
Good for demo purposes, but this is a pretty big roll (because of how busy we’ve
been!)

I was going to break this out into its own DEPS roll, hopefully later today, to
isolate Mac impact a little bit.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:129: if
(command_line.HasSwitch("crashpad_handler")) {
If you’re using command_line, should this go into chrome_switches or something?

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:129: if
(command_line.HasSwitch("crashpad_handler")) {
I know Carlos asked you to change this, but since you’ve got to run through the
whole command line anyway, I don’t know if base::CommandLine gains much, and it
puts more unexpected stuff in between us and handler startup. You’re doing it up
to three times here, and you don’t need at least one of them.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:129: if
(command_line.HasSwitch("crashpad_handler")) {
--type=crashpad_handler would probably be more Chromey.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:159: if (RunAsCrashpadHandler(&rc))
Carry the wchar_t* argument to RunAsCrashpadHandler() so it doesn’t have to call
GetCommandLine(), but I guess you’d still need to call GetCommandLine()
ADDRESS_SANITIZER case, kind of how |instance| is handled.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:160: return rc;
It’d be cool to someday put the client init right here.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:170: base::CommandLine::Init(0, NULL);
If you moved this above RunAsCrashpadHandler(), it’d kind of justify using
CommandLine a bit more, because you could use FromCurrentProcess(). But this
should most likely not move above most of the other stuff that precedes it, and
that other stuff may be undesirable for crashpad_handler.

Dependencies. Yuck!

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_main...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_main...
chrome/app/chrome_main_delegate.cc:486: #if defined(OS_WIN) &&
!defined(COMPONENT_BUILD)
Not sure what this is about.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:99: // We wrap the call to MessageBoxW with a
SEH handler because it some
it→on

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:103: *exit_now = (IDOK != ::MessageBoxW(NULL,
text, caption, flags));
and maybe restyle this copied stuff to modern standards, nullptr, IDOK on the
right side.

https://codereview.chromium.org/1416133003/diff/350001/chrome/browser/crash_u...
File chrome/browser/crash_upload_list_mac.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/chrome/browser/crash_u...
chrome/browser/crash_upload_list_mac.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
You’ll want to do something to make chrome://crashes talk to Crashpad on
Windows, unless your changes in crashpad.cc were enough to make that work.

Perhaps this should become crash_upload_list_crashpad.{cc,h}.

https://codereview.chromium.org/1416133003/diff/350001/components/crash.gypi
File components/crash.gypi (right):

https://codereview.chromium.org/1416133003/diff/350001/components/crash.gypi#...
components/crash.gypi:93: # TODO(mark): https://crbug.com/466890: merge this
target with
Jeez, I really need to clean this up…

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad.cc:106: DCHECK(browser_process ||
process_type == "relauncher");
The relauncher side of this is Mac-specific.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad.cc:112: database_path =
Move the declaration to this line.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad.cc:112: database_path =
database_path will only be valid in the browser process (actually, if
initial_client is true for any reason, but we only care about it in the
browser). That comment lives on in the _mac and _win files, but it should
probably be duplicated here.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad.cc:127: // crash reports appear in
~/Library/Logs/DiagnosticReports provides a
This comment is Mac-specific.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad.cc:132:
crashpad_info->set_system_crash_reporter_forwarding(
This doesn’t have any effect on Windows at all. If it’s not possible to make it
work, maybe we should remove it from the interface to avoid the illusion that it
may work.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:28: static const char
kPipeNameVar[] = "CHROME_CRASHPAD_PIPE_NAME";
static’s not helping (maybe not really hurting either, but definitely not
helping).

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:43: process_annotations["prod"] =
base::UTF16ToUTF8(product_name);
It looks like components/crash/content/app/crash_keys_win.cc sets these for
Breakpad. It sets a few additional keys. You should decide which make sense as
process annotations and which make sense as module annotations, and set them up
as needed.

ptype, pid, and plugin-path are pretty obviously module, and I guess channel,
profile-type, and special would be process.

We should make sure that the crash keys we’re expecting are making it all the
way through to the crash server as part of our validation that this is ready to
replace Breakpad.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:45: #if defined(ARCH_CPU_X86)
build/build_config.h

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:47: #else
#elif defined(ARCH_CPU_X86_64)

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:50: std::string url =
"https://clients2.google.com/cr/report";
In GOOGLE_CHROME_BUILD + OFFICIAL_BUILD only, otherwise, an empty string. We
don’t want to be getting reports from random builds we’ve never heard of if
someone runs with a database whose settings say uploads are enabled.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:52: std::vector<std::string>
arguments;
(1, std::string("--crashpad_handler"))

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:74: CHECK(result);
CHECK() seems a little heavy-handed. You didn’t CHECK() that StartHandler() or
SetHandlerIPCPipe() succeeded, but both of those seem like the same kind of
thing.

I’ve been thinking of getting rid of UseHandler() and just making it something
that StartHandler() and SetHandlerIPCPipe() do on their own. I’m finding it hard
to come up with good reasons for keeping them separate now that we’ve got both
working.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:92: #ifdef _WIN64
ARCH_CPU_X86_64

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:102: // See
http://msdn.microsoft.com/en-us/library/ddssxxy8.aspx
https

[scottmg, 2015-11-16 21:48:40]

Sorry for the very slow turnaround on this, a bunch of (unrelated) things have
come up this last week. There's two substantive things I haven't addressed yet:

1) It doesn't link after making the upload list "work" -- components/crash is
only linked into chrome.exe (which is probably where it should be) but
chrome/browser wants to GetUploadedReports(). (It also can't just be linked into
both because then crash_reporter::internal::g_crashpad_client would have two
instances, one in the exe, one in the dll if we did that.) I don't think we want
chrome.dll to depend on functions imported from chrome.exe, so I need to think
more about what to do.

2) I need to modify Crashpad to do something different for passing process
annotations because there are per-process ones passed in StartHandler() (e.g.
pid) that need to be passed via UseHandler() so they can be different for the
renderers.

https://codereview.chromium.org/1416133003/diff/350001/DEPS
File DEPS (right):

https://codereview.chromium.org/1416133003/diff/350001/DEPS#newcode130
DEPS:130: Var('chromium_git') + '/crashpad/crashpad.git' + '@' +
'9e4cd8f07b107edf9cbba5c5760967d5aeb8e2bd',
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> Good for demo purposes, but this is a pretty big roll (because of how busy
we’ve
> been!)
> 
> I was going to break this out into its own DEPS roll, hopefully later today,
to
> isolate Mac impact a little bit.

OK, sounds good. I just needed it locally for testing, but having a separate
roll would be good.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:129: if
(command_line.HasSwitch("crashpad_handler")) {
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> I know Carlos asked you to change this, but since you’ve got to run through
the
> whole command line anyway, I don’t know if base::CommandLine gains much, and
it
> puts more unexpected stuff in between us and handler startup. You’re doing it
up
> to three times here, and you don’t need at least one of them.

Switching to base::CommandLine ended up being less code, so that seems somewhat
better. Looking at the current implementation of CommandLine::FromString() it's
not doing too much different than we'd be doing here.

The only thing I could find is that it has a few more options for treating --,
/, etc. as options, but I can't see how they would affect us here.

I'm not sure what you mean by 3 times, iterating through argv you mean?

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:129: if
(command_line.HasSwitch("crashpad_handler")) {
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> If you’re using command_line, should this go into chrome_switches or
something?

Done.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:129: if
(command_line.HasSwitch("crashpad_handler")) {
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> --type=crashpad_handler would probably be more Chromey.

Went with --type=crashpad-handler (_ seems pretty uncommon in Chrome command
line args).

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:159: if (RunAsCrashpadHandler(&rc))
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> Carry the wchar_t* argument to RunAsCrashpadHandler() so it doesn’t have to
call
> GetCommandLine(), but I guess you’d still need to call GetCommandLine()
> ADDRESS_SANITIZER case, kind of how |instance| is handled.

wWinMain's "command line" argument doesn't include the program name which
confuses CommandLine. I guess that's an argument for doing the processing
ourselves, but we'd have to add a [fake] program name for argv[0] for
HandlerMain() anyway.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:160: return rc;
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> It’d be cool to someday put the client init right here.

Yeah, I'm not sure if there's anything stopping us from doing that. I think
maybe a few small things like the Paths initialization order that I fiddled with
a bit.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:170: base::CommandLine::Init(0, NULL);
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> If you moved this above RunAsCrashpadHandler(), it’d kind of justify using
> CommandLine a bit more, because you could use FromCurrentProcess(). But this
> should most likely not move above most of the other stuff that precedes it,
and
> that other stuff may be undesirable for crashpad_handler.
> 
> Dependencies. Yuck!

Yeah, I moved it up, but then moved it back down again. I think it might be OK
to move it up and share initialization from looking at the code that gets called
between here and there.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (left):

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:237: if (base::win::GetVersion() >=
base::win::VERSION_WIN10)
On 2015/11/10 03:06:50, cpu wrote:
> scary ..

Yeah, I asked Will about this one, and he said it was OK to try removing. I'd
rather not have to hack exception-swallowing into Crashpad if we can avoid it.

In one of the linked bugs, we there's a log of the specific exception type we
should see in crash/ so we'll know pretty quickly if we're OK.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
File chrome/app/main_dll_loader_win.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:99: // We wrap the call to MessageBoxW with a
SEH handler because it some
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> it→on

Done.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:103: *exit_now = (IDOK != ::MessageBoxW(NULL,
text, caption, flags));
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> and maybe restyle this copied stuff to modern standards, nullptr, IDOK on the
> right side.

Done.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/main_dll_lo...
chrome/app/main_dll_loader_win.cc:144: : dll_(nullptr),
metro_mode_(base::win::IsMetroProcess()) {
On 2015/11/10 03:06:50, cpu wrote:
> note that IsProcessImmersive() does not exist in win7 or earlier.

Hmm, is that OK? IsMetroProcess
https://code.google.com/p/chromium/codesearch#chromium/src/base/win/metro.cc&...

-> IsProcessImmersive (ours) -> IsImmersiveProcess() (Windows') and otherwise
returns false. So on <= 7 we'll get false, which I think matches what this
wants?

https://codereview.chromium.org/1416133003/diff/350001/chrome/browser/crash_u...
File chrome/browser/crash_upload_list_mac.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/chrome/browser/crash_u...
chrome/browser/crash_upload_list_mac.cc:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> You’ll want to do something to make chrome://crashes talk to Crashpad on
> Windows, unless your changes in crashpad.cc were enough to make that work.
> 
> Perhaps this should become crash_upload_list_crashpad.{cc,h}.

Done.

https://codereview.chromium.org/1416133003/diff/350001/components/crash.gypi
File components/crash.gypi (right):

https://codereview.chromium.org/1416133003/diff/350001/components/crash.gypi#...
components/crash.gypi:93: # TODO(mark): https://crbug.com/466890: merge this
target with
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> Jeez, I really need to clean this up…

Yeah, sorry, I didn't make it any better. :( I just noticed there's remoting
which is also a (separate) Breakpad client that needs to be moved over to
Crashpad too.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad.cc:106: DCHECK(browser_process ||
process_type == "relauncher");
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> The relauncher side of this is Mac-specific.

Done.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad.cc:112: database_path =
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> database_path will only be valid in the browser process (actually, if
> initial_client is true for any reason, but we only care about it in the
> browser). That comment lives on in the _mac and _win files, but it should
> probably be duplicated here.

Done.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad.cc:112: database_path =
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> Move the declaration to this line.

Done.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad.cc:127: // crash reports appear in
~/Library/Logs/DiagnosticReports provides a
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> This comment is Mac-specific.

Done.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad.cc:132:
crashpad_info->set_system_crash_reporter_forwarding(
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> This doesn’t have any effect on Windows at all. If it’s not possible to make
it
> work, maybe we should remove it from the interface to avoid the illusion that
it
> may work.

I'm not sure if it's possible. I guess it might be possible to save the previous
unhandled exception filter and then pass it on, but I'm not sure we'd ever use
it.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:28: static const char
kPipeNameVar[] = "CHROME_CRASHPAD_PIPE_NAME";
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> static’s not helping (maybe not really hurting either, but definitely not
> helping).

Done.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:43: process_annotations["prod"] =
base::UTF16ToUTF8(product_name);
On 2015/11/10 17:21:18, Mark Mentovai wrote:
> It looks like components/crash/content/app/crash_keys_win.cc sets these for
> Breakpad. It sets a few additional keys. You should decide which make sense as
> process annotations and which make sense as module annotations, and set them
up
> as needed.
> 
> ptype, pid, and plugin-path are pretty obviously module, and I guess channel,
> profile-type, and special would be process.
> 
> We should make sure that the crash keys we’re expecting are making it all the
> way through to the crash server as part of our validation that this is ready
to
> replace Breakpad.

Hmm, yeah, that's a bit sticky. I guess I found a use for
StartHandler/UseHandler() split after all -- annotations will have to be passed
in UseHandler() instead of StartHandler() as on Windows there's process-specific
annotations added (but only one handler started of course).

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:45: #if defined(ARCH_CPU_X86)
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> build/build_config.h

Done.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:47: #else
On 2015/11/10 17:21:18, Mark Mentovai wrote:
> #elif defined(ARCH_CPU_X86_64)

Done.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:50: std::string url =
"https://clients2.google.com/cr/report";
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> In GOOGLE_CHROME_BUILD + OFFICIAL_BUILD only, otherwise, an empty string. We
> don’t want to be getting reports from random builds we’ve never heard of if
> someone runs with a database whose settings say uploads are enabled.

Done (Not OFFICIAL_BUILD, that's an optimization setting, not a product
configuration one).

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:52: std::vector<std::string>
arguments;
On 2015/11/10 17:21:18, Mark Mentovai wrote:
> (1, std::string("--crashpad_handler"))

Done.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:74: CHECK(result);
On 2015/11/10 17:21:18, Mark Mentovai wrote:
> CHECK() seems a little heavy-handed. You didn’t CHECK() that StartHandler() or
> SetHandlerIPCPipe() succeeded, but both of those seem like the same kind of
> thing.
> 
> I’ve been thinking of getting rid of UseHandler() and just making it something
> that StartHandler() and SetHandlerIPCPipe() do on their own. I’m finding it
hard
> to come up with good reasons for keeping them separate now that we’ve got both
> working.

Agreed, it seems unnecessary now, and I can't think of anything that would make
sense to have done between the two steps.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:92: #ifdef _WIN64
On 2015/11/10 17:21:17, Mark Mentovai wrote:
> ARCH_CPU_X86_64

Done.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:102: // See
http://msdn.microsoft.com/en-us/library/ddssxxy8.aspx
On 2015/11/10 17:21:18, Mark Mentovai wrote:
> https

Done.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:155: 
On 2015/11/10 03:06:50, cpu wrote:
> oh this brings back memories...   carry on ...

I don't understand any of this, so I'm glad you're enjoying it, at least.

[Mark Mentovai, 2015-11-16 22:11:13]

No new review yet, just responding to stuff.

scottmg wrote:
> Sorry for the very slow turnaround on this, a bunch of (unrelated) things have
> come up this last week. There's two substantive things I haven't addressed
yet:
> 
> 1) It doesn't link after making the upload list "work" -- components/crash is
> only linked into chrome.exe (which is probably where it should be) but
> chrome/browser wants to GetUploadedReports(). (It also can't just be linked
into
> both because then crash_reporter::internal::g_crashpad_client would have two
> instances, one in the exe, one in the dll if we did that.) I don't think we
want
> chrome.dll to depend on functions imported from chrome.exe, so I need to think
> more about what to do.

Does g_crashpad_client need to be in chrome::internal as you have it now or can
it be in an unnamed namespace? It’s not declared in any header or used by any
other file, so I bet that the unnamed namespace will work.

> 2) I need to modify Crashpad to do something different for passing process
> annotations because there are per-process ones passed in StartHandler() (e.g.
> pid) that need to be passed via UseHandler() so they can be different for the
> renderers.

Just make those into module annotations. We can up module annotations in
components/crash/content/app/crashpad.cc (or crashpad_win.cc) as easily as
process annotations. I don’t think UseHandler() needs to be involved in
annotations at all.

But do we really need pid? We carry that in the minidump elsewhere, and I’d find
it hard to imagine that anything in the procesing pipeline does anything useful
with it. Sure, OK, we can start with it for maximum Breakpad compatibility, but
ultimately, we ought to be able to get rid of it, and even “plat” too.

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:129: if
(command_line.HasSwitch("crashpad_handler")) {
scottmg wrote:
> I'm not sure what you mean by 3 times, iterating through argv you mean?

Yeah. But not the end of the world.

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad.cc:132:
crashpad_info->set_system_crash_reporter_forwarding(
scottmg wrote:
> I'm not sure if it's possible. I guess it might be possible to save the
previous
> unhandled exception filter and then pass it on, but I'm not sure we'd ever use
> it.

Yeah, I don’t know if it can be made to work without changing the “crash”
protocol from “signal an OS event” to “write something to a pipe handle you’re
hanging on to, with that goofy retry loop and all.”

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:50: std::string url =
"https://clients2.google.com/cr/report";
scottmg wrote:
> On 2015/11/10 17:21:17, Mark Mentovai wrote:
> > In GOOGLE_CHROME_BUILD + OFFICIAL_BUILD only, otherwise, an empty string. We
> > don’t want to be getting reports from random builds we’ve never heard of if
> > someone runs with a database whose settings say uploads are enabled.
> 
> Done (Not OFFICIAL_BUILD, that's an optimization setting, not a product
> configuration one).

Right, but do we ever want to upload reports from, say, official debug builds?

[scottmg, 2015-11-17 01:09:31]

Patchset #13 (id:430001) has been deleted

[Mark Mentovai, 2015-11-17 17:10:42]

I wrote:
> Yeah, I don’t know if it can be made to work without changing the “crash”
> protocol from “signal an OS event” to “write something to a pipe handle
you’re
> hanging on to, with that goofy retry loop and all.”

I don’t actually think we should do that. Not now, anyway. I was just thinking
out loud.

[scottmg, 2015-11-17 19:11:20]

Description was changed from

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad_handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193
==========

to

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad-handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193
==========

[scottmg, 2015-11-18 00:16:00]

Description was changed from

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad-handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193
==========

to

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad-handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

Sample renderer crash/1aed2bc785e28995
Sample browser: crash/ed0e3b57126a0a2c

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193
==========

[scottmg, 2015-11-18 18:36:18]

Patchset #20 (id:590001) has been deleted

[scottmg, 2015-11-19 01:25:41]

OK, I think ready for another look after a bunch of mucking around with build
files. :/

On 2015/11/16 22:11:13, Mark Mentovai wrote:
> No new review yet, just responding to stuff.
> 
> scottmg wrote:
> > Sorry for the very slow turnaround on this, a bunch of (unrelated) things
have
> > come up this last week. There's two substantive things I haven't addressed
> yet:
> > 
> > 1) It doesn't link after making the upload list "work" -- components/crash
is
> > only linked into chrome.exe (which is probably where it should be) but
> > chrome/browser wants to GetUploadedReports(). (It also can't just be linked
> into
> > both because then crash_reporter::internal::g_crashpad_client would have two
> > instances, one in the exe, one in the dll if we did that.) I don't think we
> want
> > chrome.dll to depend on functions imported from chrome.exe, so I need to
think
> > more about what to do.

I ended up moving it to the dll, like Mac. It means the client is initialized a
bit later (at the same point as Mac). Ideally, as you mentioned earlier, it'd be
best to have the client code only in the exe, and connect immediately after the
thunk to crashpad_handler. But that'll take a bunch more mucking around.

> 
> Does g_crashpad_client need to be in chrome::internal as you have it now or
can
> it be in an unnamed namespace? It’s not declared in any header or used by any
> other file, so I bet that the unnamed namespace will work.

Yup, done.

> 
> > 2) I need to modify Crashpad to do something different for passing process
> > annotations because there are per-process ones passed in StartHandler()
(e.g.
> > pid) that need to be passed via UseHandler() so they can be different for
the
> > renderers.
> 
> Just make those into module annotations. We can up module annotations in
> components/crash/content/app/crashpad.cc (or crashpad_win.cc) as easily as
> process annotations. I don’t think UseHandler() needs to be involved in
> annotations at all.

Windows is more like Mac now, and these are done in the common code as part of
the annotations map.

> 
> But do we really need pid? We carry that in the minidump elsewhere, and I’d
find
> it hard to imagine that anything in the procesing pipeline does anything
useful
> with it. Sure, OK, we can start with it for maximum Breakpad compatibility,
but
> ultimately, we ought to be able to get rid of it, and even “plat” too.

I kept it for now, but I can't see it being used anywhere other than the generic
"Fields" display in crash/.

> 
>
https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
> File chrome/app/chrome_exe_main_win.cc (right):
> 
>
https://codereview.chromium.org/1416133003/diff/350001/chrome/app/chrome_exe_...
> chrome/app/chrome_exe_main_win.cc:129: if
> (command_line.HasSwitch("crashpad_handler")) {
> scottmg wrote:
> > I'm not sure what you mean by 3 times, iterating through argv you mean?
> 
> Yeah. But not the end of the world.
> 
>
https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
> File components/crash/content/app/crashpad.cc (right):
> 
>
https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
> components/crash/content/app/crashpad.cc:132:
> crashpad_info->set_system_crash_reporter_forwarding(
> scottmg wrote:
> > I'm not sure if it's possible. I guess it might be possible to save the
> previous
> > unhandled exception filter and then pass it on, but I'm not sure we'd ever
use
> > it.
> 
> Yeah, I don’t know if it can be made to work without changing the “crash”
> protocol from “signal an OS event” to “write something to a pipe handle
you’re
> hanging on to, with that goofy retry loop and all.”
> 
>
https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
> File components/crash/content/app/crashpad_win.cc (right):
> 
>
https://codereview.chromium.org/1416133003/diff/350001/components/crash/conte...
> components/crash/content/app/crashpad_win.cc:50: std::string url =
> "https://clients2.google.com/cr/report";
> scottmg wrote:
> > On 2015/11/10 17:21:17, Mark Mentovai wrote:
> > > In GOOGLE_CHROME_BUILD + OFFICIAL_BUILD only, otherwise, an empty string.
We
> > > don’t want to be getting reports from random builds we’ve never heard of
if
> > > someone runs with a database whose settings say uploads are enabled.
> > 
> > Done (Not OFFICIAL_BUILD, that's an optimization setting, not a product
> > configuration one).
> 
> Right, but do we ever want to upload reports from, say, official debug builds?

buildtype=Official debug won't compile I don't think. But if you mean "chrome"
by "official" then, yeah, mostly only me when testing. But I don't like putting
behaviour on an optimization flag. (Also, Release with buildtype=Official takes
multiple hours to link on Windows.)

[scottmg, 2015-11-19 17:39:33]

[[Looks like we have a couple problems semi-unrelated to this CL:

- GN isn't handling the .asm file properly:
https://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng...

- CreateProcess() in StartHandler() is failing with
"[3320:4864:1118/221435:63464467:ERROR:crashpad_client_win.cc(293)]
CreateProcess: The parameter is incorrect. (0x57)" on some bots. I debugged this
a little and it seems to be related to the handle restriction, in that if I
don't pass EXTENDED_STARTUPINFO_PRESENT it's OK. But I'll track down what's
actually happening now.
]]

[Mark Mentovai, 2015-11-19 22:21:38]

You fixed the CreateProcess() thing so I guess now we’re just down to the gn asm
problem?

https://codereview.chromium.org/1416133003/diff/910001/build/secondary/third_...
File build/secondary/third_party/crashpad/crashpad/client/BUILD.gn (right):

https://codereview.chromium.org/1416133003/diff/910001/build/secondary/third_...
build/secondary/third_party/crashpad/crashpad/client/BUILD.gn:32: if (is_mac) {
File a bug about making the filename rules apply to .S?

Wonder when we’ll want to start carrying this file in Crashpad.

https://codereview.chromium.org/1416133003/diff/910001/build/secondary/third_...
File build/secondary/third_party/crashpad/crashpad/util/BUILD.gn (right):

https://codereview.chromium.org/1416133003/diff/910001/build/secondary/third_...
build/secondary/third_party/crashpad/crashpad/util/BUILD.gn:118:
"win/address_types.h",
If gn’s rules don’t understand mac/, do they understand win/?

https://codereview.chromium.org/1416133003/diff/910001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:134: L"--" +
base::UTF8ToUTF16(switches::kProcessType);
Leave an L"=" hanging on the end of this.

https://codereview.chromium.org/1416133003/diff/910001/chrome/app/chrome_main...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/chrome/app/chrome_main...
chrome/app/chrome_main_delegate.cc:679: InitMacCrashReporter(command_line,
process_type);
After you land this, it seems like a good idea to let the Mac and Windows
implementations actually just share the InitializeCrashpad() line you’re adding.

https://codereview.chromium.org/1416133003/diff/910001/chrome/chrome_tests_un...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1416133003/diff/910001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:2062: 'app/chrome_crash_reporter_client.cc',
Weird file, being built in several targets…

https://codereview.chromium.org/1416133003/diff/910001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:1252: // platforms, crashpad_handler is a
standalone binary.
binary→executable

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad.cc:154: SetCrashKeyValue("pid",
base::IntToString(::GetCurrentProcessId()).c_str());
Why’d you do c_str() here if it wasn’t needed for OS_POSIX?

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad.cc:192: void
InitializeCrashpadDatabaseOnly() {
Unused as far as I can tell.

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:63: std::string url = "";
Without the = "" should be slightly cheaper.

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:83:
DCHECK(!env->HasVar(kPipeNameVar));
If the browser restarts itself (like after a change in about:flags), will this
be tripped?

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:108: extern "C" int
__declspec(dllexport) CrashForException(
This and the other functions after this point (except where noted) can come out
of the namespace (since you’re not really using the namespacing at all) and
share a big extern "C" {}.

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:115: int
CrashForExceptionInNonABICompliantCodeRange(
This can be in an anonymous namespace.

https://codereview.chromium.org/1416133003/diff/910001/components/crash/core/...
File components/crash/core/common/crash_keys.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/components/crash/core/...
components/crash/core/common/crash_keys.cc:21: const char kClientId[] = "guid";
Crashpad owns the "guid" key, what’s this about?

[scottmg, 2015-11-20 19:52:48]

Thanks. (Bots should all be green now, at long last.)

https://codereview.chromium.org/1416133003/diff/910001/build/secondary/third_...
File build/secondary/third_party/crashpad/crashpad/client/BUILD.gn (right):

https://codereview.chromium.org/1416133003/diff/910001/build/secondary/third_...
build/secondary/third_party/crashpad/crashpad/client/BUILD.gn:32: if (is_mac) {
On 2015/11/19 22:21:37, Mark Mentovai wrote:
> File a bug about making the filename rules apply to .S?
> 
> Wonder when we’ll want to start carrying this file in Crashpad.

The guideline is not to, unless it's required really broadly:
https://code.google.com/p/chromium/codesearch#chromium/src/build/config/BUILD...

Yeah, we should probably move these into the Crashpad repo instead so there's
less fiddling to do at roll time, and we can test the GN build on the Crashpad
waterfall. I didn't realize there was as much Win GN on the waterfall as there
is.

https://codereview.chromium.org/1416133003/diff/910001/build/secondary/third_...
File build/secondary/third_party/crashpad/crashpad/util/BUILD.gn (right):

https://codereview.chromium.org/1416133003/diff/910001/build/secondary/third_...
build/secondary/third_party/crashpad/crashpad/util/BUILD.gn:118:
"win/address_types.h",
On 2015/11/19 22:21:37, Mark Mentovai wrote:
> If gn’s rules don’t understand mac/, do they understand win/?

It actually understands "mac/" (and "win/") but not "mach/". I moved only the
mach/ out instead.

https://codereview.chromium.org/1416133003/diff/910001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_win.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_win.cc:134: L"--" +
base::UTF8ToUTF16(switches::kProcessType);
On 2015/11/19 22:21:37, Mark Mentovai wrote:
> Leave an L"=" hanging on the end of this.

Done.

https://codereview.chromium.org/1416133003/diff/910001/chrome/app/chrome_main...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/chrome/app/chrome_main...
chrome/app/chrome_main_delegate.cc:679: InitMacCrashReporter(command_line,
process_type);
On 2015/11/19 22:21:37, Mark Mentovai wrote:
> After you land this, it seems like a good idea to let the Mac and Windows
> implementations actually just share the InitializeCrashpad() line you’re
adding.

Yeah, sounds good.

https://codereview.chromium.org/1416133003/diff/910001/chrome/chrome_tests_un...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/1416133003/diff/910001/chrome/chrome_tests_un...
chrome/chrome_tests_unit.gypi:2062: 'app/chrome_crash_reporter_client.cc',
On 2015/11/19 22:21:37, Mark Mentovai wrote:
> Weird file, being built in several targets…

Yeah, I don't love it. I'm hoping to simplify in future CLs, hopefully we can
get all binaries over to Crashpad for Mac/Win soon which will simplify a bunch.

https://codereview.chromium.org/1416133003/diff/910001/chrome/common/chrome_s...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/chrome/common/chrome_s...
chrome/common/chrome_switches.cc:1252: // platforms, crashpad_handler is a
standalone binary.
On 2015/11/19 22:21:37, Mark Mentovai wrote:
> binary→executable

Done.

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad.cc:154: SetCrashKeyValue("pid",
base::IntToString(::GetCurrentProcessId()).c_str());
On 2015/11/19 22:21:37, Mark Mentovai wrote:
> Why’d you do c_str() here if it wasn’t needed for OS_POSIX?

Done.

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad.cc:192: void
InitializeCrashpadDatabaseOnly() {
On 2015/11/19 22:21:37, Mark Mentovai wrote:
> Unused as far as I can tell.

Oops. This was going to be what the DLL-side had to call to re-initialize with
its own instance of the database, so that it could fetch uploaded reports, when
the client registration lived only in the .exe. But I changed my mind on that
and forgot to delete this.

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:63: std::string url = "";
On 2015/11/19 22:21:38, Mark Mentovai wrote:
> Without the = "" should be slightly cheaper.

Done.

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:83:
DCHECK(!env->HasVar(kPipeNameVar));
On 2015/11/19 22:21:37, Mark Mentovai wrote:
> If the browser restarts itself (like after a change in about:flags), will this
> be tripped?

Possibly. It looks like some similar failures in browser_tests once I got past
the more generic failures.

https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...

I guess if it's set, we don't want to start at all even if we otherwise think
we're the first client.

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:108: extern "C" int
__declspec(dllexport) CrashForException(
On 2015/11/19 22:21:37, Mark Mentovai wrote:
> This and the other functions after this point (except where noted) can come
out
> of the namespace (since you’re not really using the namespacing at all) and
> share a big extern "C" {}.

Done.

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:115: int
CrashForExceptionInNonABICompliantCodeRange(
On 2015/11/19 22:21:37, Mark Mentovai wrote:
> This can be in an anonymous namespace.

How about static because of the inter-function calls?

https://codereview.chromium.org/1416133003/diff/910001/components/crash/core/...
File components/crash/core/common/crash_keys.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/components/crash/core/...
components/crash/core/common/crash_keys.cc:21: const char kClientId[] = "guid";
On 2015/11/19 22:21:38, Mark Mentovai wrote:
> Crashpad owns the "guid" key, what’s this about?

The Breakpad-using targets use this file too (content_shell, setup, etc.) and so
still need kClientId. Really need to get everything switched so that Mac & Win
== Crashpad everywhere.

[Mark Mentovai, 2015-11-20 20:00:23]

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:83:
DCHECK(!env->HasVar(kPipeNameVar));
scottmg wrote:
> I guess if it's set, we don't want to start at all even if we otherwise think
> we're the first client.

I don’t agree. I think that if we’re the browser, we should always start a
crashpad_handler.

What if the restart happened because a new Chrome version was installed? We want
to talk to the new crashpad_handler, which may have been updated. We also want
to start a new crashpad_handler because the version annotation goes to
crashpad_handler as an argument.

https://codereview.chromium.org/1416133003/diff/910001/components/crash/core/...
File components/crash/core/common/crash_keys.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/components/crash/core/...
components/crash/core/common/crash_keys.cc:21: const char kClientId[] = "guid";
scottmg wrote:
> On 2015/11/19 22:21:38, Mark Mentovai wrote:
> > Crashpad owns the "guid" key, what’s this about?
> 
> The Breakpad-using targets use this file too (content_shell, setup, etc.) and
so
> still need kClientId. Really need to get everything switched so that Mac & Win
> == Crashpad everywhere.

OK, can you make the comment say something more about that then, because “while
transitioning to Crashpad” told me that it was temporary, but not how things are
supposed to work now and how things will improve in the future.

[scottmg, 2015-11-20 21:30:10]

Thanks.

(Requires https://codereview.chromium.org/1470483002/ for x86 GN, sigh.)

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/components/crash/conte...
components/crash/content/app/crashpad_win.cc:83:
DCHECK(!env->HasVar(kPipeNameVar));
On 2015/11/20 20:00:23, Mark Mentovai wrote:
> scottmg wrote:
> > I guess if it's set, we don't want to start at all even if we otherwise
think
> > we're the first client.
> 
> I don’t agree. I think that if we’re the browser, we should always start a
> crashpad_handler.
> 
> What if the restart happened because a new Chrome version was installed? We
want
> to talk to the new crashpad_handler, which may have been updated. We also want
> to start a new crashpad_handler because the version annotation goes to
> crashpad_handler as an argument.

OK, makes sense. In the test case, it's browser_tests (which starts a
crashpad_handler) launching a sub-test consisting of a browser_tests
--as-browser, which is going to start a crashpad_handler. But that seems OK. So,
I've just removed the DCHECK here.

https://codereview.chromium.org/1416133003/diff/910001/components/crash/core/...
File components/crash/core/common/crash_keys.cc (right):

https://codereview.chromium.org/1416133003/diff/910001/components/crash/core/...
components/crash/core/common/crash_keys.cc:21: const char kClientId[] = "guid";
On 2015/11/20 20:00:23, Mark Mentovai wrote:
> scottmg wrote:
> > On 2015/11/19 22:21:38, Mark Mentovai wrote:
> > > Crashpad owns the "guid" key, what’s this about?
> > 
> > The Breakpad-using targets use this file too (content_shell, setup, etc.)
and
> so
> > still need kClientId. Really need to get everything switched so that Mac &
Win
> > == Crashpad everywhere.
> 
> OK, can you make the comment say something more about that then, because
“while
> transitioning to Crashpad” told me that it was temporary, but not how things
are
> supposed to work now and how things will improve in the future.

Done.

[Mark Mentovai, 2015-11-20 21:34:03]

scottmg wrote:
> Thanks.
> 
> (Requires https://codereview.chromium.org/1470483002/ for x86 GN, sigh.)

If that’s all you needed for gn asm, that’s not that bad. I was afraid that it
needed to be taught how to run ml64 or something.

[scottmg, 2015-11-20 21:37:54]

On 2015/11/20 21:34:03, Mark Mentovai wrote:
> scottmg wrote:
> > Thanks.
> > 
> > (Requires https://codereview.chromium.org/1470483002/ for x86 GN, sigh.)
> 
> If that’s all you needed for gn asm, that’s not that bad. I was afraid that it
> needed to be taught how to run ml64 or something.

Yeah, it uses gyp-win-tool asm-wrapper (for better or worse) so it's mostly the
same as gyp and seems to work OK.
https://code.google.com/p/chromium/codesearch#chromium/src/build/toolchain/wi...

[Mark Mentovai, 2015-11-20 22:35:56]

LGTM!!

Awesome job.

https://codereview.chromium.org/1416133003/diff/1030001/DEPS
File DEPS (right):

https://codereview.chromium.org/1416133003/diff/1030001/DEPS#newcode1
DEPS:1: # This file is used to manage the dependencies of the Chromium src repo.
It is
Patch set 41. Lucky 41. Here we go!

https://codereview.chromium.org/1416133003/diff/1030001/chrome/common/chrome_...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/1416133003/diff/1030001/chrome/common/chrome_...
chrome/common/chrome_switches.cc:1252: // platforms, crashpad_handler is a
standalone executable.
Capture the rationale for this structure here (unless you can think of a better
place for it): your concerns about binary whitelisting and overzealous antivirus
software and application firewalls, etc.

https://codereview.chromium.org/1416133003/diff/1030001/components/crash/cont...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/1416133003/diff/1030001/components/crash/cont...
components/crash/content/app/crashpad.cc:158: SetCrashKeyValue("plugin-path",
base::CommandLine::ForCurrentProcess()
I think this is better done in PpapiPluginMain() or the ChromeMainDelegate. We
shouldn’t be baking in too much knowledge of process types here except for the
well-noted exceptions that we’ve made because of things that can’t be done
elsewhere. I can’t think of a reason that we can’t do this during ppapi process
initialization.

https://codereview.chromium.org/1416133003/diff/1030001/components/crash/cont...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1416133003/diff/1030001/components/crash/cont...
components/crash/content/app/crashpad_win.cc:118: static int
CrashForExceptionInNonABICompliantCodeRange(
No funny calling convention attribute because this is just x86_64, right?

[scottmg, 2015-11-21 03:16:59]

Thanks.

I'm going to hold off on landing for the moment -- I just realized crash/ is
processing but discarding the minidumps for most browser crashes because they're
exceeding a size limit on the minidump (maybe 1M?), which would be annoying,
given the effort we put in to gather more information.

I'll see next week what the memory map looks like, and if there's anything
sensible we could remove, or capture less of. Otherwise, I'll ask the crash team
if they can consider increasing that limit.

https://codereview.chromium.org/1416133003/diff/1030001/DEPS
File DEPS (right):

https://codereview.chromium.org/1416133003/diff/1030001/DEPS#newcode1
DEPS:1: # This file is used to manage the dependencies of the Chromium src repo.
It is
On 2015/11/20 22:35:56, Mark Mentovai wrote:
> Patch set 41. Lucky 41. Here we go!

:-)

Sadly, I think there's probably a reasonably likelihood of a few re-lands. So
many configs and variations.

https://codereview.chromium.org/1416133003/diff/1030001/chrome/common/chrome_...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/1416133003/diff/1030001/chrome/common/chrome_...
chrome/common/chrome_switches.cc:1252: // platforms, crashpad_handler is a
standalone executable.
On 2015/11/20 22:35:56, Mark Mentovai wrote:
> Capture the rationale for this structure here (unless you can think of a
better
> place for it): your concerns about binary whitelisting and overzealous
antivirus
> software and application firewalls, etc.

Done.

https://codereview.chromium.org/1416133003/diff/1030001/components/crash/cont...
File components/crash/content/app/crashpad.cc (right):

https://codereview.chromium.org/1416133003/diff/1030001/components/crash/cont...
components/crash/content/app/crashpad.cc:158: SetCrashKeyValue("plugin-path",
base::CommandLine::ForCurrentProcess()
On 2015/11/20 22:35:56, Mark Mentovai wrote:
> I think this is better done in PpapiPluginMain() or the ChromeMainDelegate. We
> shouldn’t be baking in too much knowledge of process types here except for the
> well-noted exceptions that we’ve made because of things that can’t be done
> elsewhere. I can’t think of a reason that we can’t do this during ppapi
process
> initialization.

Done.

https://codereview.chromium.org/1416133003/diff/1030001/components/crash/cont...
File components/crash/content/app/crashpad_win.cc (right):

https://codereview.chromium.org/1416133003/diff/1030001/components/crash/cont...
components/crash/content/app/crashpad_win.cc:118: static int
CrashForExceptionInNonABICompliantCodeRange(
On 2015/11/20 22:35:56, Mark Mentovai wrote:
> No funny calling convention attribute because this is just x86_64, right?

Yeah, it's not doing anything on the other functions either, but I guess since
https://code.google.com/p/chromium/codesearch#chromium/src/gin/public/debug.h...
has it, it's best to keep them matching.

This function isn't exported or used outside of this file, so no need to do
anything odd with it.

[Mark Mentovai, 2015-11-21 14:53:07]

LGTM still.

Hopefully there won’t be too many re-lands, but I’m betting on there at least
being follow-ups. So much changing…

https://codereview.chromium.org/1416133003/diff/1050001/content/ppapi_plugin/...
File content/ppapi_plugin/ppapi_plugin_main.cc (right):

https://codereview.chromium.org/1416133003/diff/1050001/content/ppapi_plugin/...
content/ppapi_plugin/ppapi_plugin_main.cc:74: "plugin-path",
command_line.GetSwitchValueASCII("plugin-path"));
switches::kPluginPath for the one on the right side.

[scottmg, 2015-11-24 18:30:41]

Description was changed from

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad-handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

Sample renderer crash/1aed2bc785e28995
Sample browser: crash/ed0e3b57126a0a2c

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193
==========

to

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad-handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

Sample renderer crash/1aed2bc785e28995
Sample browser: crash/66c822815474a5d8

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193
==========

[scottmg, 2015-11-24 19:04:25]

In a static library build, the minidumps are ~300k smaller which seems to be
below crash's limit. So I think it's OK to land from that perspective.

Just need to track down why this is failing again (sometimes?)
https://bugs.chromium.org/p/crashpad/issues/detail?id=77 and then I'll land.

https://codereview.chromium.org/1416133003/diff/1050001/content/ppapi_plugin/...
File content/ppapi_plugin/ppapi_plugin_main.cc (right):

https://codereview.chromium.org/1416133003/diff/1050001/content/ppapi_plugin/...
content/ppapi_plugin/ppapi_plugin_main.cc:74: "plugin-path",
command_line.GetSwitchValueASCII("plugin-path"));
On 2015/11/21 14:53:07, Mark Mentovai wrote:
> switches::kPluginPath for the one on the right side.

I hadn't even checked, but looking now, it turns out we don't use the argument
for ppapi, so I just removed it entirely.

(e.g. from Canary:
"C:\Users\scott\AppData\Local\Google\Chrome SxS\Application\chrome.exe"
--type=ppapi --channel="15600.24.1123495397\746756095"
--ppapi-flash-args=enable_hw_video_decode=1 --lang=en-US
--device-scale-factor=1.5 --font-cache-shared-handle=12728 --ignored="
--type=renderer " /prefetch:-632637702
)

[scottmg, 2015-11-25 18:00:15]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2015-11-25 18:00:16]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
mark@chromium.org
Link to the patchset: https://codereview.chromium.org/1416133003/#ps1160001
(title: "add some stub gn files")

[commit-bot: I haz the power, 2015-11-25 18:02:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1416133003/1160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1416133003/1160001

[commit-bot: I haz the power, 2015-11-25 21:55:54]

Description was changed from

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad-handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

Sample renderer crash/1aed2bc785e28995
Sample browser: crash/66c822815474a5d8

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193
==========

to

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad-handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

Sample renderer crash/1aed2bc785e28995
Sample browser: crash/66c822815474a5d8

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193
==========

[commit-bot: I haz the power, 2015-11-25 21:55:55]

Committed patchset #48 (id:1160001)

[commit-bot: I haz the power, 2015-11-25 21:57:19]

Description was changed from

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad-handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

Sample renderer crash/1aed2bc785e28995
Sample browser: crash/66c822815474a5d8

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193
==========

to

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad-handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

Sample renderer crash/1aed2bc785e28995
Sample browser: crash/66c822815474a5d8

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193

Committed: https://crrev.com/aaa2ff656f9fcabcbdd9e9964e3b2bdc8f5102ed
Cr-Commit-Position: refs/heads/master@{#361742}
==========

[commit-bot: I haz the power, 2015-11-25 21:57:20]

Patchset 48 (id:??) landed as
https://crrev.com/aaa2ff656f9fcabcbdd9e9964e3b2bdc8f5102ed
Cr-Commit-Position: refs/heads/master@{#361742}

[raymes, 2015-11-25 23:24:06]

A revert of this CL (patchset #48 id:1160001) has been created in
https://codereview.chromium.org/1475213004/ by raymes@chromium.org.

The reason for reverting is: Link failure on
https://build.chromium.org/p/chromium.win/builders/Win8%20GN%20%28dbg%29/buil....

[hans, 2015-11-26 01:52:38]

This broke the Clang build on Windows. I've uploaded a fix here:
https://codereview.chromium.org/1473073008/

It would be great if we could commit and roll that in somehow.

[scottmg, 2015-11-26 02:22:22]

On 2015/11/26 01:52:38, hans wrote:
> This broke the Clang build on Windows. I've uploaded a fix here:
> https://codereview.chromium.org/1473073008/
> 
> It would be great if we could commit and roll that in somehow.

Thanks for that. I did test clang and made some fixes, but I only tried GN
clang, not gyp clang, and I think those build files were probably incomplete at
the time. Sigh.

I'm reverting for some excitingly failing tests so I'll be sure to include those
fixes in the next land.

[scottmg, 2015-11-26 02:23:15]

A revert of this CL (patchset #48 id:1160001) has been created in
https://codereview.chromium.org/1476193002/ by scottmg@chromium.org.

The reason for reverting is: Some failing tests
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....
Unclear why they didn't fail on trybots so I'm going to revert and investigate..

[scottmg, 2015-11-26 02:29:47]

Description was changed from

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad-handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

Sample renderer crash/1aed2bc785e28995
Sample browser: crash/66c822815474a5d8

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193

Committed: https://crrev.com/aaa2ff656f9fcabcbdd9e9964e3b2bdc8f5102ed
Cr-Commit-Position: refs/heads/master@{#361742}
==========

to

==========
Crashpad Windows: Use the Crashpad client instead of Breakpad on Windows

Crashpad is always compiled into chrome and its handler is always
enabled. It only uploads in Official builds.

On Windows, the crash handler is chrome.exe run with a
--crashpad-handler argument. This is due to concern about
incompatibilities of shipping an additional new different binary for AV,
firewalls, etc.

Sample renderer crash/1aed2bc785e28995
Sample browser: crash/66c822815474a5d8

See also http://crbug.com/427611 .

R=mark@chromium.org,cpu@chromium.org
BUG=447073,546288, 456193

Committed: https://crrev.com/aaa2ff656f9fcabcbdd9e9964e3b2bdc8f5102ed
Cr-Commit-Position: refs/heads/master@{#361742}
==========

[scottmg, 2015-11-26 02:42:24]

On 2015/11/26 02:23:15, scottmg wrote:
> A revert of this CL (patchset #48 id:1160001) has been created in
> https://codereview.chromium.org/1476193002/ by mailto:scottmg@chromium.org.
> 
> The reason for reverting is: Some failing tests
>
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....
> Unclear why they didn't fail on trybots so I'm going to revert and
investigate..

Ah, I'm guess it's because they're dbg only failures. I thought there was one of
those in the default tryjob set, but I guess only on the waterfall.

[scottmg, 2015-11-26 20:21:05]

Patchset #49 (id:1180001) has been deleted

[scottmg, 2015-11-27 16:40:41]

Patchset #49 (id:1200001) has been deleted