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

RESOLVED WONTFIX

Status

Testing
Mozbase
RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: bc, Assigned: bc)

Tracking

Trunk
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8523339 [details] [diff] [review]
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/...
Attachment #8523339 - Flags: review?(wlachance)
Attachment #8523339 - Flags: feedback?(jmaher)
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.
Attachment #8523339 - Flags: feedback?(jmaher) → feedback+
(Assignee)

Comment 2

3 years ago
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 on attachment 8523339 [details] [diff] [review]
devicemanagerADB-launchProcess.patch

Makes sense, I think. I kinda wish we could make this method go away.
Attachment #8523339 - Flags: review?(wlachance) → review+
(Assignee)

Comment 4

3 years ago
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
https://hg.mozilla.org/mozilla-central/rev/4386c2d95db3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

3 years ago
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.