Integrate fallback crash handling in chrome

Integrate fallback crash handling in chrome

TBR=grt
BUG=678959
Review-Url: https://codereview.chromium.org/2638433002
Cr-Commit-Position: refs/heads/master@{#443963}
Committed: https://chromium.googlesource.com/chromium/src/+/62fd0c1ec30ee121aa0720d8d7d390cc7e9ba226

[Sigurður Ásgeirsson, 2017-01-13 21:59:41]

Description was changed from

==========
Integrate fallback crash handler with run_as_crashpad_handler_win

BUG=
==========

to

==========
Integrate fallback crash handling in chrome

BUG=678959
==========

[Sigurður Ásgeirsson, 2017-01-13 21:59:42]

siggi@chromium.org changed reviewers:
+ scottmg@chromium.org

[Sigurður Ásgeirsson, 2017-01-13 22:00:50]

Hey Scott,

this is a lot^Wbit less ugly than my first attempt. I don't love the file names
- suggestions?

NB: I've yet to test this.

Siggi

[Sigurður Ásgeirsson, 2017-01-13 22:02:20]

siggi@chromium.org changed reviewers:
+ grt@google.com

[Sigurður Ásgeirsson, 2017-01-13 22:02:21]

Hey Greg,

mind taking a look at my use of install_static here?

Siggi

[scottmg, 2017-01-13 22:20:00]

I think the name's fine.

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
File components/crash/content/app/fallback_crash_handling_win.cc (right):

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win.cc:8: #include <string>
Can remove this, it's already in the header

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win.cc:25: LONG WINAPI
FallbackUnhandledExceptionFilter(EXCEPTION_POINTERS* exc_ptrs) {
This doesn't seem to be used?

And apropros of that, any way we could test this? :)

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
File components/crash/content/app/fallback_crash_handling_win.h (right):

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win.h:4: #ifndef
COMPONENTS_CRASH_CONTENT_APP_FALLBACK_CRASH_HANDLING_WIN_H_
\n before this line

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win.h:24: // that's
previously called SetupfallbackCrashHandler.
Capital F on SetupFallback...

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win.h:26: // as propoerties
of the crash for the purposes of upload.
properties

[Sigurður Ásgeirsson, 2017-01-13 22:40:28]

Thanks, glad you hate it less than my first cut. There's too much hate in the
world.

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
File components/crash/content/app/fallback_crash_handling_win.cc (right):

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win.cc:25: LONG WINAPI
FallbackUnhandledExceptionFilter(EXCEPTION_POINTERS* exc_ptrs) {
On 2017/01/13 22:20:00, scottmg wrote:
> This doesn't seem to be used?
> 
> And apropros of that, any way we could test this? :)

I sure hope so. Monday, first thing. I wanna ship this thing...

[Sigurður Ásgeirsson, 2017-01-16 18:18:39]

The CQ bit was checked by siggi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 18:18:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sigurður Ásgeirsson, 2017-01-16 18:23:24]

Thanks - now with testing, and may actually work.

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
File components/crash/content/app/fallback_crash_handling_win.cc (right):

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win.cc:8: #include <string>
On 2017/01/13 22:20:00, scottmg wrote:
> Can remove this, it's already in the header

Done.

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
File components/crash/content/app/fallback_crash_handling_win.h (right):

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win.h:4: #ifndef
COMPONENTS_CRASH_CONTENT_APP_FALLBACK_CRASH_HANDLING_WIN_H_
On 2017/01/13 22:20:00, scottmg wrote:
> \n before this line

Done.

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win.h:24: // that's
previously called SetupfallbackCrashHandler.
On 2017/01/13 22:20:00, scottmg wrote:
> Capital F on SetupFallback...

Done.

https://codereview.chromium.org/2638433002/diff/20001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win.h:26: // as propoerties
of the crash for the purposes of upload.
On 2017/01/13 22:20:00, scottmg wrote:
> properties

Done.

[Sigurður Ásgeirsson, 2017-01-16 18:26:31]

The CQ bit was checked by siggi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 18:26:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2017-01-16 18:32:57]

https://codereview.chromium.org/2638433002/diff/60001/components/crash/conten...
File components/crash/content/app/fallback_crash_handling_win.cc (right):

https://codereview.chromium.org/2638433002/diff/60001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win.cc:69:
SetUnhandledExceptionFilter(&FallbackUnhandledExceptionFilter);
I have the feeling this won't do anything, due to
https://cs.chromium.org/chromium/src/chrome_elf/chrome_elf_main.cc?rcl=0&l=53.
Is that incorrect?

[scottmg, 2017-01-16 18:40:29]

Generally seems fine. I'm not sure what we should do about the disabled SetUEF
though.

https://codereview.chromium.org/2638433002/diff/60001/components/crash/conten...
File components/crash/content/app/fallback_crash_handling_win_unittest.cc
(right):

https://codereview.chromium.org/2638433002/diff/60001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win_unittest.cc:59: //
Launch a subprocess to
Finish this comment.

[Sigurður Ásgeirsson, 2017-01-16 18:52:44]

Thanks - I'm going to try this on a full release build of Chrome, see whether it
holds up.

https://codereview.chromium.org/2638433002/diff/60001/components/crash/conten...
File components/crash/content/app/fallback_crash_handling_win.cc (right):

https://codereview.chromium.org/2638433002/diff/60001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win.cc:69:
SetUnhandledExceptionFilter(&FallbackUnhandledExceptionFilter);
On 2017/01/16 18:32:57, scottmg wrote:
> I have the feeling this won't do anything, due to
> https://cs.chromium.org/chromium/src/chrome_elf/chrome_elf_main.cc?rcl=0&l=53.
> Is that incorrect?

I think you may be right - I haven't tested this in a full build of Chrome yet.

https://codereview.chromium.org/2638433002/diff/60001/components/crash/conten...
File components/crash/content/app/fallback_crash_handling_win_unittest.cc
(right):

https://codereview.chromium.org/2638433002/diff/60001/components/crash/conten...
components/crash/content/app/fallback_crash_handling_win_unittest.cc:59: //
Launch a subprocess to
On 2017/01/16 18:40:29, scottmg wrote:
> Finish this comment.

Done.

[Sigurður Ásgeirsson, 2017-01-16 20:48:05]

The CQ bit was checked by siggi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 20:48:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sigurður Ásgeirsson, 2017-01-16 20:49:29]

Ok, I verified that as-was, this was hitting the stubbed-out SetUEF. Also
verified that this now is going to kernel32!SetUEF. I'm not sure how I'd go
about getting the handler to crash, but maybe we can let this ride and see what
it nets us in tomorrow's canary?

[scottmg, 2017-01-16 20:56:41]

I think it's OK to just test the handler crash manually to satisfy yourself that
a fallback crash will actually get something into the database.

And assuming that works, lgtm.

[Sigurður Ásgeirsson, 2017-01-16 21:46:19]

Description was changed from

==========
Integrate fallback crash handling in chrome

BUG=678959
==========

to

==========
Integrate fallback crash handling in chrome

TBR=grt
BUG=678959
==========

[Sigurður Ásgeirsson, 2017-01-16 21:47:00]

Now actually works - I'd love to get this landed for tomorrow's Canary, so I've
added grt to TBR for the install_static changes.

[Sigurður Ásgeirsson, 2017-01-16 21:47:09]

The CQ bit was checked by siggi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 21:47:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sigurður Ásgeirsson, 2017-01-16 21:55:19]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2017-01-16 21:55:20]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2638433002/#ps120001
(title: "Misc fixes to allow the fallback handler to actually work.")

[commit-bot: I haz the power, 2017-01-16 21:55:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Sigurður Ásgeirsson, 2017-01-16 22:05:04]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2017-01-16 22:05:04]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2638433002/#ps140001
(title: "Fix the thread_id recorded in the minidump.")

[commit-bot: I haz the power, 2017-01-16 22:05:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 23:16:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 23:16:53]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Sigurður Ásgeirsson, 2017-01-16 23:37:56]

The CQ bit was checked by siggi@chromium.org

[Sigurður Ásgeirsson, 2017-01-16 23:37:56]

The patchset sent to the CQ was uploaded after l-g-t-m from scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2638433002/#ps160001
(title: "Fix signedness comparison goof.")

[commit-bot: I haz the power, 2017-01-16 23:38:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-17 00:34:28]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1484609876445900,
"parent_rev": "b21c4de55ac00562dab6c1f5848a1cf11966e00a", "commit_rev":
"62fd0c1ec30ee121aa0720d8d7d390cc7e9ba226"}

[commit-bot: I haz the power, 2017-01-17 00:35:00]

Description was changed from

==========
Integrate fallback crash handling in chrome

TBR=grt
BUG=678959
==========

to

==========
Integrate fallback crash handling in chrome

TBR=grt
BUG=678959

Review-Url: https://codereview.chromium.org/2638433002
Cr-Commit-Position: refs/heads/master@{#443963}
Committed:
https://chromium.googlesource.com/chromium/src/+/62fd0c1ec30ee121aa0720d8d7d3...
==========

[commit-bot: I haz the power, 2017-01-17 00:35:02]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/62fd0c1ec30ee121aa0720d8d7d3...

[brucedawson, 2017-01-26 19:43:20]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2017-01-26 19:43:21]

This seems wrong, and it also surprises me that this wasn't caught by the sizes
checks - we used to check for global constructors/destructors, I thought.

https://codereview.chromium.org/2638433002/diff/160001/components/crash/conte...
File components/crash/content/app/fallback_crash_handling_win.cc (right):

https://codereview.chromium.org/2638433002/diff/160001/components/crash/conte...
components/crash/content/app/fallback_crash_handling_win.cc:27:
std::unique_ptr<FallbackCrashHandlerLauncher> g_fallback_crash_handler_launcher;
Why are we using a global variable with a constructor and destructor here?
Doesn't this violate the style guide?

I discovered this by stepping through all of the global initializers called by
{,,chrome.exe}_initterm

[scottmg, 2017-01-26 20:34:28]

On 2017/01/26 19:43:21, brucedawson wrote:
> This seems wrong, and it also surprises me that this wasn't caught by the
sizes
> checks - we used to check for global constructors/destructors, I thought.
> 
>
https://codereview.chromium.org/2638433002/diff/160001/components/crash/conte...
> File components/crash/content/app/fallback_crash_handling_win.cc (right):
> 
>
https://codereview.chromium.org/2638433002/diff/160001/components/crash/conte...
> components/crash/content/app/fallback_crash_handling_win.cc:27:
> std::unique_ptr<FallbackCrashHandlerLauncher>
g_fallback_crash_handler_launcher;
> Why are we using a global variable with a constructor and destructor here?
> Doesn't this violate the style guide?
> 
> I discovered this by stepping through all of the global initializers called by
> {,,chrome.exe}_initterm

Yeah, you're right, that ought to be a Lazy/Leaky/pointer, sorry.

We've never had any enforcement of that for Windows unfortunately. I wrote a DIA
tool long ago in tools/win/static_initializers but around the same time as I
wrote it we switched from a bot step to a slightly more magical perf bot
tracker, and it was never integrated into that dashboard.

(The short version is ... we had quite a lot of static initializers on Windows
last time I checked.)

[brucedawson, 2017-01-27 19:00:31]

> Yeah, you're right, that ought to be a Lazy/Leaky/pointer, sorry.
> 
> We've never had any enforcement of that for Windows unfortunately. I wrote a
DIA
> tool long ago in tools/win/static_initializers but around the same time as I
> wrote it we switched from a bot step to a slightly more magical perf bot
> tracker, and it was never integrated into that dashboard.
> 
> (The short version is ... we had quite a lot of static initializers on Windows
> last time I checked.)

Thanks for the pointer to that tool. I updated it so that it detects this
global. Although, it still has to be run manually.

I'd open a bug to track cleaning this up (getting rid of global constructors and
destructors and adding monintoring) but we've got a few already: 637202, 341941,
and 94925 for starters.

Anyway, not an emergency. It would be good to get this cleaned up, but not
critical.

[brettw, 2017-08-24 23:35:45]

brettw@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2017-08-24 23:35:46]

https://codereview.chromium.org/2638433002/diff/160001/components/crash/conte...
File components/crash/content/app/BUILD.gn (right):

https://codereview.chromium.org/2638433002/diff/160001/components/crash/conte...
components/crash/content/app/BUILD.gn:81:
"//chrome/install_static:install_static_util",
This is a layering violation. Can you please fix it? I filed
https://bugs.chromium.org/p/chromium/issues/detail?id=758759 to track.

Thanks!

[Sigurður Ásgeirsson, 2017-08-25 20:20:55]

https://codereview.chromium.org/2638433002/diff/160001/components/crash/conte...
File components/crash/content/app/BUILD.gn (right):

https://codereview.chromium.org/2638433002/diff/160001/components/crash/conte...
components/crash/content/app/BUILD.gn:81:
"//chrome/install_static:install_static_util",
On 2017/08/24 23:35:46, brettw wrote:
> This is a layering violation. Can you please fix it? I filed
> https://bugs.chromium.org/p/chromium/issues/detail?id=758759 to track.
> 
> Thanks!

Done.

https://codereview.chromium.org/2638433002/diff/160001/components/crash/conte...
File components/crash/content/app/fallback_crash_handling_win.cc (right):

https://codereview.chromium.org/2638433002/diff/160001/components/crash/conte...
components/crash/content/app/fallback_crash_handling_win.cc:27:
std::unique_ptr<FallbackCrashHandlerLauncher> g_fallback_crash_handler_launcher;
On 2017/01/26 19:43:21, brucedawson wrote:
> Why are we using a global variable with a constructor and destructor here?
> Doesn't this violate the style guide?
> 
> I discovered this by stepping through all of the global initializers called by
> {,,chrome.exe}_initterm

Done.