810469 - devicemanagerADB.py tries to use "run-as" for running "am" which segfaults

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: crash log

When I try to run "make mochitests-robotium" on my partially-rooted Galaxy Nexus running Jellybean, something (which I thought was fennec but turned out to be "am") crashes. Crash log attached. I traced this down to the part of devicemanagerADB.py in _checkCmd that inserts the "run-as". Apparently without the "run-as" it works fine, but doing "adb shell run-as org.mozilla.fennec_kats am instrument ..." segfaults immediately. This may be a jellybean-specific thing since running that on another older device doesn't seem to segfault.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

This fixes it for me locally but I don't know if it will break other configurations.

Comment 2

[William Lachance (:wlach)]

I really wonder if this run-as stuff buys us anything besides headaches. It's a neat hack and all, but as far as I can tell all the things we care about run just fine without it. I'd personally favor just ripping it out.

Comment 3

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #2)
> I really wonder if this run-as stuff buys us anything besides headaches.
> It's a neat hack and all, but as far as I can tell all the things we care
> about run just fine without it. I'd personally favor just ripping it out.

Digging a bit, it looks like the run-as support was added for running xpcshell tests over ADB, see bug 669549. But these aren't working at the moment regardless, see bug 799863 (and especially https://bugzilla.mozilla.org/show_bug.cgi?id=799863#c4).

So maybe we shouldn't just rip it out, but we probably need some more coherent story of what the requirements for the xpcshell tests are. It's not clear to me that they will ever work properly on an unrooted device.

Comment 4

[Geoff Brown [:gbrown]]

(In reply to William Lachance (:wlach) from comment #2)
> I really wonder if this run-as stuff buys us anything besides headaches.

I have mixed feelings about run-as. On the one hand, it makes sense to be accessing files and executing commands as fennec (might turn up permission issues, etc), and if root is not available, running as fennec might enable access that wouldn't be available to the shell user. On the other hand, it certainly complicates matters and I can't think of a specific scenario where we actually need it (but I'm always running on rooted devices these days).

Comment 5

[William Lachance (:wlach)]

Comment on attachment 680213 [details] [diff] [review]
Patch

Drive by r+. We want this for dmADB configurations (am should always be executed by the adb user).

Comment 6

[William Lachance (:wlach)]

(In reply to Geoff Brown [:gbrown] from comment #4)
> (In reply to William Lachance (:wlach) from comment #2)
> > I really wonder if this run-as stuff buys us anything besides headaches.
> 
> I have mixed feelings about run-as. On the one hand, it makes sense to be
> accessing files and executing commands as fennec (might turn up permission
> issues, etc), and if root is not available, running as fennec might enable
> access that wouldn't be available to the shell user. On the other hand, it
> certainly complicates matters and I can't think of a specific scenario where
> we actually need it (but I'm always running on rooted devices these days).

I would note that all of our *automated* testing is on devices where root is available, so this run-as thing is only testing the "files owned by user" case when a developer happens to be running a test on their workstation. I can't recall any case in my time working on this stuff where this has turned up an actionable bug.

I propose that for the immediate term we land kats' patch to fix the am issue and maybe revisit this later if it turns out to still be (1) unneeded and (2) causing problems.

Comment 7

[William Lachance (:wlach)]

Attachment: Don't run-as with am in dmADB

This expands on kats' fix to correct runCmd as well

Comment 8

[Geoff Brown [:gbrown]]

Seems sensible - no objection here.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 681113 [details] [diff] [review]
Don't run-as with am in dmADB

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

Lgtm.

Comment 10

[William Lachance (:wlach)]

Pushed (crediting :kats):

https://github.com/mozilla/mozbase/commit/b0bbf0a6010ae7dc320ca3e30be668dda591286d