Last Comment Bug 890026 - add mozcrash.kill_and_get_minidump
: add mozcrash.kill_and_get_minidump
Status: RESOLVED FIXED
: sheriffing-P1
Product: Testing
Classification: Components
Component: Mozbase (show other bugs)
: unspecified
: x86_64 Windows 7
-- normal (vote)
: ---
Assigned To: Chris Manchester (:chmanchester)
: Henrik Skupin (:whimboo) [away 02/18 - 02/27]
:
Mentors:
: 811320 1032849 (view as bug list)
Depends on: 1194935
Blocks: 1311429 911249 1027802 1032849 1185602 1193738
  Show dependency treegraph
 
Reported: 2013-07-03 13:10 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2016-10-19 09:21 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
add mozcrash.kill_and_get_minidump (4.24 KB, patch)
2013-07-03 13:10 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
Add mozcrash.kill_and_get_minidump (5.43 KB, patch)
2014-06-19 12:56 PDT, Ted Mielczarek [:ted.mielczarek]
no flags Details | Diff | Splinter Review
MozReview Request: bug 890026 - Add mozcrash.kill_and_get_minidump (40 bytes, text/x-review-board-request)
2015-06-21 09:59 PDT, Chris Manchester (:chmanchester)
jmathies: review+
Details | Review
MozReview Request: Bug 890026 - Use kill_and_get_minidump in place of crashinject.exe in mochitest so we can stacks from more hangs. (40 bytes, text/x-review-board-request)
2015-06-21 09:59 PDT, Chris Manchester (:chmanchester)
ted: review+
Details | Review
Fixup to import signal in mozcrash (775 bytes, patch)
2015-07-17 11:53 PDT, Chris Manchester (:chmanchester)
ahalberstadt: review+
Details | Diff | Splinter Review

Description User image Ted Mielczarek [:ted.mielczarek] 2013-07-03 13:10:01 PDT
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 1 User image William Lachance (:wlach) (use needinfo!) 2013-07-03 14:35:23 PDT
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 User image Ted Mielczarek [:ted.mielczarek] 2013-07-03 16:02:32 PDT
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 3 User image Ted Mielczarek [:ted.mielczarek] 2013-08-30 09:59:03 PDT
*** Bug 811320 has been marked as a duplicate of this bug. ***
Comment 4 User image :Ms2ger (⌚ UTC+1/+2) 2014-02-18 04:58:54 PST
What's the status here?
Comment 5 User image Ted Mielczarek [:ted.mielczarek] 2014-03-24 13:55:37 PDT
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 User image Ted Mielczarek [:ted.mielczarek] 2014-06-19 12:56:04 PDT
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).
Comment 7 User image Ted Mielczarek [:ted.mielczarek] 2014-06-20 08:32:55 PDT
Ed: note that as currently written this patch doesn't do anything for Android tests.
Comment 8 User image Ed Morley [:emorley] 2014-06-20 08:48:04 PDT
Yeah but it was my understanding that we would expand kill_and_get_minidump() later?
Comment 9 User image Chris Manchester (:chmanchester) 2015-06-20 09:51:03 PDT
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 User image Chris Manchester (:chmanchester) 2015-06-21 09:59:40 PDT
Created attachment 8625158 [details]
MozReview Request: bug 890026 - Add mozcrash.kill_and_get_minidump

bug 890026 - Add mozcrash.kill_and_get_minidump
Comment 11 User image Chris Manchester (:chmanchester) 2015-06-21 09:59:42 PDT
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.
Comment 12 User image Ted Mielczarek [:ted.mielczarek] 2015-07-01 03:56:53 PDT
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 User image Chris Manchester (:chmanchester) 2015-07-01 11:45:20 PDT
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 User image Jim Mathies [:jimm] 2015-07-06 08:30:56 PDT
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 User image Chris Manchester (:chmanchester) 2015-07-06 12:45:23 PDT
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 User image Chris Manchester (:chmanchester) 2015-07-06 13:29:21 PDT
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 User image Chris Manchester (:chmanchester) 2015-07-06 13:29:22 PDT
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 User image Jim Mathies [:jimm] 2015-07-06 13:47:43 PDT
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 20 User image Carsten Book [:Tomcat] 2015-07-09 09:00:07 PDT
https://hg.mozilla.org/mozilla-central/rev/fced61beb43e
Comment 21 User image Ted Mielczarek [:ted.mielczarek] 2015-07-15 11:28:40 PDT
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 23 User image Ryan VanderMeulen [:RyanVM] 2015-07-16 12:05:09 PDT
https://hg.mozilla.org/mozilla-central/rev/613f8e602f8b
Comment 24 User image Chris Manchester (:chmanchester) 2015-07-17 11:53:43 PDT
Created attachment 8635465 [details] [diff] [review]
Fixup to import signal in mozcrash

Missing import causing problems on mac and linux.
Comment 26 User image Wes Kocher (:KWierso) 2015-07-17 18:03:26 PDT
https://hg.mozilla.org/mozilla-central/rev/b6910c867d38
Comment 27 User image Chris Manchester (:chmanchester) 2015-07-22 12:21:39 PDT
*** Bug 1032849 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.