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

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[William Lachance (:wlach)]

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.

Comment 1

[William Lachance (:wlach)]

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

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).

Comment 2

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

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.

Comment 3

[Christian Holler (:decoder)]

(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.

Comment 4

[Christian Holler (:decoder)]

(In reply to Christian Holler (:decoder) from comment #3)

> Raiding the error

Should of course be "Raising" >.<

Comment 5

[William Lachance (:wlach)]

(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.

Comment 6

[William Lachance (:wlach)]

Attachment: Updated patch to simplify checking for unsupported run-as string

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.

Comment 7

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

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9964bcbdc4e2

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9964bcbdc4e2