1703070 - [mozdevice] Unify adb responses by removing possible carriage returns (\r)

Status: RESOLVED FIXED

(Testing :: Mozbase Rust, defect)

Description

[Andrew Creskey [:acreskey]]

As this example push shows, the android perftests are failing.

https://treeherder.mozilla.org/jobs?repo=try&tier=1%2C2%2C3&revision=423c76197fdcbeff2385d213fa19af52c70a7c22&selectedTaskRun=Vll80QKzTbe7gSSGpAuoTg.0

From the logs:
[task 2021-04-05T16:12:48.548Z] [2021-04-05 16:10:57] INFO: Running tests using Firefox - 25 iteration(s)
[task 2021-04-05T16:12:48.548Z] [2021-04-05 16:11:10] INFO: firefox failed to start, trying 2 more time(s): adb error: secure_mkdirs failed: Read-only file system
[task 2021-04-05T16:12:48.548Z] [2021-04-05 16:11:22] INFO: firefox failed to start, trying 1 more time(s): adb error: secure_mkdirs failed: Read-only file system
[task 2021-04-05T16:12:48.548Z] [2021-04-05 16:11:34] INFO: firefox failed to start, trying 0 more time(s): adb error: secure_mkdirs failed: Read-only file system
[task 2021-04-05T16:12:48.548Z] [2021-04-05 16:11:34] ERROR: BrowserError: Could not start firefox with 3 tries
[task 2021-04-05T16:12:48.548Z] at SeleniumRunner.start (/builds/worker/.mozbuild/browsertime/node_modules/browsertime/lib/core/seleniumRunner.js:86:13)

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I wonder if this related to the Android 11 problems that whimboo & bdekoz were working on bug 1680407. If it is related, I guess the problem is that the directory they changed Geckodriver to read/write to isn't accessible to the shell or our app, perhaps because it's never accessible after the permissions updates or some state change hasn't occurred yet (e.g. the directory is inaccessible until the app is installed and run).

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

There is indeed a change of behavior visible with my geckodriver patches:

https://treeherder.mozilla.org/jobs?repo=mozilla-central&resultStatus=superseded%2Ctestfailed%2Cbusted%2Cexception%2Csuccess%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&tier=3&searchStr=perftest%2Candroid&fromchange=058997a8167d261290624e8c68ebacda48205615&tochange=098c3172afae38b129ae08e1586499f698a816f3

But previously these tests were always failing with an Internal Server Error. So it's hard to say what's going on here and why my patch changed that.

It would be good to get more details specifically the trace logs from geckodriver, or maybe it's related to some code in browsertime? The adb logcat output doesn't actually show something helpful in respect to this failure.

Andrew, I will hold back the release of geckodriver but would need some help on your side to figure out what's wrong with mozperftest.

Comment 3

[Andrew Creskey [:acreskey]]

Prior to the geckodriver change, these tests were intermittently failing on the Internal Server Error, see Bug 1481291.

Here's one that succeeded.

But yes, I'll do anything I can to help debug this.
Just building the latest now.

Comment 4

[Andrew Creskey [:acreskey]]

From a perftest-engineering discussion:

acreskey:
ok, getting much closer.
I have a push in CI that includes the geckodriver debug logs:
https://firefoxci.taskcluster-artifacts.net/KUm26_P_TSGpso7Xkt4C_g/0/public/logs/live_backing.log
It looks like $EXTERNAL_STORAGE is "/sdcard\r\n"
[taskcluster 2021-04-05T15:39:11.768Z] Worker Type (proj-autophone/gecko-t-bitbar-gw-perf-g5) settings: [taskcluster 2021-04-05T15:39:11.768Z] { [taskcluster 2021-04-05T15:39:11.768Z] "config": {
And I am guessing we get into trouble with this:
mkdir -p /sdcard\r/Android/data/org.mozilla.geckoview_example/files/test_root
whimboo: oh. https://searchfox.org/mozilla-central/source/testing/geckodriver/src/android.rs#180
this extra \r!!

I've put up a test patch that strips the trailing \r from the external storage path.
It's taking a while to run, but hoping it works.
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=e28JKBymSC24Skrwu5MB9Q.0&tier=1%2C2%2C3&revision=88a8e520ea2e0d9d57ce987eab839fa2577797a9

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great finding and good that we caught that before the 0.29.1 release! Thanks a lot Andrew.

As discussed I will take that bug for a final fix and test.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

A better approach than making this change in geckodriver only for the $EXTERNAL_STORAGE retrieval would actually be to unify the adb responses in mozdevice to never include a carriage return. As such all interactions with adb will benefit from.

I pushed a try build to check if that works as expected:
https://treeherder.mozilla.org/jobs?repo=try&revision=b59253e0ad542c837e1c2d183a7e0bfb68b5fd8f

If it's fine I will do a full try build, and get the patch uploaded.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1703070 - [mozdevice-rust] Unify new lines by removing possible carriage returns from adb response.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c0e65a78ade
[mozdevice-rust] Unify new lines by removing possible carriage returns from adb response. r=webdriver-reviewers,jgraham

Comment 9

[Andrew Creskey [:acreskey]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #6)

A better approach than making this change in geckodriver only for the $EXTERNAL_STORAGE retrieval would actually be to unify the adb responses in mozdevice to never include a carriage return. As such all interactions with adb will benefit from.

I pushed a try build to check if that works as expected:
https://treeherder.mozilla.org/jobs?repo=try&revision=b59253e0ad542c837e1c2d183a7e0bfb68b5fd8f

If it's fine I will do a full try build, and get the patch uploaded.

Great work!
I added a few more mozperftest android jobs, but all the ones that you added have succeeded. (We still have Bug 1481291, but I triggered retry for those).

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/3c0e65a78ade