Closed Bug 838844 Opened 11 years ago Closed 11 years ago

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

Categories

(Testing :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla21

People

(Reporter: wlach, Assigned: wlach)

References

Details

Attachments

(3 files)

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.
I can also just flash it with a different version of android if that is easier.
(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...
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
Attachment #711532 - Flags: review?(gbrown)
Attachment #711532 - Flags: feedback?(jmaher)
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.
Attachment #711538 - Flags: review?(gbrown)
Attachment #711538 - Flags: feedback?(jmaher)
Comment on attachment 711532 [details] [diff] [review]
Add info option to get sutagent user info

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

nice.
Attachment #711532 - Flags: feedback?(jmaher) → feedback+
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?
Attachment #711538 - Flags: feedback?(jmaher) → feedback+
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+ ?
Attachment #711542 - Flags: feedback?(jmaher) → feedback-
(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.
(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 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.
Attachment #711542 - Flags: feedback- → feedback+
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.
Attachment #711532 - Flags: review?(gbrown) → review+
Comment on attachment 711538 [details] [diff] [review]
Improve error handling when launchFennec/launchApp fail in mozdevice

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

LGTM
Attachment #711538 - Flags: review?(gbrown) → review+
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.
Attachment #711542 - Flags: review?(gbrown) → review+
(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.
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)
https://hg.mozilla.org/mozilla-central/rev/69e3d0de1e01
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Blocks: 862508
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: