add mozcrash.kill_and_get_minidump

RESOLVED FIXED

Status

Testing
Mozbase
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: ted, Assigned: chmanchester)

Tracking

(Blocks: 1 bug, {sheriffing-P1})

unspecified
x86_64
Windows 7
sheriffing-P1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 770985 [details] [diff] [review]
add mozcrash.kill_and_get_minidump

I've been trying to find a root cause for bug 874647, and I had this code laying around somewhere, so I integrated it into a new mozcrash method. It's kind of crazy and I didn't write any tests for it (because they're probably going to be a PITA), but it works.
Comment on attachment 770985 [details] [diff] [review]
add mozcrash.kill_and_get_minidump

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

drive-by comment on the docstring style, since that tends to be missed by other reviewers...

::: mozcrash/mozcrash/mozcrash.py
@@ +231,5 @@
> +
> +    `pid` is the process ID to kill.
> +    `dump_directory` is the directory where a minidump should be written on
> +    Windows, where the dump will be written from outside the process.
> +

It would be better to use the :param: declaration here. See for example:

https://github.com/mozilla/mozbase/blob/master/mozdevice/mozdevice/devicemanager.py#L188
(Reporter)

Comment 2

4 years ago
Yeah, apparently that changed (or we just didn't know about it) since we did our doc sprint, so I was following file style. Thanks for reminding me!
(Reporter)

Updated

4 years ago
Duplicate of this bug: 811320

Updated

4 years ago
Blocks: 911249
Keywords: sheriffing-P1
What's the status here?
Flags: needinfo?(ted)
(Reporter)

Comment 5

4 years ago
This mostly just needs cleanup and polish. I need to make sure that it's not losing any features from the existing implementation.
Flags: needinfo?(ted)
(Reporter)

Comment 6

3 years ago
Created attachment 8443005 [details] [diff] [review]
Add mozcrash.kill_and_get_minidump

SLightly updated. I tested that this works on Try (I have separate patches to enable its use in the xpcshell harness and Mochitest).

Updated

3 years ago
Blocks: 1027802
(Reporter)

Updated

3 years ago
Attachment #770985 - Attachment is obsolete: true
(Reporter)

Comment 7

3 years ago
Ed: note that as currently written this patch doesn't do anything for Android tests.

Comment 8

3 years ago
Yeah but it was my understanding that we would expand kill_and_get_minidump() later?
(Assignee)

Comment 9

2 years ago
Here's a before[1] and after[2] for an infinite loop in content JS, and before[3] and after[4] for one of the hangs associated with bug 1032849. The former case is about the same, and the latter yields a lot more information, so if there aren't outstanding concerns with the approach I think we should take this. I'm testing a rebased patch plugged in to mochitests now and am planning to land this if everything looks ok.

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=8728227&repo=try
[2] https://treeherder.mozilla.org/logviewer.html#?job_id=8749615&repo=try
[3] https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-central&job_id=1617377
[4] https://treeherder.mozilla.org/logviewer.html#?job_id=8758029&repo=try
(Assignee)

Comment 10

2 years ago
Created attachment 8625158 [details]
MozReview Request: bug 890026 - Add mozcrash.kill_and_get_minidump

bug 890026 - Add mozcrash.kill_and_get_minidump
Attachment #8625158 - Flags: review?(ted)
(Assignee)

Comment 11

2 years ago
Created attachment 8625159 [details]
MozReview Request: Bug 890026 - Use kill_and_get_minidump in place of crashinject.exe in mochitest so we can stacks from more hangs.

Bug 890026 - Use kill_and_get_minidump in place of crashinject.exe in mochitest so we can stacks from more hangs.
Attachment #8625159 - Flags: review?(ted)
(Assignee)

Updated

2 years ago
Blocks: 1032849
(Reporter)

Comment 12

2 years ago
As much as I want to land these patches, I wrote them so I can't review them. I can't figure out how to reassign the reviews in ReviewBoard. I'm sure we can find someone else to review them, let's ask around today.
(Assignee)

Comment 13

2 years ago
Comment on attachment 8625158 [details]
MozReview Request: bug 890026 - Add mozcrash.kill_and_get_minidump

bug 890026 - Add mozcrash.kill_and_get_minidump
Attachment #8625158 - Flags: review?(ted) → review?(jmathies)

Comment 14

2 years ago
https://reviewboard.mozilla.org/r/11787/#review11135

::: testing/mozbase/mozcrash/mozcrash/mozcrash.py:376
(Diff revision 1)
> +        if not isinstance(file_name, unicode):

I'm not sure what isinstance does so a comment here explaining what this check / change is would be useful.

::: testing/mozbase/mozcrash/mozcrash/mozcrash.py:380
(Diff revision 1)
> +                                           FILE_SHARE_READ | FILE_SHARE_WRITE,

Why do we need FILE_SHARE_READ | FILE_SHARE_WRITE?

::: testing/mozbase/mozcrash/mozcrash/mozcrash.py:353
(Diff revision 1)
> +    def write_minidump(pid, dump_directory):

Should this return a result indicating success or failure?

::: testing/mozbase/mozcrash/mozcrash/mozcrash.py:386
(Diff revision 1)
> +        if file_handle:

success here is a value > 0, failure is  INVALID_HANDLE_VALUE, which is -1. I don't think your check here tests for failure correctly.

Updated

2 years ago
Attachment #8625158 - Flags: review?(jmathies) → review-
(Assignee)

Comment 15

2 years ago
https://reviewboard.mozilla.org/r/11787/#review11169

::: testing/mozbase/mozcrash/mozcrash/mozcrash.py:353
(Diff revision 1)
> +    def write_minidump(pid, dump_directory):

I don't think the result would be significant to any of the current callers - check_for_crashes is called after this, and will handle any minidump files we write here.

::: testing/mozbase/mozcrash/mozcrash/mozcrash.py:380
(Diff revision 1)
> +                                           FILE_SHARE_READ | FILE_SHARE_WRITE,

I don't think we do.
(Assignee)

Comment 16

2 years ago
Comment on attachment 8625158 [details]
MozReview Request: bug 890026 - Add mozcrash.kill_and_get_minidump

bug 890026 - Add mozcrash.kill_and_get_minidump
Attachment #8625158 - Flags: review- → review?(jmathies)
(Assignee)

Comment 17

2 years ago
Comment on attachment 8625159 [details]
MozReview Request: Bug 890026 - Use kill_and_get_minidump in place of crashinject.exe in mochitest so we can stacks from more hangs.

Bug 890026 - Use kill_and_get_minidump in place of crashinject.exe in mochitest so we can stacks from more hangs.

Updated

2 years ago
Attachment #8625158 - Flags: review?(jmathies) → review+

Comment 18

2 years ago
Comment on attachment 8625158 [details]
MozReview Request: bug 890026 - Add mozcrash.kill_and_get_minidump

https://reviewboard.mozilla.org/r/11789/#review11187

Ship It!

Comment 19

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/fced61beb43e
(Assignee)

Updated

2 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/fced61beb43e
(Reporter)

Comment 21

2 years ago
Comment on attachment 8625159 [details]
MozReview Request: Bug 890026 - Use kill_and_get_minidump in place of crashinject.exe in mochitest so we can stacks from more hangs.

https://reviewboard.mozilla.org/r/11791/#review11997

Ship It!
Attachment #8625159 - Flags: review?(ted) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 22

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/613f8e602f8b
https://hg.mozilla.org/mozilla-central/rev/613f8e602f8b
(Assignee)

Comment 24

2 years ago
Created attachment 8635465 [details] [diff] [review]
Fixup to import signal in mozcrash

Missing import causing problems on mac and linux.
Attachment #8635465 - Flags: review?(ahalberstadt)
(Assignee)

Updated

2 years ago
Assignee: ted → cmanchester
Status: NEW → ASSIGNED
Attachment #8635465 - Flags: review?(ahalberstadt) → review+

Comment 25

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6910c867d38
https://hg.mozilla.org/mozilla-central/rev/b6910c867d38
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox42: --- → fixed
Keywords: leave-open
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1032849
(Reporter)

Updated

2 years ago
Blocks: 1193738
(Assignee)

Updated

2 years ago
Depends on: 1194935

Updated

2 years ago
Blocks: 1185602
(Reporter)

Updated

10 months ago
Blocks: 1311429
You need to log in before you can comment on or make changes to this bug.