932349 - Generate useful crash stacks from mochitest hangs/timeouts

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 1

[(not currently active) Ted Mielczarek]

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.

Comment 2

[Jonathan Griffin (:jgriffin)]

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.

Comment 3

[(not currently active) Ted Mielczarek]

Yeah, it's this:
https://github.com/mozilla/mozbase/blob/master/mozprocess/mozprocess/processhandler.py#L709

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: Add a kill_on_timeout parameter to ProcessHandlerMixin

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.

Comment 5

[Andrew Halberstadt [:ahal]]

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

Comment 6

[(not currently active) Ted Mielczarek]

https://github.com/mozilla/mozbase/commit/896c4a0147f2d7220a10fe54db8fdcf52e6151b1

Need to cherry-pick this to m-i and fix Mochitest to use it.

Comment 7

[(not currently active) Ted Mielczarek]

Attachment: slightly rework how kill_on_timeout=False works

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.

Comment 8

[(not currently active) Ted Mielczarek]

Attachment: make Mochitest use kill_on_timeout=False and an onTimeout handler

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.

Comment 9

[Andrew Halberstadt [:ahal]]

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.)"

Comment 10

[Jeff Hammel]

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?

Comment 11

[(not currently active) Ted Mielczarek]

Second mozprocess change:
https://github.com/mozilla/mozbase/commit/584705824e033b4d058d3e61ee2fff2a7106fbaa

Yeah, or just cherry-picking those two mozprocess commits.

Comment 12

[(not currently active) Ted Mielczarek]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e76f1912071
https://hg.mozilla.org/mozilla-central/rev/b6b291d68a7f