1099475 - devicemanagerADB.py - Do not quote uri in launchProcess since it is passed in an argument array in _checkCmd

Status: RESOLVED WONTFIX

(Testing :: Mozbase, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: devicemanagerADB-launchProcess.patch

http://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozdevice/mozdevice/devicemanagerADB.py#371

 if uri != "":
            acmd.append("-d")
            acmd.append(''.join(['\'',uri, '\'']));

This ends up double quoting the uri since _checkCmd uses Popen and an array of arguments. The arguments are passed down to one of the os.exec family and then to the underlying OS's exec. I don't think the quoting is necessary because exec knows the argument boundaries since they are contained in an array.

Note adb_device.py doesn't do the quote thingy.

I found this problem trying to run the mochitests using adb instead of sut (adjust to suit your environment):

python -u mochitest/runtestsremote.py --run-only-tests=android.json --certificate-path=certs --close-when-done --autorun --console-level=DEBUG --log-tbpl=dummy-mochitest-1-samsung-gs3-3.log --log-tbpl-level=DEBUG --dm_trans=adb --deviceSerial=501a2aa7 --app=org.mozilla.fennec --xre-path=/mozilla/projects/autophone/downloads/firefox --utility-path=/mozilla/projects/autophone/downloads/tests/bin --timeout=4800 --remote-webserver=192.168.1.50 --http-port=54313 --ssl-port=60558 --total-chunks=16 --this-chunk=1 --pidfile=mochitest-samsung-gs3-3.pid --symbols-path=/mozilla/projects/autophone/src/autophone/builds/aHR0cDovL2Z0cC5tb3ppbGxhLm9yZy9wdWIvbW96aWxsYS5vcmcvbW9iaWxlL3RpbmRlcmJveC1idWlsZHMvbW96aWxsYS1pbmJvdW5kLWFuZHJvaWQvMTQxNTkzNTIyNC9mZW5uZWMtMzYuMGExLmVuLVVTLmFuZHJvaWQtYXJtLmFwaw==/symbols

The symptom was that the mochitest was loaded using a url of the form 'http://mochi.test:8888/... rather than http://mochi.test.8888/...

Comment 1

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

Comment on attachment 8523339 [details] [diff] [review]
devicemanagerADB-launchProcess.patch

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

the code change looks fine.  The reason for quoting is when we have & and ? in a url it would cause problems with the shell when we would launch fennec.  Assuming this change works for mochitest and reftest via adb, I suspect this is fine.  it could be a specific version of android had issues, but at the time it was probably all we were testing on.

Comment 2

[Bob Clary [:bc] (inactive)]

I'll double check reftests though I didn't see a problem with them. I think that is due to the way reftests are run via the extension instead of loading a url.

Comment 3

[William Lachance (:wlach)]

Comment on attachment 8523339 [details] [diff] [review]
devicemanagerADB-launchProcess.patch

Makes sense, I think. I kinda wish we could make this method go away.

Comment 4

[Bob Clary [:bc] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb92abb8a0f6

I had tried a try run but could not get android tests to run. I finally gave up
and pushed anyway.
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4c0312072512

Comment 5

[Bob Clary [:bc] (inactive)]

(In reply to Bob Clary [:bc:] from comment #4)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fb92abb8a0f6

should have been:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4386c2d95db3

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4386c2d95db3

Comment 7

[Geoff Brown [:gbrown] (pto Apr 11-18)]

This caused bug 1119962, breaking adb-based local mochitests for Android. See discussion in 1119962. 

Backed-out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6485d16e2d79