Closed
Bug 838844
Opened 11 years ago
Closed 11 years ago
"am" command doesn't work under sutagent in jellybean
Categories
(Testing :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla21
People
(Reporter: wlach, Assigned: wlach)
References
Details
Attachments
(3 files)
2.67 KB,
patch
|
gbrown
:
review+
jmaher
:
feedback+
|
Details | Diff | Splinter Review |
2.45 KB,
patch
|
gbrown
:
review+
jmaher
:
feedback+
|
Details | Diff | Splinter Review |
3.70 KB,
patch
|
gbrown
:
review+
jmaher
:
feedback+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 2•11 years ago
|
||
(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...
Assignee | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
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)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #711542 -
Flags: review?(gbrown)
Attachment #711542 -
Flags: feedback?(jmaher)
Comment 6•11 years ago
|
||
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 7•11 years ago
|
||
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 8•11 years ago
|
||
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-
Assignee | ||
Comment 9•11 years ago
|
||
(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.
Assignee | ||
Comment 10•11 years ago
|
||
(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•11 years ago
|
||
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 12•11 years ago
|
||
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 13•11 years ago
|
||
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 14•11 years ago
|
||
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+
Assignee | ||
Comment 15•11 years ago
|
||
(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.
Assignee | ||
Comment 16•11 years ago
|
||
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/69e3d0de1e01
Assignee | ||
Comment 17•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/69e3d0de1e01
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in
before you can comment on or make changes to this bug.
Description
•