Closed
Bug 932349
Opened 11 years ago
Closed 11 years ago
Generate useful crash stacks from mochitest hangs/timeouts
Categories
(Testing :: Mozbase, defect)
Testing
Mozbase
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla28
People
(Reporter: RyanVM, Assigned: ted)
References
Details
(Keywords: sheriffing-P1)
Attachments
(3 files)
(deleted),
patch
|
ahal
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
ahal
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
k0scist
:
review+
|
Details | Diff | Splinter Review |
In bug 921635, we ran into problems diagnosing the cause of the timeouts due to lack of any stacks when the process was killed. Until we can get usable stacks to point developers in the right direction, the tests will not be able to be re-enabled.
Assignee | ||
Comment 1•11 years ago
|
||
Is this happening on all platforms or just OS X?
I still suspect the lack of stacks is fallout from bug 746243, with mozprocess or mozrunner somehow killing the process before we get a chance to try to get a stack.
Updated•11 years ago
|
Keywords: sheriffing-P1
Comment 2•11 years ago
|
||
You're probably right. If a test times out at the harness level (so that it's mozprocess that handles the timeout), then mozprocess will kill the process when the timeout occurs.
We probably need to add a no_kill parameter to mozprocess/mozrunner.
Assignee | ||
Comment 3•11 years ago
|
||
Yeah, it's this:
https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/processhandler.py#L709
Assignee: nobody → ted
Component: Mochitest → Mozbase
QA Contact: hskupin
Assignee | ||
Comment 4•11 years ago
|
||
This adds a kill_on_timeout parameter to ProcessHandlerMixin, which defaults to True. If you set it to False then the process won't be killed on timeout, and .wait() won't call proc.wait() if the process has timed out. All the existing tests pass, and the new test I added passes.
This should be sufficient to make Mochitest do the right thing.
Attachment #824827 -
Flags: review?(ahalberstadt)
Comment 5•11 years ago
|
||
Comment on attachment 824827 [details] [diff] [review]
Add a kill_on_timeout parameter to ProcessHandlerMixin
Review of attachment 824827 [details] [diff] [review]:
-----------------------------------------------------------------
Lgtm
::: mozprocess/mozprocess/processhandler.py
@@ +742,5 @@
> count += 1
> if timeout and count > timeout:
> return
> + if self.didTimeout and not self._kill_on_timeout:
> + return None
nit: drop the None for consistency
Attachment #824827 -
Flags: review?(ahalberstadt) → review+
Assignee | ||
Comment 6•11 years ago
|
||
https://github.com/mozilla/mozbase/commit/896c4a0147f2d7220a10fe54db8fdcf52e6151b1
Need to cherry-pick this to m-i and fix Mochitest to use it.
Assignee | ||
Comment 7•11 years ago
|
||
After testing integration of this into Mochitest, I realized that what I currently had wasn't fantastic, since you still need to wait on the process after you kill it. This patch removes the block I put in wait() and changes the ProcessHandlerMixin docs to note that if you want to kill the process yourself you should do so in an onTimeout handler. I changed the test to reflect this as well.
Attachment #825984 -
Flags: review?(ahalberstadt)
Assignee | ||
Comment 8•11 years ago
|
||
This wires up support for the new kill_on_timeout=False in Mochitest. It also moves the timeout handling into an onTimeout handler, which seems appropriate anyway. This works fine in my local testing, I get stacks from hangs.
Attachment #825993 -
Flags: review?(jhammel)
Comment 9•11 years ago
|
||
Comment on attachment 825984 [details] [diff] [review]
slightly rework how kill_on_timeout=False works
Review of attachment 825984 [details] [diff] [review]:
-----------------------------------------------------------------
::: mozprocess/mozprocess/processhandler.py
@@ +36,4 @@
> :param cwd: working directory for command (defaults to None).
> :param env: is the environment to use for the process (defaults to os.environ).
> :param ignore_children: causes system to ignore child processes when True, defaults to False (which tracks child processes).
> + :param kill_on_timeout: when True, the process will be killed when a timeout is reached. When False, the caller is responsible for killing the process. (Defaults to True.) Note that you may need to kill the process in an onTimeout handler if you expect calling wait() with no timeout to return when a run() timeout is hit.
I think the last part of this is hard to parse. What about something like:
"when True, the process will be killed when a timeout is reached. When False, the caller is responsible for killing the process. Failure to do so could cause a call to wait() to hang indefinitely (Defaults to True.)"
Attachment #825984 -
Flags: review?(ahalberstadt) → review+
Comment 10•11 years ago
|
||
Comment on attachment 825993 [details] [diff] [review]
make Mochitest use kill_on_timeout=False and an onTimeout handler
Review of attachment 825993 [details] [diff] [review]:
-----------------------------------------------------------------
lgtm; is this contingent on a mozprocess mirroring?
Attachment #825993 -
Flags: review?(jhammel) → review+
Assignee | ||
Comment 11•11 years ago
|
||
Second mozprocess change:
https://github.com/mozilla/mozbase/commit/584705824e033b4d058d3e61ee2fff2a7106fbaa
Yeah, or just cherry-picking those two mozprocess commits.
Assignee | ||
Comment 12•11 years ago
|
||
I pushed this to try and it looked good:
https://tbpl.mozilla.org/?tree=Try&rev=0b35d34288ee
I cherry-picked those two commits (squashed into one):
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7e76f1912071
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b6b291d68a7f
Reporter | ||
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7e76f1912071
https://hg.mozilla.org/mozilla-central/rev/b6b291d68a7f
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in
before you can comment on or make changes to this bug.
Description
•