1650891 - Change the location of the geckoview profile used by geckodriver in order to support root-less Android testing

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P1)

Description

[Bob Clary [:bc] (inactive)]

In bug 1486004, we are updating mozdevice to support testing on unrooted android devices. When running on an unrooted device we create the test_root inside the app's directory as /data/data/<app>/test_root though if we are running on a rooted device, we create the test_root in /data/local/tmp/test_root.

geckodriver needs to change the location of the profile in AndroidHandler new() and how the profile is populated in AndroidHandler prepare(). It isn't strictly necessary that the storage permissions be granted in AndroidHandler prepare

If the device is rooted (either adbd is running as root or can be restarted as root, su -c id or su 0 id reports uid=0), then the profile should be pushed directly to /data/local/tmp/test_root/profile and then chmod -R run on it.

If the device is not rooted, then the profile should be pushed to an intermediate directory on /data/local/tmp, then run-as <app> cp -R intermediate /data/data/<app>/test_root/profile to copy the profile into the app's directory then chmod -R must be run on it and the intermediate directory must be deleted after the copy.

NOTE:
There are device specific issues which can possibly be worked around though it adds complexity to the logic to do so. In the future, I expect there to be more device specific limiations or work arounds that may be necessary if the device is to be supported.

• Sony Xperia Z5 Compact (E5823) can hang if adb root is called and should the adb root call should be excluded if possible. See _check_adb_root() in the D79650 patch.
• Samsung S10g SM-G973F will fail to execute run-as if the global setting art_verifier_verify_debuggable is set. mozdevice will raise an exception if it is set with the S10g. See run_as_package() in the D79650 patch.
• su 0 id should be tested last only if the other approaches to detect if the device is rooted due to potential hangs if the device is rooted using Magisk.

The new logic is outlined in the phabricator revision D79650

It might be advisable to allow the original logic or this new logic to be selected via a command line argument.

I believe this covers the requirements. I will be available to test any new versions of geckodriver to make sure they work with the mozdevice.

Comment 1

[Andrew Creskey [:acreskey]]

FYI, I found out why Fenix only now started crashing with Browsertime/geckodriver on Android 10 devices.
They just upgraded to Android10 sdk July 9th:
https://github.com/mozilla-mobile/fenix/pull/11014/commits/02533afb73889cb76f26ce2a9ac12ffeed85cc85

It is definitely the Android 10 Scoped Storage causing the issue since reverting to legacy storage in the manifest allows the app to run.
i.e. <application ... android:requestLegacyExternalStorage="true" ...

Comment 2

[Andrew Creskey [:acreskey]]

Would either of you be able to make a patch with this change?

A custom build of geckodriver would unblock numerous folks working on Fenix performance using Browsertime/geckodriver on unrooted Android 10 devices.

Comment 3

[Benjamin De Kosnik [:bdekoz]]

I second this part: "It might be advisable to allow the original logic or this new logic to be selected via a command line argument."
Having this would give testers much more flexibility in the future.

Comment 4

[Bob Clary [:bc] (inactive)]

I haven't done any work in Rust before. I could try but it might be a bit before I got something worth while. Whimboo is out until August 10 which seems like a long time for you to wait. I'll ask if this is something I can work on.

Comment 5

[James Graham [:jgraham]]

I can help with Rust and GeckoDriver; please reach out if you get stuck.

Comment 6

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

(In reply to Andrew Creskey [:acreskey] [he/him] from comment #1)

It is definitely the Android 10 Scoped Storage causing the issue since reverting to legacy storage in the manifest allows the app to run.
i.e. <application ... android:requestLegacyExternalStorage="true" ...

Is that something that needs to be done at build time? Or is it possible to set this flag later? If Geckodriver could set it, it might be a workaround for the time being. I know that it would only work until we upgraded to SDK11, which will ignore that flag.

Comment 7

[Andrew Creskey [:acreskey]]

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

(In reply to Andrew Creskey [:acreskey] [he/him] from comment #1)

It is definitely the Android 10 Scoped Storage causing the issue since reverting to legacy storage in the manifest allows the app to run.
i.e. <application ... android:requestLegacyExternalStorage="true" ...

Is that something that needs to be done at build time? Or is it possible to set this flag later? If Geckodriver could set it, it might be a workaround for the time being. I know that it would only work until we upgraded to SDK11, which will ignore that flag.

Unfortunately this flag has to be included in the manifest at build time.

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: Bug 1650891 - [geckodriver] Make profile customizable to support root-less Android devices.

Comment 9

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

Bob, please note that I already started with this bug earlier today but didn't assign myself yet due to some open questions. Does it mean that you want to work on it now?

Comment 10

[Bob Clary [:bc] (inactive)]

No. Just that I hadn't heard anything from you and I wanted to discuss it via the phabricator request and which would only take the patch if I gave the bug number and which resulted in the bug being assigned. I've assigned it to you to take forward. If you would like to discuss it before you continue, I think that would be a good idea.

Comment 11

[Bob Clary [:bc] (inactive)]

feedback ni...

Comment 12

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

I will have a look at the review request soon.

Comment 13

[Bob Clary [:bc] (inactive)]

Looks like I'll be taking this forward after all.

Comment 14

[Bob Clary [:bc] (inactive)]

The initial motivation for my patch was to illustrate the issues that needed to be handled. The current version of this patch does do that but not in an acceptable "Rust" way. I think it is a better use of resources if someone who actually knows rust continues this forward.

Comment 15

[Bob Clary [:bc] (inactive)]

I guess Phabricator won't let me go unless I abandon the the patch.

Comment 16

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

(In reply to Bob Clary [:bc] from comment #15)

I guess Phabricator won't let me go unless I abandon the the patch.

Thanks a lot for all your hard work on this bug Bob! It's really appreciated. So I will pick-up the remaining work for refactoring and would hope that you can test the final bits again before we get it landed.

Regarding the patch, lets reopen the revision but I will commandeer it.

Comment 17

[James Graham [:jgraham]]

Attachment: Bug 1650891 - Make AndroidHandler::new connect to the device,

This is a proposed change on top of D87464 which means we don't expose the
unconnected state of AndroidHandler, allowing us to simplify the code.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a272b971b33
[geckodriver] Make profile customizable to support root-less Android devices. r=webdriver-reviewers,jgraham,bc,marionette-reviewers

Comment 19

[Atila Butkovits]

Backed out for rusttest bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/af27a522366657ef93342e508a5c1c54cd2bec65

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=1a272b971b3387a2c00b1957dd56aeb0486dcac3&searchStr=Linux%2Cx64%2Cdebug%2Cbuild-linux64-rusttests%2Fdebug%2CBR&selectedTaskRun=MYDqJMJqSs2TM7h5oCYpXg.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=319789510&repo=autoland&lineNumber=8178

Comment 20

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

Various problems here. So first the compiler fails to build geckodriver. I'm not sure if that is related to the unused AndroidStorage input or not. For me it runs all fine locally. So I just removed that import, and lets see if that works with the next try build.

Also the tests in android.rs cannot be run in CI. As such we would have to mark those as ignored, but still allow them to run locally with cargo test -- --ignored. The same for most of the mozdevice tests. Some of the latter could still be run in CI, and as such I will add mozdevice to the rust crates to test beside geckodriver.

Also com.android.calendar doesn't seem to be a package that is available everywhere, even I thought so because it's part of the gapps suite. Bob, maybe you don't have gapps installed, or just a smaller package that doesn't include the calendar? Which package could we use in the interim to get tests working?

Comment 21

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

The most recent try build is clean:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb47219a229b4b79bbfa4cd0866a90e3a070d137

Comment 22

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

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

Also com.android.calendar doesn't seem to be a package that is available everywhere, even I thought so because it's part of the gapps suite. Bob, maybe you don't have gapps installed, or just a smaller package that doesn't include the calendar? Which package could we use in the interim to get tests working?

Maybe we add a requirement to the tests that org.mozilla.firefox needs to be installed? That would give us better results too.

Comment 23

[Bob Clary [:bc] (inactive)]

org.mozilla.firefox won't work since it isn't debuggable as far as I know. As I mentioned on Element, perhaps your idea of ignoring these unless --ignored is passed and checking that org.mozilla.geckoview_example is installed before running the tests is sufficient.

Comment 24

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

Ok, that sounds good. I will make that change and have an updated patch soon.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/21370b0569b6
[geckodriver] Make profile customizable to support root-less Android devices. r=webdriver-reviewers,jgraham,bc,marionette-reviewers

Comment 26

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/21370b0569b6

Comment 27

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

I believe there is an error in the implementation of this: in the latest GeckoDriver, fenix crashes on start up. I believe this happens because the profile path does not exist because the adb commands to create the profile fail. They fail because, on unrooted devices, we don't have access to the app's data directory. The description in comment 0 mentions:

If the device is not rooted, then the profile should be pushed to an intermediate directory on /data/local/tmp, then run-as <app> cp -R intermediate /data/data/<app>/test_root/profile to copy the profile into the app's directory...

However, when I try using run-as <app> ... on my unrooted Pixel 2 with Android 11, I get: run-as: package not debuggable: org.mozilla.fenix. I similarly found that output when run through mozdevice (those commands return a non-zero exit code but mozdevice returns success for them). See my investigation in https://bugzilla.mozilla.org/show_bug.cgi?id=1680407#c8 for further details. I suggest we try to implement the correct behavior in that bug.

Comment 28

[Bob Clary [:bc] (inactive)]

I guess you won't see this but others will.

If your device is not rooted, and your app is not debuggable and you have Android 11 which enforces the scope storage restrictions which prevent the use of the sdcard and requires either a test root in /data/local/tmp or the app's directory, then you are out of luck. In that circumstance, we can not use /data/local/tmp since it will result in files and directories in the test root which are owned by the app's user and which are not accessible to the shell user and we can not use the app's directory since we can not use run-as to run commands as the app's user.

We have an idea to improve the auto detection to use the sdcard in the case where the device is not rooted and the app is not debuggable but only if scoped storage restriction is not in effect which means typically Android 8 or before though 9 may work.

Comment 29

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

Work for that will happen on bug 1680041, and it's dependencies.