890026 - add mozcrash.kill_and_get_minidump

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[(not currently active) Ted Mielczarek]

Attachment: 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 1

[William Lachance (:wlach)]

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

Comment 2

[(not currently active) Ted Mielczarek]

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!

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

What's the status here?

Comment 5

[(not currently active) Ted Mielczarek]

This mostly just needs cleanup and polish. I need to make sure that it's not losing any features from the existing implementation.

Comment 6

[(not currently active) Ted Mielczarek]

Attachment: 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).

Comment 7

[(not currently active) Ted Mielczarek]

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

Comment 8

[Ed Morley [:emorley]]

Yeah but it was my understanding that we would expand kill_and_get_minidump() later?

Comment 9

[Chris Manchester (limited bugmail, email directly)]

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

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bug 890026 - Add mozcrash.kill_and_get_minidump

bug 890026 - Add mozcrash.kill_and_get_minidump

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Attachment: 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.

Comment 12

[(not currently active) Ted Mielczarek]

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.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

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

bug 890026 - Add mozcrash.kill_and_get_minidump

Comment 14

[Jim Mathies [:jimm]]

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.

Comment 15

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 16

[Chris Manchester (limited bugmail, email directly)]

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

bug 890026 - Add mozcrash.kill_and_get_minidump

Comment 17

[Chris Manchester (limited bugmail, email directly)]

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.

Comment 18

[Jim Mathies [:jimm]]

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

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fced61beb43e

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fced61beb43e

Comment 21

[(not currently active) Ted Mielczarek]

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!

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/613f8e602f8b

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/613f8e602f8b

Comment 24

[Chris Manchester (limited bugmail, email directly)]

Attachment: Fixup to import signal in mozcrash

Missing import causing problems on mac and linux.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6910c867d38

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b6910c867d38