803254 - Marionette should detect when a gecko/emulator instance it has launched has crashed

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

Currently, the emulator is subject to a crash while running telephony tests.  When this occurs, it is hard to tell from Marionette's log that this is what happened.

If Marionette has launched a gecko or emulator instance, it should check if this instance is alive after running each test.  If it isn't, it should output an appropriate failure and skip the rest of the tests, rather than emitting an error for each of them like "Connection refused".

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: Detect emulator crashes,

This detects when an emulator has crashed.  It doesn't detect when a Firefox instance has crashed - that will be more work to get right across OS's, and we may have to tweak mozprocess to get it to work.  So even though we want to do that eventually, I'd like to punt on that for now, and just fix the emulator case that we need for TBPL work.

Comment 2

[Andrew Halberstadt [:ahal]]

Comment on attachment 673009 [details] [diff] [review]
Detect emulator crashes,

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

I guess this ok, but shouldn't this logic live in emulator.py? Seems like anything that uses emulators might want to know if it crashed.

r+ anyway because the check_for_process method doesn't really do anything groundbreaking. Please address the nit though.

::: testing/marionette/client/marionette/runtests.py
@@ +331,5 @@
>          testgroup.submit()
>  
> +    def check_for_process(self):
> +        if (self.marionette.emulator and self.marionette.emulator.proc and
> +                not self.marionette.emulator.is_running):

nit: This is confusing, since if the emulator or proc objects are None, check_for_process returns True, which would make the user think that the process does exist. Changing the name of the function is probably good enough.

Comment 3

[Jonathan Griffin (:jgriffin)]

(In reply to Andrew Halberstadt [:ahal] from comment #2)
> Comment on attachment 673009 [details] [diff] [review]
> Detect emulator crashes,
> 
> Review of attachment 673009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess this ok, but shouldn't this logic live in emulator.py? Seems like
> anything that uses emulators might want to know if it crashed.
> 
> r+ anyway because the check_for_process method doesn't really do anything
> groundbreaking. Please address the nit though.
> 
> ::: testing/marionette/client/marionette/runtests.py
> @@ +331,5 @@
> >          testgroup.submit()
> >  
> > +    def check_for_process(self):
> > +        if (self.marionette.emulator and self.marionette.emulator.proc and
> > +                not self.marionette.emulator.is_running):
> 
> nit: This is confusing, since if the emulator or proc objects are None,
> check_for_process returns True, which would make the user think that the
> process does exist. Changing the name of the function is probably good
> enough.

What would you suggest instead?  The tricky part is that you can run tests on an emulator you've launched yourself, in which case Marionette cannot determine if the emulator is alive or not (since it doesn't know its pid).  In that case, emulator.proc is None.

I guess we could reverse things to do something like:

def check_if_crashed(self):
    # logic

which would always return False in the case of a manually-launched emulator, i.e., when emulator.proc is None.  Does that make more sense?

Comment 4

[Jonathan Griffin (:jgriffin)]

Attachment: Detect emulator crashes,

Ok, I think this is a cleaner solution

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 674027 [details] [diff] [review]
Detect emulator crashes,

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

r+ on the condition that this gets added to the emulator.py in mozdevice as well since marionette is supposed to be using that in the near future.

Comment 6

[Jonathan Griffin (:jgriffin)]

Attachment: Detect emulator crashes,

Updated with review comments, pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2efc963747d6&noignore=1

Comment 7

[Jonathan Griffin (:jgriffin)]

Comment on attachment 674430 [details] [diff] [review]
Detect emulator crashes,

Carrying r+ forward

Comment 8

[Jonathan Griffin (:jgriffin)]

Try run was green, landed as https://hg.mozilla.org/mozilla-central/rev/b1e54bf5bd6f

Comment 9

[Jonathan Griffin (:jgriffin)]

Landed on ash as aurora-try: https://tbpl.mozilla.org/?tree=Ash&rev=0f8d21413c86

Comment 10

[Jonathan Griffin (:jgriffin)]

Attachment: patch for ash

A modified patch that I'm landing on ash, since the original didn't apply cleanly:  https://hg.mozilla.org/projects/ash/rev/6d4dff1f9b3e

Comment 11

[Jonathan Griffin (:jgriffin)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/d45c28a32e26

Comment 12

[William Lachance (:wlach)]

Retroactively pushed the mozdevice portion of this patch to mozbase:

https://github.com/mozilla/mozbase/commit/b9ccccacd903b7fbc1a52a3d3d49d20bc6b61a3a