865944 - Use foreground activity instead of process name to determine if remote browser has terminated

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Geoff Brown [:gbrown]]

Bug 865311 discusses problems associated with using the process name for determining if a launched process -- typically the browser under test -- has completed.

This bug focuses on using the Android foreground activity instead of the process name as a possible solution.

Comment 1

[Geoff Brown [:gbrown]]

Attachment: very rough wip - sutagent

Just something to discuss...

Comment 2

[Geoff Brown [:gbrown]]

Attachment: very rough wip - devicemanager and automation

Ditto!

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 742132 [details] [diff] [review]
very rough wip - sutagent

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

while I don't know the android system calls that well, this is generally a good approach!

::: build/mobile/sutagent/android/DoCommand.java
@@ +177,5 @@
>          TZGET ("tzget"),
>          TZSET ("tzset"),
>          ADB ("adb"),
>          CHMOD ("chmod"),
> +        FOREGROUNDACTIVITY ("foregroundactivity"),

I would love to go back to our 4 character limit imposed by blassey in the old days of sutagent :)

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 742133 [details] [diff] [review]
very rough wip - devicemanager and automation

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

nice, this might be a simple fix.  If we agree on something here, we can get a sutagent upgrade followed by a talos update also ;)  Could we hack this for now without the sutagent to prove this helps?  i.e. use dumpsys window input method?  I agree for a final solution something integrated into sutagent would be ideal.

Comment 5

[Dan Minor [:dminor]]

This approach makes sense to me.

Comment 6

[Geoff Brown [:gbrown]]

Attachment: (1) add "activity" command to sutagent; bump version

Adds the "activity" command to sutagent, which returns the package name of the top / foreground activity. 

Bumped the sutagent version from 1.16 to 1.17.

Tested locally on panda and tegra. This is from a panda:

$ telnet 192.168.0.111 20701
Trying 192.168.0.111...
Connected to 192.168.0.111.
Escape character is '^]'.
$>ver
SUTAgentAndroid Version 1.17
$>activity
com.mozilla.SUTAgentAndroid
$>quit
quit
$>Connection closed by foreign host.
$ adb shell am start -a android.activity.MAIN -n org.mozilla.fennec_mozdev/.App
Starting: Intent { act=android.activity.MAIN cmp=org.mozilla.fennec_mozdev/.App }
$ telnet 192.168.0.111 20701
Trying 192.168.0.111...
Connected to 192.168.0.111.
Escape character is '^]'.
$>activity
org.mozilla.fennec_mozdev
$>quit
quit
$>Connection closed by foreign host.
$ adb shell am force-stop org.mozilla.fennec_mozdev
$ telnet 192.168.0.111 20701
Trying 192.168.0.111...
Connected to 192.168.0.111.
Escape character is '^]'.
$>activity
com.mozilla.SUTAgentAndroid
$>

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 742458 [details] [diff] [review]
(1) add "activity" command to sutagent; bump version

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

this looks great.  this can be landed as a DONTBUILD.  Lets put sutagent1.17.apk on a people account and file a bug for deployment.

::: build/mobile/sutagent/android/DoCommand.java
@@ +3856,5 @@
> +        List< ActivityManager.RunningTaskInfo > taskInfo = null;
> +        ComponentName componentInfo = null;
> +        if (aMgr != null)
> +            {
> +            taskInfo = aMgr.getRunningTasks(1); 

nit: trailing whitespace

Comment 8

[Geoff Brown [:gbrown]]

sut changes: https://hg.mozilla.org/integration/mozilla-inbound/rev/ec5be6e1bf45

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[William Lachance (:wlach)]

Comment on attachment 742133 [details] [diff] [review]
very rough wip - devicemanager and automation

Drive by comment: I'm a bit concerned that this is adding something very Android-specific into the devicemanager code, which we now also use for testing FirefoxOS. Over the last few months I've put quite a bit of effort into documenting mozdevice and clearly seperating the Android functionality from that which is universal to all types of devices (see e.g. https://mozbase.readthedocs.org/en/latest/mozdevice.html#android-extensions). I'm afraid to say that this patch, as is, kind of undoes that. :(

It's a little bit of extra effort (since the harness code will likely need to be updated as well), but I'd much prefer if this method were added into droid.py: 

https://github.com/mozilla/mozbase/blob/master/mozdevice/mozdevice/droid.py

Another alternative might be to reimplement the "foregroundactivity" command as an "info" subcommand in the agent. Then we wouldn't need to add any new code to mozdevice at all (just the harness). Maybe just follow the model of bug 838844 for getting the user serial.

Comment 11

[Geoff Brown [:gbrown]]

Attachment: (2) Makefile changes so that droid.py can be used from remote reftests and remote mochitests

:wlach -- I'll just flag you for feedback on this one patch, but I am interested in your feedback / co-review for all of the patches to come, if you have time.

Comment 12

[Geoff Brown [:gbrown]]

Attachment: (3) Avoid 30 second wait for process start in fireProcess when launching am.

This is mostly unrelated to activity-vs-process but affects the timing of our wait-for-launch / wait-for-completion, so wanted to include this here.

Comment 13

[Geoff Brown [:gbrown]]

Attachment: (4) Use droid.getTopActivity in remoteautomation.py

Comment 14

[Geoff Brown [:gbrown]]

Attachment: (5) Do not use adb run-as for dumpsys

When running DroidADB.getTopActivity, devicemanagerADB will try to execute dumpsys as org.mozilla.fennec. In my experience, this usually fails, whereas running as the normal shell user succeeds.

Comment 15

[William Lachance (:wlach)]

Comment on attachment 744712 [details] [diff] [review]
(2) Makefile changes so that droid.py can be used from remote reftests and remote mochitests

Looks like the right thing. Always happy to provide feedback. :)

Comment 16

[Geoff Brown [:gbrown]]

Attachment: (6) Add getTopActivity to droid

Comment 17

[Geoff Brown [:gbrown]]

Attachment: (7) Enable more robocop tests

These tests run well on tegra and panda with these patches. The other 2 disabled tests appear to have other issues.

Comment 18

[Geoff Brown [:gbrown]]

Try run with extra debugging: https://tbpl.mozilla.org/?tree=Try&rev=b5c7deee3b3f
Try run with cleaned up patches: https://tbpl.mozilla.org/?tree=Try&rev=10cc0ef4c1f3

Comment 19

[Geoff Brown [:gbrown]]

Retries indicate there may be a (new, different?) problem in testFindInPage on panda -- I will follow-up.

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 744712 [details] [diff] [review]
(2) Makefile changes so that droid.py can be used from remote reftests and remote mochitests

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

great, it doesn't look like droid.py requires additional files.

Comment 21

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 744716 [details] [diff] [review]
(4) Use droid.getTopActivity in remoteautomation.py

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

nice

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 744719 [details] [diff] [review]
(5) Do not use adb run-as for dumpsys

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

::: testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py
@@ +540,5 @@
>              finalArgs.extend(['-s', self._deviceSerial])
>          # use run-as to execute commands as the package we're testing if
>          # possible
>          if not self._haveRootShell and self._useRunAs and \
> +                args[0] == "shell" and args[1] not in [ "run-as", "am", "dumpsys" ]:

this seems like a hack...can we document a bit more of this.  want to make it easy for future hackers to realize they can exclude commands (like dumpsys) and to ensure we understand why we do this.

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 744724 [details] [diff] [review]
(6) Add getTopActivity to droid

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

this will need to land on the github mozbase instance and then uplifted into mozilla-central.

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 744728 [details] [diff] [review]
(7) Enable more robocop tests

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

this gets us halfway towards running all tests!

Comment 25

[Geoff Brown [:gbrown]]

(In reply to Joel Maher (:jmaher) from comment #22)
> Comment on attachment 744719 [details] [diff] [review]
> (5) Do not use adb run-as for dumpsys
> 
> Review of attachment 744719 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py
> @@ +540,5 @@
> >              finalArgs.extend(['-s', self._deviceSerial])
> >          # use run-as to execute commands as the package we're testing if
> >          # possible
> >          if not self._haveRootShell and self._useRunAs and \
> > +                args[0] == "shell" and args[1] not in [ "run-as", "am", "dumpsys" ]:
> 
> this seems like a hack...can we document a bit more of this.  want to make
> it easy for future hackers to realize they can exclude commands (like
> dumpsys) and to ensure we understand why we do this.

:wlach -- any thoughts here? I find myself wondering if all the _useRunAs code might be better off in Droid.

Comment 26

[William Lachance (:wlach)]

(In reply to Geoff Brown [:gbrown] from comment #25)
> (In reply to Joel Maher (:jmaher) from comment #22)
> > Comment on attachment 744719 [details] [diff] [review]
> > (5) Do not use adb run-as for dumpsys
> > 
> > Review of attachment 744719 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py
> > @@ +540,5 @@
> > >              finalArgs.extend(['-s', self._deviceSerial])
> > >          # use run-as to execute commands as the package we're testing if
> > >          # possible
> > >          if not self._haveRootShell and self._useRunAs and \
> > > +                args[0] == "shell" and args[1] not in [ "run-as", "am", "dumpsys" ]:
> > 
> > this seems like a hack...can we document a bit more of this.  want to make
> > it easy for future hackers to realize they can exclude commands (like
> > dumpsys) and to ensure we understand why we do this.
> 
> :wlach -- any thoughts here? I find myself wondering if all the _useRunAs
> code might be better off in Droid.

Just use shellCheckOutput as opposed to ._runAs, that should avoid this issue altogether. :)

I think transplanting run-as into droid would be rather involved, not sure. Personally I'm not really convinced that run-as has really proven to help our testing efforts, but that's a discussion we can have another day. :)

Comment 27

[William Lachance (:wlach)]

> Just use shellCheckOutput as opposed to ._runAs, that should avoid this
> issue altogether. :)

Sorry, I meant "_runCmd", not "_runAs".

Comment 28

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #19)
> Retries indicate there may be a (new, different?) problem in testFindInPage
> on panda -- I will follow-up.

That's a pre-existing condition:
https://tbpl.mozilla.org/php/getParsedLog.php?id=22506130&tree=Mozilla-Inbound&full=1#error0
https://tbpl.mozilla.org/php/getParsedLog.php?id=22507286&tree=Mozilla-Inbound&full=1#error7
https://tbpl.mozilla.org/php/getParsedLog.php?id=22508644&tree=Mozilla-Inbound&full=1#error7

I won't try to address it in this bug.

Comment 29

[Geoff Brown [:gbrown]]

(In reply to William Lachance (:wlach) from comment #26) 
> Just use shellCheckOutput as opposed to ._runAs, that should avoid this
> issue altogether. :)

That works just fine -- good idea!

Comment 30

[Geoff Brown [:gbrown]]

Attachment: (6) Add getTopActivity to droid

Updated to use shellCheckOutput -- eliminates need for hack-ish patch (5).

Comment 31

[Geoff Brown [:gbrown]]

(In reply to Joel Maher (:jmaher) from comment #23)
> Comment on attachment 744724 [details] [diff] [review]
> (6) Add getTopActivity to droid
> 
> Review of attachment 744724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this will need to land on the github mozbase instance and then uplifted into
> mozilla-central.

Can I get a hand with this? I think patches (3) and (6) need to land on github.

Comment 32

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 744944 [details] [diff] [review]
(6) Add getTopActivity to droid

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

good stuff!

Comment 33

[William Lachance (:wlach)]

(In reply to Geoff Brown [:gbrown] from comment #31)
> (In reply to Joel Maher (:jmaher) from comment #23)
> > Comment on attachment 744724 [details] [diff] [review]
> > (6) Add getTopActivity to droid
> > 
> > Review of attachment 744724 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > this will need to land on the github mozbase instance and then uplifted into
> > mozilla-central.
> 
> Can I get a hand with this? I think patches (3) and (6) need to land on
> github.

Landed them on github:

https://github.com/mozilla/mozbase/commit/12baea63d19070ba5664a228b179baedcef779bf
https://github.com/mozilla/mozbase/commit/0c933d1867f58d2604d681b0ff0c77a45bb438ea

Comment 34

[Geoff Brown [:gbrown]]

Patch (2) https://hg.mozilla.org/integration/mozilla-inbound/rev/29a70c8e22cc
Patch (3) https://hg.mozilla.org/integration/mozilla-inbound/rev/87ef7a0e5f94
Patch (4) https://hg.mozilla.org/integration/mozilla-inbound/rev/d1651293c9d2
Patch (6) https://hg.mozilla.org/integration/mozilla-inbound/rev/d2688e330aa0

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/29a70c8e22cc
https://hg.mozilla.org/mozilla-central/rev/87ef7a0e5f94
https://hg.mozilla.org/mozilla-central/rev/d1651293c9d2
https://hg.mozilla.org/mozilla-central/rev/d2688e330aa0

Comment 36

[Geoff Brown [:gbrown]]

Patch (7) https://hg.mozilla.org/integration/mozilla-inbound/rev/548558263c23

(...but I kept testDoorHanger disabled -- see bug 868681.)

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/548558263c23

Comment 38

[Geoff Brown [:gbrown]]

Those patches seem to have significantly reduced the "2400 seconds without output" failures. We should also port these changes to talos, in case there are similar problems there. Consider, for instance, bug 849478.

Comment 39

[William Lachance (:wlach)]

There were some formatting errors in the getTopActivity patch that we committed to mozbase (5 space instead of 5 space indentation). Fixed here:

https://github.com/mozilla/mozbase/commit/78c450cd0ed1243376749adc31ff347db9255681

Comment 40

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: talos to use getTopActivity (WIP)

just a patch of what I think would need to be done.  Unfortunately when I ran with this, I continued to get DroidSUT error, getTopActivity() is not defined :(

Comment 41

[Nick Alexander :nalexander [he/him]]

The sequence of patches with the log information [1] break |make mochitest-remote| on all of my local devices.  Bisecting shows that running mochitests (via runtestsremote.py) succeeds for the Nightly build at

http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2013/05/2013-05-03-03-09-20-mozilla-central-android/

and fails for the build at

http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2013/05/2013-05-04-03-09-13-mozilla-central-android/

Testing a more recent branch with the four commits reverted allows the mochitests to run.

I will try to figure out what is happening locally and suggest a fix, but in the meantime, if I could get independent verification of my claims that would be great.  (I have verified that invoking via |make mochitest-remote| versus runtestsremote.py directly is not the issue.)

[1]  Candidate changes:

changeset:   130750:29a70c8e22cc
user:        Geoff Brown <[e-mail]>
date:        Fri May 03 11:37:51 2013 -0600
summary:     Bug 865944 - Add droid.py to list of files used by remote reftests and mochitests; r=jmaher

changeset:   130751:87ef7a0e5f94
user:        Geoff Brown <[e-mail]>
date:        Fri May 03 11:37:57 2013 -0600
summary:     Bug 865944 - Do not wait for "am instrument" to start; r=jmaher

changeset:   130752:d1651293c9d2
user:        Geoff Brown <[e-mail]>
date:        Fri May 03 11:37:59 2013 -0600
summary:     Bug 865944 - Use top activity instead of process to check launch success; r=jmaher

changeset:   130753:d2688e330aa0
user:        Geoff Brown <[e-mail]>
date:        Fri May 03 11:38:01 2013 -0600
summary:     Bug 865944 - Add getTopActivity to droid; r=jmaher

Comment 42

[Geoff Brown [:gbrown]]

Are you using adb or sut?

I can check on this first thing tomorrow.

Comment 43

[Nick Alexander :nalexander [he/him]]

(In reply to Geoff Brown [:gbrown] from comment #42)
> Are you using adb or sut?

adb.

> I can check on this first thing tomorrow.

Now that I have a work-around, it's not urgent.  We just need to figure out the right thing for local and buildbot use.  Do /any/ of the buildbots use adb?  It is my understanding that none do.

Comment 44

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: updated patch for android_foreground (0.9)

Will update this once I finish testing on try server.  This worked in my initial tests.  Also I have some debugging for tspaint in here.

Comment 45

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8361218 [details] [diff] [review]
updated patch for android_foreground (0.9)

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

try server is pretty green, I feel comfortable about landing this:
https://tbpl.mozilla.org/?tree=Try&rev=b757e8339944

Comment 46

[Dan Minor [:dminor]]

Comment on attachment 8361218 [details] [diff] [review]
updated patch for android_foreground (0.9)

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

For the most part looks good, but you need to fix the hard coded activity name "org.mozilla.fennec".

::: talos/ffprocess_remote.py
@@ +89,3 @@
>          processes_with_names = []
>          for process_name in process_names:
> +            print self.testAgent

nit: I assume this was for debugging and can be removed.

@@ +152,4 @@
>                  total_time = 0
>                  while total_time < timeout:
>                      time.sleep(5)
> +#                    if not self.testAgent.processExist(cmd):

nit: remove commented line

@@ +153,5 @@
>                  while total_time < timeout:
>                      time.sleep(5)
> +#                    if not self.testAgent.processExist(cmd):
> +#TODO: make this a real processname, how do we get that?
> +                    if not self.testAgent.getTopActivity() == "org.mozilla.fennec":

This won't work with a local build where the activity will be e.g. org.mozilla.fennec_dminor.

Comment 47

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: updated patch for android_foreground (1.1)

updated and tested on try.

Comment 48

[Geoff Brown [:gbrown]]

Comment on attachment 8366752 [details] [diff] [review]
updated patch for android_foreground (1.1)

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

::: talos/ffprocess_remote.py
@@ +89,3 @@
>          processes_with_names = []
>          for process_name in process_names:
> +            if self.testAgent.getTopActivity() == process_name:

I suggest pulling the call to getTopActivity() out of the loop -- faster, less SUT traffic, etc.

Comment 49

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: updated patch for android_foreground (1.2)

updated as per gbrown's comment!

Comment 50

[Dan Minor [:dminor]]

Comment on attachment 8366752 [details] [diff] [review]
updated patch for android_foreground (1.1)

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

Looks good to me.

::: talos/ffprocess_remote.py
@@ +89,4 @@
>          processes_with_names = []
>          for process_name in process_names:
> +            if self.testAgent.getTopActivity() == process_name:
> +                processes_with_names.append((-1, process_name))

Is ProcessesWithNames still used anywhere or can it just be removed?
I couldn't find it called from inside talos using grep.

If it is still required, I think you should have a comment or something in the docstring that makes it clear than for the remote case you are looking at activities and not processes.

Comment 51

[Dan Minor [:dminor]]

Comment on attachment 8366782 [details] [diff] [review]
updated patch for android_foreground (1.2)

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

See comment on earlier version of patch.

Comment 52

[Joel Maher ( :jmaher ) (UTC -8)]

ProcessesWithNames is a function that is called from ffprocess to checkallprocesses:
http://hg.mozilla.org/build/talos/file/tip/talos/ffprocess.py#l51

I will add a comment.

Comment 53

[Joel Maher ( :jmaher ) (UTC -8)]

landed in talos:
https://hg.mozilla.org/build/talos/rev/78d138bbb33a

this bug is marked as leaveopen, is there a reason forthat still?

Comment 54

[Geoff Brown [:gbrown]]

(In reply to Joel Maher (:jmaher) from comment #53)
> this bug is marked as leaveopen, is there a reason forthat still?

We are almost done, but there is one last issue I am mulling over -- best to keep it open still.

Comment 55

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #54) 
> We are almost done, but there is one last issue I am mulling over -- best to
> keep it open still.

I was thinking that I would like to change the way sutagent's exec works, especially as it relates to robocop. Currently exec waits for up to 5 minutes for native processes (Robocop/am instrument...), but returns immediately for Java apps (mochitest/reftest/org.mozilla.fennec). So if waitForFinish breaks for Robocop (as it has a few times), it only causes a problem in the rare case that the test runs for more than 5 minutes. 

I tried changing the exec wait behavior for native processes and experienced some test failures, which seemed possibly related to the activity-waiting behavior of this bug, but I am not sure of the root cause. It's hard to justify investigating further given that everything seems to work fine now, so let's close this now.