669549 - Some DeviceManagerADB functions do not work

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Geoff Brown [:gbrown]]

While developing a remote version of runxpcshelltests.py (bug 668349) I tried using various DeviceManagerADB functions to drive tests on device (Samsung Galaxy S, Android 2.2.1) over USB. I noted these failures:

mkDirs(path) failed for all paths. Note that mkDirs invokes adb shell command "mkdir -p <path>", but the -p option is not available on Android (at least for me); attempting to use mkdir -p <path> on device creates a "-p" directory.

dirExists(path) returns true regardless of the existence of <path>. Note that dirExists invokes adb shell command "ls <path>" and only returns false if this generates an exception. But "ls nonexistentpath" simply generates output:

nonexistentpath: No such file or directory

and there is no cause for an exception.

Comment 1

[Geoff Brown [:gbrown]]

I notice isDir(path) now -- it seems to work fine. The SUT implementation has different implementations of isDir and dirExists; it is not clear to me if there should be a (subtle?) difference between these functions. Could adb.dirExists just call adb.isDir?

Comment 2

[Geoff Brown [:gbrown]]

Attachment: patch for discussion

I stole the SUT version of mkDirs, recursively calling mkDir; it works for me. Should this be moved into the base class (devicemanager)?

I implemented dirExists as a call to isDir. I think they should be equivalent:
                            dirExists    isDir
path specifies a file         false*     false
path specifies a dir          true       true
path does not exist           false      false

but an alternate interpretation is:
                            dirExists    isDir
path specifies a file         true*      false
path specifies a dir          true       true
path does not exist           false      false

Comment 3

[Geoff Brown [:gbrown]]

Attachment: updated patch for discussion

I found another problem. pushDir was implemented simply as "adb push <localdir> <remotedir>" and that mostly works...except for symbolic links. If <localdir> contains a symbolic link, the link is pushed, rather than the linked-to file. To get around this, I walk <localdir> and call pushFile for each file encountered.

Comment 4

[Geoff Brown [:gbrown]]

Attachment: updated patch for discussion

Another minor change: pushFile(local, remote) calls chmodDir(remote); when remote is a directory, this resulted in adb chmod on every file in the directory. Now, only the newly-pushed file is chmod'ed.

Comment 5

[Geoff Brown [:gbrown]]

Attachment: patch for review

Comment 6

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

Comment on attachment 544873 [details] [diff] [review]
patch for review

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

looks good, lets get this in.  I would only like to consider the self.mkDir vs self.mkDirs comment before checking in.

::: build/mobile/devicemanagerADB.py
@@ +67,1 @@
>  

while it would be nice to refactor this into the base class, I would like to wait until this file is stable for a while.  There is some uncertainty with requirements as to rooting a device and run-as support.

@@ +89,5 @@
> +          if (relRoot!="."):
> +            targetDir = targetDir + relRoot + "/"
> +          targetDir = targetDir + dir
> +          if (not self.dirExists(targetDir)):
> +            self.mkDir(targetDir)

does this make all the parent directories as well?  I would think a self.mkDirs() to be safe

@@ +458,5 @@
>          else:
>            self.checkCmd(["shell", "chmod", "777", remoteDir.strip()])
>            print "chmod " + remoteDir.strip()
>        self.checkCmd(["shell", "chmod", "777", remoteDir])
> +      #print "chmod " + remoteDir

why no .strip() here?

Comment 7

[Geoff Brown [:gbrown]]

(In reply to comment #6)
> while it would be nice to refactor this into the base class, I would like to
> wait until this file is stable for a while.  There is some uncertainty with
> requirements as to rooting a device and run-as support.

That sounds sensible - let's revisit later.
 
> @@ +89,5 @@
> > +          if (relRoot!="."):
> > +            targetDir = targetDir + relRoot + "/"
> > +          targetDir = targetDir + dir
> > +          if (not self.dirExists(targetDir)):
> > +            self.mkDir(targetDir)
> 
> does this make all the parent directories as well?  I would think a
> self.mkDirs() to be safe

It works as-is, because this code is nested in the for ... os.walk. I tested by pushing a directory containing directories containing directories (and files) - everything was pushed.
 
> @@ +458,5 @@
> >          else:
> >            self.checkCmd(["shell", "chmod", "777", remoteDir.strip()])
> >            print "chmod " + remoteDir.strip()
> >        self.checkCmd(["shell", "chmod", "777", remoteDir])
> > +      #print "chmod " + remoteDir
> 
> why no .strip() here?

I am not sure, but have not had trouble with it yet, so left it alone!

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 544873 [details] [diff] [review]
patch for review

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

something about this patch is leaving files in the profile with rw-rw-rw- permissions, which breaks running reftests because the profile can't be locked.

Comment 9

[Geoff Brown [:gbrown]]

Attachment: updated patch addressing comment 8

isDir() had a bug too! isDir(file) would return true if file existed and was a file, because trailing whitespace (\r\n) was invalidating a string comparison. My change to avoid over-chmod'ing relied on isDir to detect whether a destination file or directory was specified during a push and concatenated the filename when only the destination directory was specified. Interaction of these features created a bad filename for many chmod requests (eg ".../user.js/user.js") I fixed isDir and re-tested pushDir.

Comment 10

[Geoff Brown [:gbrown]]

Attachment: patch for review

This version fixes a problem with permissions on the top-level directory of a pushDir.

Comment 11

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

if we can change /data/local inside of getDeviceRoot to /mnt/sdcard, I saw mochitest and reftest work just fine on my tegra using the ADB agent.

Comment 12

[Geoff Brown [:gbrown]]

devicemanagerSUT's getDeviceRoot uses the SD card as a default, if it exists; otherwise, android.content.Context.getFilesDir(). I changed my ADB getDeviceRoot to use /mnt/sdcard if it exists, else /data/local. For me, this caused a mkDirs failure on /mnt; I updated mkDirs to skip directory components that already exist -- problem solved.

Still, on my device, I see minor permission problems using /mnt/sdcard: I cannot chmod or chown /mnt/sdcard/tests or any sub-directory, except as root. My /mnt/sdcard has u=system g=sdcard_rw perms 775. Running as u=shell, g=shell (sdcard_rw), I can create/read/write in /mnt/sdcard, but chmod returns "Operation not permitted".

I have not had any problems running in /data/local/tests, as shell/shell.

Comment 13

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

I see the operation no permitted messages as well.  Also, we need to change the listFiles command to be ls -a as we can have .files.

Comment 14

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

Attachment: patch that I am using to run unittests via adb

with this patch I can run talos, reftest, mochitest unittests on a nexus S unrooted virgin phone.  I just wanted to get what I had working posted.  This patch has a lot of permissions type errors for the fileIO (/mnt/sdcard stuff), but things do run just fine.

Comment 15

[Geoff Brown [:gbrown]]

Attachment: updated patch

This patch includes most of the changes in jmaher's patch (comment 14) and also attempts to chown <fennec-uid> each directory created. I find that chown always fails with "Operation not permitted", unless run with root permissions...so this may not be a useful feature.

Note that this version of getDeviceRoot:
 - returns /data/local/tests IF that directory exists, else
    - returns /mnt/sdcard/tests IF /mnt/sdcard exists, else
       - returns /data/local/tests
The intention is that /mnt/sdcard/tests is the default, unless there is no sdcard, or unless over-ridden by the creation of /data/local/tests (xpcshell tests will likely use this, since it looks like they cannot run on /sdcard).

Comment 16

[Geoff Brown [:gbrown]]

Interesting chown/chgrp behavior...

$ cd /tmp
$ echo hello > zz
$ ls -l zz
-rw-rw-rw- shell    shell           6 2011-07-11 18:57 zz
$ run-as org.mozilla.fennec_unofficial id
uid=10083(app_83) gid=10083(app_83) groups=1003(graphics),1004(input),1007(log),1009(mount),1011(adb),1015(sdcard_rw),3001(net_bt_admin),3002(net_bt),3003(inet)
$ chgrp 10083 zz
chgrp: zz: Operation not permitted
$ chgrp app_83 zz
chgrp: zz: Operation not permitted
$ chgrp 1003 zz
$ ls -l zz
-rw-rw-rw- shell    graphics        6 2011-07-11 18:57 zz
$ chgrp log zz
$ ls -l zz
-rw-rw-rw- shell    log             6 2011-07-11 18:57 zz

...but this does not help us.

Maybe devicemanagerADB should "run-as org.mozilla.fennec_unofficial mkdir ..."? But how does devicemanagerADB come up with the package name?

Comment 17

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

in devicemanagerSUT, we look for an appdir in /data/data/org.mozilla.*;  This can be deterministic of the package name.  The problem is we would need a rooted device.  

Another solution is to require the package name at some point during initialization or before we do fileIO.  This might break existing scripts, but all the automation that uses this to date (talos, reftest, mochitest, some [maybe all] of the releng tools) pass in a package name (i.e. org.mozilla.fennec).  We would need to update the scripts that instantiate devicemanagerADB() and have them setup the package name.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #16)
> Maybe devicemanagerADB should "run-as org.mozilla.fennec_unofficial mkdir
> ..."? 
That makes sense 

> But how does devicemanagerADB come up with the package name?
Seems reasonable to pass that into the initialization. devicemanagerADB should probably default to org.mozilla.fennec_unofficial, but the run_*_remote.py scripts should specify it.

Comment 19

[Geoff Brown [:gbrown]]

Attachment: updated patch - mkDir uses run-as <package>

Thanks for the tips. This version of the patch no longer attempts chown; instead, mkDir executes "adb shell run-as <package-name> mkdir <dir>". The default package-name is org.mozilla.fennec_unofficial, and can be modified in the devicemanagerADB ctor.

Note that this change does not affect pushFile, so files are still created with shell ownership.

Comment 20

[Geoff Brown [:gbrown]]

Attachment: updated patch - use run-as for file and directory creation

This update pushes files to a temporary location, then uses "run-as <package> cp ..." to create files with the correct ownership. It works great for me and I have no other updates in mind, so would appreciate another review.

Comment 21

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

Attachment: updates to mochitest and reftest harness to work with packageName (1.0)

additional changes needed to existing tools so we can run out of the box.

Comment 22

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

Comment on attachment 545521 [details] [diff] [review]
updated patch - use run-as for file and directory creation

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

this works great.  I added another patch to this bug which fixes the runtestsremote.py and remotereftests.py to work as well.  If you can land both patches together that will help!

::: build/mobile/devicemanagerADB.py
@@ +489,2 @@
>    def chmodDir(self, remoteDir):
> +    #print "called chmodDir"

can we remove this comment?

Comment 23

[Geoff Brown [:gbrown]]

Comment on attachment 545662 [details] [diff] [review]
updates to mochitest and reftest harness to work with packageName (1.0)

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

::: layout/tools/reftest/remotereftest.py
@@ +390,5 @@
>          sys.exit(1)
>  
>      if (options.dm_trans == "adb"):
>          if (options.deviceIP):
> +            dm = devicemanagerADB.DeviceManagerADB(options.deviceIP, options.devicePort, options.app)

The 3rd parameter in the DeviceManagerADB initializer is actually retrylimit, not packageName. Your tests probably work because retrylimit is unused, and packageName has a useful default, but you won't be able to set the package name this way.

::: testing/mochitest/runtestsremote.py
@@ +312,5 @@
>      parser = RemoteOptions(auto, scriptdir)
>      options, args = parser.parse_args()
>      if (options.dm_trans == "adb"):
>          if (options.deviceIP):
> +            dm = devicemanagerADB.DeviceManagerADB(options.deviceIP, options.devicePort, options.app)

Ditto.

Comment 24

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

Attachment: updates to mochitest and reftest harness to work with packageName (1.1)

odd why it worked for me since I was using org.mozilla.fennec and had never installed a org.mozilla.fennec_unofficial on the device I have been using for test.  This is updated and should be more accurate.

Comment 25

[Geoff Brown [:gbrown]]

Comment on attachment 545702 [details] [diff] [review]
updates to mochitest and reftest harness to work with packageName (1.1)

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

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

These were landed on m-i, and backed out because they broke android tests.

Comment 27

[Brad Lassey [:blassey] (use needinfo?)]

Geoff, here's the results from m-i so you can diagnose the problem http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=abe8569eca76

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

======== BuildStep started ========
'python mochitest/runtestsremote.py ...' warnings
=== Output ===
python mochitest/runtestsremote.py --deviceIP 10.250.49.44 --xre-path ../hostutils/xre --utility-path ../hostutils/bin --certificate-path certs --app org.mozilla.fennec --console-level INFO --http-port 30057 --ssl-port 31057 --pidfile /builds/tegra-057/test/../runtestsremote.pid --browser-chrome --test-path mobile
 in dir /builds/tegra-057/test/build/tests (timeout 2400 secs)
 watching logfiles {}
 argv: ['python', 'mochitest/runtestsremote.py', '--deviceIP', '10.250.49.44', '--xre-path', '../hostutils/xre', '--utility-path', '../hostutils/bin', '--certificate-path', 'certs', '--app', 'org.mozilla.fennec', '--console-level', 'INFO', '--http-port', '30057', '--ssl-port', '31057', '--pidfile', '/builds/tegra-057/test/../runtestsremote.pid', '--browser-chrome', '--test-path', 'mobile']
 environment:
  PATH=/opt/local/bin:/opt/local/sbin:/opt/local/Library/Frameworks/Python.framework/Versions/2.6/bin:/usr/bin:/bin:/usr/sbin:/sbin:/usr/local/bin:/usr/X11/bin
  PWD=/builds/tegra-057/test/build/tests
  SUT_IP=10.250.49.44
  SUT_NAME=tegra-057
  __CF_USER_TEXT_ENCODING=0x1F6:0:0
 closing stdin
 using PTY: False
Traceback (most recent call last):
  File "mochitest/runtestsremote.py", line 352, in <module>
    main()
  File "mochitest/runtestsremote.py", line 310, in main
    dm_none = devicemanagerADB.DeviceManagerADB()
  File "/builds/tegra-057/test/build/tests/mochitest/devicemanagerADB.py", line 12, in __init__
    self.verifyPackage(packageName)
  File "/builds/tegra-057/test/build/tests/mochitest/devicemanagerADB.py", line 511, in verifyPackage
    data = self.runCmd(["shell", "run-as", packageName, "pwd"]).stdout.read()
  File "/builds/tegra-057/test/build/tests/mochitest/devicemanagerADB.py", line 477, in runCmd
    return subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/subprocess.py", line 623, in __init__
    errread, errwrite)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/subprocess.py", line 1141, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory
program finished with exit code 1
elapsedTime=0.625149
TinderboxPrint: mochitest-browser-chrome<br/>
Unknown Error: command finished with exit code: 1
=== Output ended ===
======== BuildStep ended ========

Comment 29

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

so when we instantiate devicemanagerADB(), it tries to do a verifyPackage().  This needs to be optional.  My bad for not catching this earlier.  The automation system doesn't have ADB in the path, nor anything connected to it, when I tested the patch with SUT, it verified the package on the ADB device I had connected.

We could even put this in a try/except block.

Comment 30

[Geoff Brown [:gbrown]]

Attachment: updated devicemanagerADB patch addressing comments 26-29

Updated devicemanagerADB to allow for initialization even when adb is missing:  guarded with try / except.

Comment 31

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

Comment on attachment 545817 [details] [diff] [review]
updated devicemanagerADB patch addressing comments 26-29

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

looks good.  This works on a SUT agent with no adb available or device connected.  I had a lot of trouble getting this to work locally via ADB and found that removing the packageName so we do not do a 'runas org.mozilla.fennec' for the mkdir/pushFile commands my tests would run.

So lets not check in the other patch to modify the harnesses until we can figure out why runas isn't working.

Comment 32

[Geoff Brown [:gbrown]]

I would like to understand the run-as failure before checking in.

I tested this morning and did not experience problems. 

However, I noted some possible sources of trouble:
 - When we run-as <a.b.c>, the cwd is set to /data/data/<a.b.c>; a script that uses relative paths or otherwise assumes a cwd will experience trouble.
 - When we run-as <a.b.c>, we may have different file access permissions as compared to shell. In particular, on my device, shell can create directories under /data/local, but org.mozilla.fennec_unofficial cannot. I do not see any comparable problems on /mnt/sdcard.

Comment 33

[Geoff Brown [:gbrown]]

It seems that jmaher's problem with run-as is a known limitation for official builds. I think we can go ahead with check-in.

Comment 34

[Joe Drew (not getting mail)]

This (along with most things committed on Friday afternoon) was backed out of mozilla-inbound in order to clear up orange.

Comment 35

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Please clarify what needs to land here.

Comment 36

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/1627d7efe035