Add a Windows event that can be set to trigger Breakpad

RESOLVED WONTFIX

Status

()

Toolkit
Crash Reporting
RESOLVED WONTFIX
5 years ago
5 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
This is a WIP. It seems to work fine but probably needs some cleanup. Also: do we want to have this on always, or enable it via an environment variable only in automation?
(Assignee)

Comment 1

5 years ago
Created attachment 765524 [details] [diff] [review]
Add a Windows event that can be set to trigger Breakpad
(Assignee)

Comment 3

5 years ago
Created attachment 765915 [details] [diff] [review]
use Breakpad event to kill processes on Windows

Too lazy to file a new bug, so this patch replaces the use of crashinject in automation.py with this new event.
(Assignee)

Comment 4

5 years ago
Created attachment 765916 [details] [diff] [review]
remove crashinject

Bonus patch: rm the crashinject code and all references to it since we don't need it after these other patches.
Attachment #765916 - Flags: review?(gps)
(Assignee)

Updated

5 years ago
Blocks: 874647
(Assignee)

Comment 5

5 years ago
Created attachment 765925 [details] [diff] [review]
Add a Windows event that can be set to trigger Breakpad

Fixed the patch so it does cleanup on the handles it creates. Tagging ehsan for review since this is mostly Win32 API fiddling.

The head_crashreporter.js diff is a little weird, I just factored out a bit of the do_crash function into a separate function, but that seems to have really confused diff (it shows the contents of the new function as unchanged, and the rest of do_crash as changed).

This all seems to work great in my local testing, and I have a try push to see if I can get useful info out of it there.
Attachment #765925 - Flags: review?(ehsan)
(Assignee)

Updated

5 years ago
Attachment #765524 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
Comment on attachment 765915 [details] [diff] [review]
use Breakpad event to kill processes on Windows

The bulk of this patch is calling Win32 APIs via ctypes, so r?ehsan here as well.
Attachment #765915 - Flags: review?(ehsan)

Updated

5 years ago
Attachment #765925 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #765915 - Flags: review?(ehsan) → review+

Comment 7

5 years ago
Comment on attachment 765916 [details] [diff] [review]
remove crashinject

Review of attachment 765916 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure why you need my review on this. This is mostly a rubber stamp review.
Attachment #765916 - Flags: review?(gps) → review+
(Assignee)

Comment 8

5 years ago
Just generic "build peer review on touching stuff under build".
(Assignee)

Comment 9

5 years ago
Apparently something about these patches was not sufficient, because they failed to do anything useful on WinXP xpcshell hangs. I'm going to try to figure this out in my WinXP VM.
(Assignee)

Comment 10

5 years ago
These work fine locally in my WinXP VM, including with an artificially-induced hang in an xpcshell test. This leads me to believe that the root of those xpcshell hangs is something unusual.
(In reply to Ted Mielczarek [:ted.mielczarek] (away June 25th-30th) from comment #10)
> These work fine locally in my WinXP VM, including with an
> artificially-induced hang in an xpcshell test. This leads me to believe that
> the root of those xpcshell hangs is something unusual.

Have you tried your patch + artificially-induced hang hang on try? This will rule out that it works only on your VM or just doesn't work in buildbot for some reason.
(Assignee)

Comment 12

5 years ago
No, but that's a good idea, so I just pushed that to try:
https://tbpl.mozilla.org/?tree=Try&rev=ace0bb176685

I won't be around to check the results until next week, so if someone cares they can look at that. I added a hang to testing/xpcshell/example/unit/test_sample.js.
(Assignee)

Comment 13

5 years ago
That works fine even on WinXP, so this technique works fine in the general case:
https://tbpl.mozilla.org/php/getParsedLog.php?id=24531267&tree=Try

It must be that the hangs we're hitting are during shutdown after we've disabled the crash reporter.
(Assignee)

Comment 14

5 years ago
Given that this isn't helping with those WinXP hangs, I'm not sure I want to use this approach at all.

Comment 15

5 years ago
Can we just use a windbg script to collect a minidump of that process (externally)?
(Assignee)

Comment 16

5 years ago
I just wrote some Python to call MiniDumpWriteDump on it, it seems to work fine:
https://hg.mozilla.org/try/rev/978dc9973bd5

I have that in a Try push currently, we'll see how it goes.
(Assignee)

Updated

5 years ago
No longer blocks: 874647
(Assignee)

Comment 17

5 years ago
Turns out I don't want to do this. I'm going to take the patch on bug 890026 instead.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.