Closed Bug 728993 Opened 11 years ago Closed 11 years ago

Need to check output to capture run-as failure in devicemanagerADB

Categories

(Testing :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla13

People

(Reporter: wlach, Assigned: wlach)

Details

Attachments

(1 file, 1 obsolete file)

Props to Christian Holler for noticing this and providing a patch:

run-as returns 0 even when it won't work, so we'll need to check for its error message. Also, we should still specify the packageName even when run-as won't work.
Here's Christian's patch against mozilla-central, modified very slightly by me for style. It looks quite straightforward and seems to give the expected behaviour to me (that is, verifyRunAs will return the appropriate exception against a non-debuggable application).
Assignee: nobody → wlachance
Attachment #599033 - Flags: review?(jmaher)
Comment on attachment 599033 [details] [diff] [review]
Need to check output to capture run-as failure in devicemanagerADB

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

I like what the patch is doing, just have a couple small things to address below.

::: build/mobile/devicemanagerADB.py
@@ +688,5 @@
> +      # when failing because of a non-existent or non-debuggable package :(
> +      runAsOut = self.runCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"]).communicate()[0].splitlines()
> +      if len(runAsOut) > 0 and runAsOut[0].startswith("run-as:") and \
> +              ("not debuggable" in runAsOut[0] or "is unknown" in runAsOut[0]):
> +        raise DMError("run-as failed sanity check")

Do we want this to raise an error?  Also I think this is a lot of and/or conditions for one long line, we should split it up a bit so it is easier to debug and modify in the future.
Attachment #599033 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #2)

> ::: build/mobile/devicemanagerADB.py
> @@ +688,5 @@
> > +      # when failing because of a non-existent or non-debuggable package :(
> > +      runAsOut = self.runCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"]).communicate()[0].splitlines()
> > +      if len(runAsOut) > 0 and runAsOut[0].startswith("run-as:") and \
> > +              ("not debuggable" in runAsOut[0] or "is unknown" in runAsOut[0]):
> > +        raise DMError("run-as failed sanity check")
> 
> Do we want this to raise an error? 

Raiding the error is required, otherwise the caller (in this case the init function) cannot detect that run-as is not working and will try to use run-as although it is broken.
(In reply to Christian Holler (:decoder) from comment #3)

> Raiding the error

Should of course be "Raising" >.<
(In reply to Joel Maher (:jmaher) from comment #2)
> Comment on attachment 599033 [details] [diff] [review]
> Need to check output to capture run-as failure in devicemanagerADB
> 
> Review of attachment 599033 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I like what the patch is doing, just have a couple small things to address
> below.
> 
> ::: build/mobile/devicemanagerADB.py
> @@ +688,5 @@
> > +      # when failing because of a non-existent or non-debuggable package :(
> > +      runAsOut = self.runCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"]).communicate()[0].splitlines()
> > +      if len(runAsOut) > 0 and runAsOut[0].startswith("run-as:") and \
> > +              ("not debuggable" in runAsOut[0] or "is unknown" in runAsOut[0]):
> > +        raise DMError("run-as failed sanity check")
> 
> Do we want this to raise an error?  Also I think this is a lot of and/or
> conditions for one long line, we should split it up a bit so it is easier to
> debug and modify in the future.

(In reply to Christian Holler (:decoder) from comment #3)
> (In reply to Joel Maher (:jmaher) from comment #2)
> 
> > ::: build/mobile/devicemanagerADB.py
> > @@ +688,5 @@
> > > +      # when failing because of a non-existent or non-debuggable package :(
> > > +      runAsOut = self.runCmd(["shell", "run-as", packageName, "mkdir", devroot + "/sanity"]).communicate()[0].splitlines()
> > > +      if len(runAsOut) > 0 and runAsOut[0].startswith("run-as:") and \
> > > +              ("not debuggable" in runAsOut[0] or "is unknown" in runAsOut[0]):
> > > +        raise DMError("run-as failed sanity check")
> > 
> > Do we want this to raise an error? 
> 
> Raiding the error is required, otherwise the caller (in this case the init
> function) cannot detect that run-as is not working and will try to use
> run-as although it is broken.

Yep. Honestly raising an exception seems like an odd way of communicating that a feature isn't supported, but that's how the function is designed to work at present (see Init() for where it's called). Perhaps we should consider changing this later.
This simplifies checking for the run-as string from four conditions to three, and removes the unnecessary step of splitting the output of calling the process into lines.
Attachment #599033 - Attachment is obsolete: true
Attachment #599166 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9964bcbdc4e2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.