838844 - "am" command doesn't work under sutagent in jellybean

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[William Lachance (:wlach)]

This is because of the new multi-user support in this version of android. We need to do something similar to what kats did here:

https://bugzilla.mozilla.org/show_bug.cgi?id=811763

This is needed to get the galaxy nexus in hax0r working properly withe eideticker.

Comment 1

[cmtalbert]

I can also just flash it with a different version of android if that is easier.

Comment 2

[William Lachance (:wlach)]

(In reply to Clint Talbert ( :ctalbert ) from comment #1)
> I can also just flash it with a different version of android if that is
> easier.

We'll have to solve this sooner or later, might as well do it sooner...

Comment 3

[William Lachance (:wlach)]

Attachment: Add info option to get sutagent user info

In order to run "am" from sutagent on jellybean (and presumably later versions), we need to pass the --user parameter to am. This patch modifies sutagent so we can get the user serial number we need for that.

I'll attach a separate patch which modifies the appropriate logic in mozdevice

Comment 4

[William Lachance (:wlach)]

Attachment: Improve error handling when launchFennec/launchApp fail in mozdevice

This is not strictly a fix for the problem mentioned in this bug, but it sure as heck makes things easier to debug. I'd like to apply it along with the other mozdevice patch.

Comment 5

[William Lachance (:wlach)]

Attachment: Get and use sutagent user id when needed/available for launching applications

Comment 6

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

Comment on attachment 711532 [details] [diff] [review]
Add info option to get sutagent user info

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

nice.

Comment 7

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

Comment on attachment 711538 [details] [diff] [review]
Improve error handling when launchFennec/launchApp fail in mozdevice

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

::: mozdevice/mozdevice/droid.py
@@ +21,5 @@
>          """
>          # only one instance of an application may be running at once
>          if self.processExist(appName):
> +            raise DMError("Only one instance of an application may be running "
> +                          "at once")

nit sure I understand the two strings here and the line break.

@@ +51,3 @@
>  
> +        shellOutput.seek(0)
> +        raise DMError("Unable to launch application (shell output: '%s')" % shellOutput.read())

anychance shellOutput could take a while to read and possibly timeout?

Comment 8

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

Comment on attachment 711542 [details] [diff] [review]
Get and use sutagent user id when needed/available for launching applications

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

I have too many questions about this to give a f+

::: mozdevice/mozdevice/droid.py
@@ +89,5 @@
> +    def _getExtraAmStartArgs(self):
> +        # versions of android after jellybean may run as a different process
> +        # than the one that started the app. we need to get back the original
> +        # user serial number back for this case
> +        if not hasattr(self, 'userSerial'):

where do we define this userSerial attribute?

@@ +94,5 @@
> +            infoDict = self.getInfo(directive="sutuserinfo")
> +            if infoDict.get('sutuserinfo') and \
> +                    len(infoDict['sutuserinfo']) > 0:
> +               userSerialString = infoDict['sutuserinfo'][0]
> +               print "serial string: %s" % userSerialString

will we always print this?  should this be conditional on a debug level?

@@ +95,5 @@
> +            if infoDict.get('sutuserinfo') and \
> +                    len(infoDict['sutuserinfo']) > 0:
> +               userSerialString = infoDict['sutuserinfo'][0]
> +               print "serial string: %s" % userSerialString
> +               m = re.match('User Serial:([0-9]+)', userSerialString)

will the serial always be 0-9+ ?

Comment 9

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #8)
> Comment on attachment 711542 [details] [diff] [review]
> Get and use sutagent user id when needed/available for launching applications
> 
> Review of attachment 711542 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I have too many questions about this to give a f+
> 
> ::: mozdevice/mozdevice/droid.py
> @@ +89,5 @@
> > +    def _getExtraAmStartArgs(self):
> > +        # versions of android after jellybean may run as a different process
> > +        # than the one that started the app. we need to get back the original
> > +        # user serial number back for this case
> > +        if not hasattr(self, 'userSerial'):
> 
> where do we define this userSerial attribute?

It's defined below. I use hasattr (which won't error out when the variable is undefined) so we don't have to set it in the constructor.

> @@ +94,5 @@
> > +            infoDict = self.getInfo(directive="sutuserinfo")
> > +            if infoDict.get('sutuserinfo') and \
> > +                    len(infoDict['sutuserinfo']) > 0:
> > +               userSerialString = infoDict['sutuserinfo'][0]
> > +               print "serial string: %s" % userSerialString
> 
> will we always print this?  should this be conditional on a debug level?

No, I should take this out before I commit.

> @@ +95,5 @@
> > +            if infoDict.get('sutuserinfo') and \
> > +                    len(infoDict['sutuserinfo']) > 0:
> > +               userSerialString = infoDict['sutuserinfo'][0]
> > +               print "serial string: %s" % userSerialString
> > +               m = re.match('User Serial:([0-9]+)', userSerialString)
> 
> will the serial always be 0-9+ ?

Yes, it's always an integer: http://developer.android.com/reference/android/os/UserManager.html#getSerialNumberForUser%28android.os.UserHandle%29

I'll link to that in a comment, since it isn't obvious.

Comment 10

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #7)
> Comment on attachment 711538 [details] [diff] [review]
> Improve error handling when launchFennec/launchApp fail in mozdevice
> 
> Review of attachment 711538 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mozdevice/mozdevice/droid.py
> @@ +21,5 @@
> >          """
> >          # only one instance of an application may be running at once
> >          if self.processExist(appName):
> > +            raise DMError("Only one instance of an application may be running "
> > +                          "at once")
> 
> nit sure I understand the two strings here and the line break.

Just trying to make things fit in 80 characters

> @@ +51,3 @@
> >  
> > +        shellOutput.seek(0)
> > +        raise DMError("Unable to launch application (shell output: '%s')" % shellOutput.read())
> 
> anychance shellOutput could take a while to read and possibly timeout?

No, the command has finished by the time you get to this point. It's all stored inside an in-memory buffer, and you can read it all at once.

Comment 11

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

Comment on attachment 711542 [details] [diff] [review]
Get and use sutagent user id when needed/available for launching applications

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

thanks for clarifying, I am fine as long as you link a comment to what a valid user id is.

Comment 12

[Geoff Brown [:gbrown]]

Comment on attachment 711532 [details] [diff] [review]
Add info option to get sutagent user info

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

::: build/mobile/sutagent/android/DoCommand.java
@@ +2626,5 @@
> +                // should just work
> +                Object userHandle = android.os.Process.class.getMethod("myUserHandle", (Class[])null).invoke(null);
> +                Object userSerial = userManager.getClass().getMethod("getSerialNumberForUser", userHandle.getClass()).invoke(userManager, userHandle);
> +                sRet += "User Serial:" + userSerial.toString();
> +            }

Consider something like:

} else {
    sRet += "User Serial not available before Android 4.2";
}

...but that may just make more parsing work when you retrieve it.

@@ +2628,5 @@
> +                Object userSerial = userManager.getClass().getMethod("getSerialNumberForUser", userHandle.getClass()).invoke(userManager, userHandle);
> +                sRet += "User Serial:" + userSerial.toString();
> +            }
> +        } catch (Exception e) {
> +            // Guard against any unexpected failures

nit: I would prefer that we e.printStackTrace(), unless there is a common exception here.

Comment 13

[Geoff Brown [:gbrown]]

Comment on attachment 711538 [details] [diff] [review]
Improve error handling when launchFennec/launchApp fail in mozdevice

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

LGTM

Comment 14

[Geoff Brown [:gbrown]]

Comment on attachment 711542 [details] [diff] [review]
Get and use sutagent user id when needed/available for launching applications

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

::: mozdevice/mozdevice/droid.py
@@ +88,5 @@
> +
> +    def _getExtraAmStartArgs(self):
> +        # versions of android after jellybean may run as a different process
> +        # than the one that started the app. we need to get back the original
> +        # user serial number back for this case

Is it "after jellybean" or "in jellybean and beyond"? 

I find this wording a little confusing. Consider something more like "In this case, we must specify the user serial number on the 'am start' command line, in order to..."

@@ +94,5 @@
> +            infoDict = self.getInfo(directive="sutuserinfo")
> +            if infoDict.get('sutuserinfo') and \
> +                    len(infoDict['sutuserinfo']) > 0:
> +               userSerialString = infoDict['sutuserinfo'][0]
> +               print "serial string: %s" % userSerialString

reminder: remove the print statement.

Comment 15

[William Lachance (:wlach)]

(In reply to Geoff Brown [:gbrown] from comment #12)
> Comment on attachment 711532 [details] [diff] [review]
> Add info option to get sutagent user info
> 
> Review of attachment 711532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/mobile/sutagent/android/DoCommand.java
> @@ +2626,5 @@
> > +                // should just work
> > +                Object userHandle = android.os.Process.class.getMethod("myUserHandle", (Class[])null).invoke(null);
> > +                Object userSerial = userManager.getClass().getMethod("getSerialNumberForUser", userHandle.getClass()).invoke(userManager, userHandle);
> > +                sRet += "User Serial:" + userSerial.toString();
> > +            }
> 
> Consider something like:
> 
> } else {
>     sRet += "User Serial not available before Android 4.2";
> }
> 
> ...but that may just make more parsing work when you retrieve it.

Yeah, it would be. I'd rather not honestly... I think it keeps things simpler. If I turn out to be wrong, we can revisit this decision later. :)

> @@ +2628,5 @@
> > +                Object userSerial = userManager.getClass().getMethod("getSerialNumberForUser", userHandle.getClass()).invoke(userManager, userHandle);
> > +                sRet += "User Serial:" + userSerial.toString();
> > +            }
> > +        } catch (Exception e) {
> > +            // Guard against any unexpected failures
> 
> nit: I would prefer that we e.printStackTrace(), unless there is a common
> exception here.

Fixed in what I pushed.

Comment 16

[William Lachance (:wlach)]

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/69e3d0de1e01

Comment 17

[William Lachance (:wlach)]

Pushed mozdevice changes as well:

https://github.com/mozilla/mozbase/commit/dbb8b4752ae2ad01106f13fbf0319898a9545557
https://github.com/mozilla/mozbase/commit/68cb8f119f271ef7ad2762cc8deae4288179c4bf
https://github.com/mozilla/mozbase/commit/e8079c8357ff16faa7da925f7bfb8fdb61c113d1

(I pushed the cleanups in an extra commit because I suck and forgot to rebase and apply before doing the initial push... ooops)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/69e3d0de1e01