1680407 - Auto detection of --android-storage cause crashes of Firefox for unrooted Android devices

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

Reporter

Description

•

3 years ago

Steps to reproduce

On Galaxy S5 (I'm unable to test on my other device due to bug 1667224 but maybe it'd have the same behavior)...
Install fenix nightly
Run mach perftest:

python3 python/mozperftest/mozperftest/runner.py \
    --flavor mobile-browser \
    --android \
    --android-app-name org.mozilla.fenix \
    --perfherder-metrics processLaunchToNavStart \
    --android-activity org.mozilla.fenix.IntentReceiverActivity \
    --android-clear-logcat \
    --android-capture-logcat logcat \
    --hooks testing/performance/hooks_android_view.py \
    --perfherder \
    --perfherder-app fenix \
    --browsertime-iterations 2 \
    --output sb/artifacts \
    testing/performance/perftest_android_view.js

Expected results

Successful run.

Actual results

On the Geckodriver provided with mc (my tip is 71ca5351d995), the browser fails to start with:

[2020-12-02 16:37:07] INFO: Browser failed to start, trying 2 more time(s): Package 'org.mozilla.fenix' not found
[2020-12-02 16:37:08] INFO: Browser failed to start, trying 1 more time(s): Package 'org.mozilla.fenix' not found
[2020-12-02 16:37:09] INFO: Browser failed to start, trying 0 more time(s): Package 'org.mozilla.fenix' not found
[2020-12-02 16:37:09] ERROR: BrowserError: Could not start the browser with 3 tries
at SeleniumRunner.start (/Users/mcomella/.mozbuild/browsertime/node_modules/browsertime/lib/core/seleniumRunner.js:86:13)
at process._tickCallback (internal/process/next_tick.js:68:7)
[2020-12-02 16:37:09] ERROR: No data to collect

I have confirmed that the package is actually on the device.

Using the versions of geckodriver downloaded from the geckodriver releases page:

28.0: the browser perma-crashes on start-up (trying to launch the browser even without perftest at this point will cause it to crash on start up until adb shell am clear-debug-app is called)
27.0: I get the same error as the default mc installation
26.0: runs successfully

I'm running this on macOS. I'm using a month-ish old build of fenix but I think I tested on the latest nightly and had the same issues.

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

Reporter

Updated

•

3 years ago

Blocks: 1667224

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

Reporter

Comment 1

•

3 years ago

I get the perma-crash behavior on my Pixel 2 with geckodriver 0.28 too.

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

Reporter

Comment 2

•

3 years ago

•

Edited

Testing on Pixel 2...

The crash logs aren't that explicit:

12-07 13:58:24.817 E/AndroidRuntime( 5941): FATAL EXCEPTION: main
12-07 13:58:24.817 E/AndroidRuntime( 5941): Process: org.mozilla.fenix, PID: 5941
12-07 13:58:24.817 E/AndroidRuntime( 5941): java.lang.RuntimeException: GeckoRuntime is shutting down
12-07 13:58:24.817 E/AndroidRuntime( 5941): at mozilla.components.browser.engine.gecko.GeckoEngine$2.onShutdown(GeckoEngine.kt:1)
12-07 13:58:24.817 E/AndroidRuntime( 5941): at org.mozilla.geckoview.GeckoRuntime$1.handleMessage(GeckoRuntime.java:3)

The file copied to my device, /data/local/tmp/org.mozilla.fenix-geckoview-config.yaml, is:

## GeckoView configuration YAML
##
## Auto-generated by geckodriver.
## See https://mozilla.github.io/geckoview/consumer/docs/automation.
---
env:
  MOZ_CRASHREPORTER: "1"
  MOZ_CRASHREPORTER_NO_REPORT: "1"
  MOZ_CRASHREPORTER_SHUTDOWN: "1"
args:
  - "--marionette"
  - "--profile"
  - /data/data/org.mozilla.fenix/test_root/org.mozilla.fenix-geckodriver-profile

That last line shows the profile is being added to the data directory of the app. However, if the device is not rooted, as far as I know this should be impossible (unless it's created by the app itself but that would affect perf and thus I doubt that we'd do that). My speculation is that Gecko is crashing because it's unable to access the profile directory (since it doesn't exist) but, since the read is in native code, we don't get to see the logs for that.

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

Reporter

Comment 3

•

3 years ago

On geckodriver 0.26 (on my GS5), I see the path /mnt/sdcard/org.mozilla.fenix-geckodriver-profile – that backs up the theory that we're requiring a rooted path in the comment above.

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

Reporter

Comment 4

•

3 years ago

Building the latest geckodriver in mozilla-central f3ebdd48906c, I still crash with the same logcat output. The path is the same as comment 2 so I think geckodriver is still assuming root privileges, despite bug 1650891.

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

Reporter

Comment 5

•

3 years ago

https://bugzilla.mozilla.org/show_bug.cgi?id=1650891#c0, which is supposed to allow unrooted testing, says:

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.

The path matches the path we see above so it's likely this is the expected process (though it could have changed in implementation). I tried run-as on my device and it doesn't work because the package is not debuggable. I wonder if we're expecting that to work and don't validate it did actually work. That might explain why it's not working.

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

Reporter

Comment 6

•

3 years ago

The need for the changes in bug 1650891 appears to be that in Android 11, Scoped Storage was enabled which limits an app's access to external storage to application-specific directories after requesting a user permission. I'm not sure how this interacts with the virtual /mnt/sdcard that seems to appear on many devices.

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

Reporter

Comment 7

•

3 years ago

Assuming we can't write to the application's data directory on unrooted devices in non-debuggable packages like we're trying to do, we may be able to write to the external app's directory: the path to the external files dir on my Pixel 2, /storage/emulated/0/Android/data/org.mozilla.fenix/files is readable by adb shell without doing anything special. One issue could be that the path may change across devices.

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

Reporter

Comment 8

•

3 years ago

I confirmed my suspicion: mkdir -p <test_root> is getting called with the path in comment 2 but the command fails because the package is not debuggable. I confirmed this by logging the command name and adb output of execute_host_command in https://searchfox.org/mozilla-central/rev/71621bfa47a371f2b1ccfd33c704913124afb933/testing/mozbase/rust/mozdevice/src/lib.rs#507:

"mkdir -p /data/data/org.mozilla.fenix/test_root"
 .   "run-as: package not debuggable: org.mozilla.fenix\n"

For some reason, this code does correctly read the exit code of these commands so it's not throwing an error sooner.

From my historical android knowledge, I don't think the approach we're trying to take is possible (writing to the app's data dir on unrooted devices) and I think we'll have to pursue an approach like comment 7.

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

Reporter

Comment 10

•

3 years ago

I'm going to simplify this issue: we only care about the latest GeckoDriver 28+. Also, since we duped bug 1667224 to this, I'll make it a part of the title too.

Summary: Cannot run mach perftest Android VIEW on GeckoDriver 27+: fails with "Could not start the browser" or crash → ./mach perftest Android VIEW fails on unrooted devices with GeckoDriver 0.28: crash on startup. Blocks testing on Android 11

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

Reporter

Updated

•

3 years ago

Blocks: 1650891

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

Reporter

Comment 11

•

3 years ago

I summarized the issue so far in the original bug that implemented this: https://bugzilla.mozilla.org/show_bug.cgi?id=1650891#c27

Bob Clary [:bc] (inactive)

Comment 12

•

3 years ago

As I stated in bug 1650891 comment 28

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.

See bug 1680041

Updated

•

3 years ago

Comment 13

•

3 years ago

Thanks for the info, bc.

What about the approach I suggest in comment 7? What about the app's external storage directory, the path returned by getExternalFilesDir (which requires Manifest.permission.WRITE_EXTERNAL_STORAGE to read)? On my unrooted pixel 2 with Android 11, this path is /storage/emulated/0/Android/data/org.mozilla.fenix/files and it's accessible by the shell.

The multiple developer docs about storage and scoped storage aren't clear about whether or not the shell should be able to access this space. The most direct wording I found is (source):

To give users more control over their files and to limit file clutter, apps that target Android 10 (API level 29) and higher are given scoped access into external storage, or scoped storage, by default. When scoped storage is enabled, apps cannot access the app-specific directories that belong to other apps.

In previous versions of Android, this scoping has been implemented by each application getting its own unix user and writing files with that user. However, that doesn't appear to be what's happening on my Pixel 2 such that the shell has access – perhaps they are using other mechanisms for scoping for whatever reason.

For completeness, my concerns with using the getExternalFilesDir approach would be:

Maybe having shell access is unintended behavior and will be changed in future versions
The app's-specific external files directory, i.e. the path returned by getExternalFilesDir, could change between devices
Not every device supports emulated external storage like my Pixel 2 does

:bc, what do you think?

I also found this note in one doc:

If your app has another use case that isn't covered by scoped storage, file a feature request.

We could also try that if this approach does not work.

Flags: needinfo?(bob)

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

Reporter

Comment 14

•

3 years ago

Note: I did test this path by recompiling geckodriver and mach perftest captured data correctly. I replaced these three lines with:

let mut buf = PathBuf::from("/storage/emulated/0/Android/data/");
buf.push(&options.package);
buf.push("test_root")

Bob Clary [:bc] (inactive)

Comment 15

•

3 years ago

That sounds very interesting and promising. My first question is how to we get the value returned from getExternalFilesDir in Rust and via adb? I'll leave the NI so I don't forgot to circle back on this. One thing to note is I will be leaving soon and probably won't have much time to work on this but I'll do what I can.

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

Reporter

Comment 16

•

3 years ago

My first question is how to we get the value returned from getExternalFilesDir in Rust and via adb?

My two thoughts are:

There appears to be an EXTERNAL_STORAGE environment variable (on my P2, it returns /sdcard, which is the same as /storage/emulated/0). Using the reconstructed path (/sdcard/Android/data/<pkg>/test_root) works on my P2 (same approach as comment 14) though this could change across devices. The docs imply Android/data/<pkg> is now a standard though. This is via https://android.stackexchange.com/a/14109
We could create a standalone app that launches, prints out the output of getExternalFilesDir, and exits. We would install it, run it, capture the logcat, and uninstall. Let me know if you want me to create that app

Thanks for your help!

Bob Clary [:bc] (inactive)

Comment 17

•

3 years ago

(In reply to Michael Comella (:mcomella) [needinfo or I won't see it] from comment #13)

What about the approach I suggest in comment 7? What about the app's external storage directory, the path returned by getExternalFilesDir (which requires Manifest.permission.WRITE_EXTERNAL_STORAGE to read)? On my unrooted pixel 2 with Android 11, this path is /storage/emulated/0/Android/data/org.mozilla.fenix/files and it's accessible by the shell.

I did notice these directories aren't created until the app is run. That might be an issue.

I no longer have a working Pixel2 so I can't check this on a real device however when testing locally with Android x86 emulators from the SDK this works for Android 7.0, 7.1, 8.0, 8.1, 9.0 and 10.0 but not for Android 11. I get permission denied errors. It may an issue with how the sdcard permissions are set up on your device but it does seem weird to leave this data world accessible just because it in on the emulated sdcard.

@aerickson: Can you or someone at bitbar help determe what the situation is for unrooted real devices with Android 11 and whether ls -la /storage/emulated/0/Android/data works?

Flags: needinfo?(bob) → needinfo?(aerickson)

Tarek Ziadé (:tarek)

Updated

•

3 years ago

Assignee: tarek → nobody

Bob Clary [:bc] (inactive)

Comment 18

•

3 years ago

(In reply to Michael Comella (:mcomella) [needinfo or I won't see it] from comment #8)

For some reason, this code does correctly read the exit code of these commands so it's not throwing an error sooner.

The implementation of the adb commands over tcp in the rust implementations do not detect failures. That situation has existed since their implementations and was not changed in geckodriver 0.28.

(In reply to Michael Comella (:mcomella) [needinfo or I won't see it] from comment #13)

Thanks for the info, bc.

What about the approach I suggest in comment 7? What about the app's external storage directory, the path returned by getExternalFilesDir (which requires Manifest.permission.WRITE_EXTERNAL_STORAGE to read)? On my unrooted pixel 2 with Android 11, this path is /storage/emulated/0/Android/data/org.mozilla.fenix/files and it's accessible by the shell.

I can not reproduce this with Fenix production or Nightly downloaded from Google Play on an Android 11 x86 emulator with Google Play which is not rooted. I fail to access anything inside /storage/emulated/0/Android/data/ or /storage/emulated/0/Android/data/org.mozilla.fenix/files. Also note that it is not possible to change file or directory permissions on external storage which can cause their own set of problems. For what it's worth, it seems like a bug to me if /storage/emulated/0/Android/data/org.mozilla.fenix/files is world accessible.

Since I will be leaving soon, it will have to be someone else to carry this forward. I will focus instead on bug 1681439 in the time remaining.

Andrew Erickson [:aerickson]

Comment 19

•

3 years ago

ls -la /storage/emulated/0/Android/data works for me on my Pixel 3 on Android 11 (unrooted).

➜  ~  adb shell getprop ro.build.version.release 
11
➜  ~  adb shell getprop ro.build.version.sdk 
30
➜  ~  adb shell
blueline:/ $ ls -la /storage/emulated/0/Android/data/org.mozilla.firefox_beta/                                      
total 22
drwxrwx--x   3 u0_a158 sdcard_rw  3488 2019-05-26 02:27 .
drwxrwx--x 225 root    sdcard_rw 20480 2020-12-11 11:25 ..
drwxrwx--x   4 u0_a158 sdcard_rw  3488 2019-05-26 09:16 files
blueline:/ $ ls -la /storage/emulated/0/Android/data/org.mozilla.firefox_beta/files                                 
total 12
drwxrwx--x 4 u0_a158 sdcard_rw 3488 2019-05-26 09:16 .
drwxrwx--x 3 u0_a158 sdcard_rw 3488 2019-05-26 02:27 ..
drwxrwx--x 2 u0_a158 sdcard_rw 3488 2019-05-26 02:27 Download
drwxrwx--x 2 u0_a158 sdcard_rw 3488 2019-05-26 09:16 temp
blueline:/ $

Flags: needinfo?(aerickson)

Bob Clary [:bc] (inactive)

Comment 20

•

3 years ago

Great! It doesn't look like you have the ability to create files or directories do you? What permissions do they have if you can create them?

Andrew Erickson [:aerickson]

Comment 21

•

3 years ago

I am able to create files.

blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ ls -la
total 22
drwxrwx--x   3 u0_a158 sdcard_rw  3488 2020-12-16 13:52 .
drwxrwx--x 225 root    sdcard_rw 20480 2020-12-11 11:25 ..
drwxrwx--x   4 u0_a158 sdcard_rw  3488 2020-12-16 13:51 files
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ touch aje_test
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ ls -la
total 22
drwxrwx--x   3 u0_a158 sdcard_rw  3488 2020-12-16 13:52 .
drwxrwx--x 225 root    sdcard_rw 20480 2020-12-11 11:25 ..
-rw-rw----   1 u0_a158 sdcard_rw     0 2020-12-16 13:52 aje_test
drwxrwx--x   4 u0_a158 sdcard_rw  3488 2020-12-16 13:51 files
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $

Bob Clary [:bc] (inactive)

Comment 22

•

3 years ago

Great again!

drwxrwx--x 225 root    sdcard_rw 20480 2020-12-11 11:25 ..
-rw-rw----   1 u0_a158 sdcard_rw     0 2020-12-16 13:52 aje_test

Is problematic. Can you write/read it? If so, that means the shell user can access things it creates. I wonder also can you access the contents of either the temp or Download directories? Those would have been created by the app and probably have the app as the owner. It would be important for us to be able to read, write and delete content owned by the app as well.

Can you change the permissions of the directories or files? Does that have any effect on the earlier attempts to create or modify files?

Andrew Erickson [:aerickson]

Comment 23

•

3 years ago

I am able to read and modify the file. chmod works, but doesn't have an effect for files or directories.

I'm able to browse temp and Download (files downloaded don't actually go to this download dir though, but to /Download/Download for some reason).

blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ ls -la
total 26
drwxrwx--x   3 u0_a158 sdcard_rw  3488 2020-12-16 13:52 .
drwxrwx--x 225 root    sdcard_rw 20480 2020-12-11 11:25 ..
-rw-rw----   1 u0_a158 sdcard_rw     5 2020-12-16 14:38 aje_test
drwxrwx--x   4 u0_a158 sdcard_rw  3488 2020-12-16 13:51 files
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ rm aje_test
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ touch aje_test
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ echo "blah 123" >> aje_test
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ cat aje_test
blah 123
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ ls -la
total 26
drwxrwx--x   3 u0_a158 sdcard_rw  3488 2020-12-16 14:38 .
drwxrwx--x 225 root    sdcard_rw 20480 2020-12-11 11:25 ..
-rw-rw----   1 u0_a158 sdcard_rw     9 2020-12-16 14:39 aje_test
drwxrwx--x   4 u0_a158 sdcard_rw  3488 2020-12-16 13:51 files
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ chmod 600 aje_test
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ ls -la
total 26
drwxrwx--x   3 u0_a158 sdcard_rw  3488 2020-12-16 14:38 .
drwxrwx--x 225 root    sdcard_rw 20480 2020-12-11 11:25 ..
-rw-rw----   1 u0_a158 sdcard_rw     9 2020-12-16 14:39 aje_test
drwxrwx--x   4 u0_a158 sdcard_rw  3488 2020-12-16 13:51 files
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ mkdir test123
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ ls -la
total 29
drwxrwx--x   4 u0_a158 sdcard_rw  3488 2020-12-16 14:42 .
drwxrwx--x 225 root    sdcard_rw 20480 2020-12-11 11:25 ..
-rw-rw----   1 u0_a158 sdcard_rw     9 2020-12-16 14:39 aje_test
drwxrwx--x   4 u0_a158 sdcard_rw  3488 2020-12-16 13:51 files
drwxrwx--x   2 u0_a158 sdcard_rw  3488 2020-12-16 14:42 test123
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ chmod 640 test123 
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $ ls -la
total 29
drwxrwx--x   4 u0_a158 sdcard_rw  3488 2020-12-16 14:42 .
drwxrwx--x 225 root    sdcard_rw 20480 2020-12-11 11:25 ..
-rw-rw----   1 u0_a158 sdcard_rw     9 2020-12-16 14:39 aje_test
drwxrwx--x   4 u0_a158 sdcard_rw  3488 2020-12-16 13:51 files
drwxrwx--x   2 u0_a158 sdcard_rw  3488 2020-12-16 14:42 test123
blueline:/storage/emulated/0/Android/data/org.mozilla.firefox_beta $

Bob Clary [:bc] (inactive)

Comment 24

•

3 years ago

Cool. I guess it might work but I'm not sure it would be any better than just using the old /mnt/sdcard/test_root approach.

The proof of the pudding so to speak would be to run the Fenix with a profile located on the sdcard.

I think we would have problems with some of the compiled tests and not being able to change file permissions. I'm making good progress on bug 1681439 which does a fallback to /mnt/sdcard for older unrooted Androids. Maybe we can proof it out there before messing with geckodriver.

Greg Mierzwinski [:sparky]

Updated

•

3 years ago

Severity: -- → S2

Priority: -- → P3

Bug 1680407 - [geckodriver] Remove --android-storage argument and always use external storage for profile folder. 3 years ago Henrik Skupin [:whimboo][⌚️UTC+1] 48 bytes, text/x-phabricator-request		Details \| Review
bug_1680407_aerickson_output.txt 3 years ago Andrew Erickson [:aerickson] 7.84 KB, text/plain		Details
Bug 1680407 - [geckodriver] Use $EXTERNAL_STORAGE environment variable to detect sdcard location. 3 years ago Henrik Skupin [:whimboo][⌚️UTC+1] 48 bytes, text/x-phabricator-request		Details \| Review
Bug 1680407 - [mozdevice-rust] Use sdcard as storage by default for rooted and unrooted devices. 3 years ago Henrik Skupin [:whimboo][⌚️UTC+1] 48 bytes, text/x-phabricator-request		Details \| Review