Closed Bug 1680407 Opened 4 years ago Closed 4 years ago

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

Categories

(Testing :: geckodriver, defect, P3)

Default
defect

Tracking

(firefox-esr78 unaffected, firefox86 wontfix, firefox87 wontfix, firefox88 wontfix, firefox89 fixed)

RESOLVED FIXED
89 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox86 --- wontfix
firefox87 --- wontfix
firefox88 --- wontfix
firefox89 --- fixed

People

(Reporter: mcomella, Assigned: whimboo)

References

(Blocks 1 open bug, Regression, )

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

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.

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

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.

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.

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.

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.

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.

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.

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.

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

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

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

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)

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")

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.

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!

(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)
Assignee: tarek → nobody

(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.

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)

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?

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 $

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?

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 $

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.

Severity: -- → S2
Priority: -- → P3

I noticed this problem today and by accident found this bug. So it's not a perftest but a geckodriver specific bug, and as such should have been moved to our geckodriver component.

The regression did indeed start with geckodriver 0.28.0 and was caused by bug 1650891.

I'll have to read through this bug tomorrow to figure out how to improve our auto-detection.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Component: mozperftest → geckodriver
Keywords: regression
Regressed by: 1650891
Summary: ./mach perftest Android VIEW fails on unrooted devices with GeckoDriver 0.28: crash on startup. Blocks testing on Android 11 → Auto detection of --android-storage cause crashes of Firefox for unrooted Android devices

We should block the 0.30.0 release on it.

Blocks: 1686110

So here an update. Agi and myself had a look at this and regarding the external storage there is an environment variable that can be used to retrieve the actual top folder:

adb shell 'echo $EXTERNAL_STORAGE'

The rest is just a hard-coded path. That means we would end-up with a folder like $EXTERNAL_STORAGE/Android/data/org.mozilla.fenix/files for our storage of test related data like the profile.

I tried that path with a Fenix release and beta. Both versions work fine. And even Fenix Nightly, which was crashing before when using the sdcard option, seems to like the external storage path. I cannot get it to crash anymore.

Also this environment variable is around since Android 1, and as such we should always be able to get the path to the external storage. Given that this path will work for both rooted and unrooted devices we could actually get rid of the --android-storage argument and all it's complicated logic. This would be a real win!

Andrew and Michael, would you mind testing the following geckodriver builds? Does it work for you now?

https://treeherder.mozilla.org/jobs?repo=try&revision=b4c94c1237f9d7b00831d29c7345ce58368a7167&searchStr=gd

Thanks

Flags: needinfo?(michael.l.comella)
Flags: needinfo?(aerickson)
Attached file bug_1680407_aerickson_output.txt (deleted) —

I download the OS X artifact in the build linked and moved it into place at ~/.mozbuild/browsertime/node_modules/.bin/browsertime. Version is reported as geckodriver 0.29.0 (b4c94c1237f9 2021-03-22 20:41 +0100).

I updated my Firefox Nightly from the app store (on my Samsung Galaxy s10) and ran mcomella's original test command. It didn't run the test, but not sure if the goal is to just get further. Output attached.

Let me know if I can help with anything else.

Flags: needinfo?(aerickson)

I tested that build on a Moto G5 and it works fine now for me.

(In reply to Andrew Erickson [:aerickson] from comment #30)

I updated my Firefox Nightly from the app store (on my Samsung Galaxy s10) and ran mcomella's original test command. It didn't run the test, but not sure if the goal is to just get further. Output attached.

The output isn't that helpful for me given that it lacks all the geckodriver / marionette trace output. You would have to start geckodriver with -vv or at least set the Firefox preference marionette.log.level to Trace. Nevertheless it looks like that you don't hit the crash anymore, which is good. And it's similar to what Peter sees. As such we should get this patch reviewed and landed, and shipped soon with another geckodriver release. Thanks for testing.

Andrew, and the problem you might run into now is most likely bug 1700290. When you use Fenix Beta or Release it should just work fine.

Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c5708498541f [geckodriver] Remove --android-storage argument and always use external storage for profile folder. r=webdriver-reviewers,agi,jgraham

Backed out for causing linux build bustage in makefiles/rust.mk

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

Push with failures

Failure log

Flags: needinfo?(hskupin)

The test android::test::android_handler_test_root_and_profile_path is run even I marked it as [#ignore]. But that I did only locally, and completely forgot to push the change to Phabricator. Will get this fixed in a minute and push again.

Flags: needinfo?(hskupin)
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b17d8c432b6b [geckodriver] Remove --android-storage argument and always use external storage for profile folder. r=webdriver-reviewers,agi,jgraham
Flags: needinfo?(hskupin)

The problem here is that Browsertime uses --android-storage to force the internal value. And with that patch it's not available anymore.

Those lines need to be removed, but then also the crash report fetching code needs to be updated to point to the external storage. Problem here is that it is based on mozdevice.test_root, and changing that is quite an amount of work. :(

James, another option for the moment could be to just change the default and the sdcard option for --android-storage to point to the external storage, and leave the argument present for now. That way browsertime can still point to the internal location. But then it's not clear when mozdevice (Python) will actually be fixed, and we would have to carry this argument with us for a while. On the other hand it might give users the opportunity to change the location, which might be helpful in case something is still broken with external storage. So maybe keeping the argument for now?

Flags: needinfo?(hskupin) → needinfo?(james)

I was hoping we could get away with just having the argument present but be a no-op. It sounds like that isn't going to work. So yeah, maybe we need to just change the default to match the new/expected behaviour, log a warning when using some other option, and then file a bug to get mozdevice/browsertime to update.

Flags: needinfo?(james)

That check for automatic adding the --android-storage is in 12.0.0-alpha.1 of Browsertime and not stable release, so I can just remove that for now if its make it easier for you (I didn't think it was gonna be fixed so fast). Just let me know and I'll fix that asap.

One thing: what version of Browsertime are the tests running against? I think it would be safer to run against a stable release instead of main as I guess it's doing now?

(In reply to Peter Hedenskog from comment #40)

One thing: what version of Browsertime are the tests running against? I think it would be safer to run against a stable release instead of main as I guess it's doing now?

Greg or Andrew can you please answer?

Flags: needinfo?(gmierz2)
Flags: needinfo?(acreskey)

(In reply to Peter Hedenskog from comment #40)

That check for automatic adding the --android-storage is in 12.0.0-alpha.1 of Browsertime and not stable release, so I can just remove that for now if its make it easier for you (I didn't think it was gonna be fixed so fast). Just let me know and I'll fix that asap.

I will have a patch ready for you tomorrow. Using --android-storage with sdcard should give you the correct path. Does it mean that an earlier release of Browsertime doesn't use the argument at all? I assume that this might be the reason why we internally base on main or the alpha.

Yes earlier version didn't use it, you needed to add it yourself if you wanted to use Geckodriver 0.28 or higher, but the bundled version with Browsertime was 0.27 but I wanted to make it easy for people to use + wanted to use latest and greatest Geckodriver.

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

(In reply to Peter Hedenskog from comment #40)

One thing: what version of Browsertime are the tests running against? I think it would be safer to run against a stable release instead of main as I guess it's doing now?

Greg or Andrew can you please answer?

We're currently running from this commit: https://github.com/sitespeedio/browsertime/commit/138fd2ed14b423f7b3143b7d277b08a69cc787fd
Which is a 3 commits away from the version 11.0.1 release.

Flags: needinfo?(gmierz2)
Flags: needinfo?(acreskey)

Ok, then the --android-storage shouldn't be a problem except if you set it yourself right in your tests? I'll just remove it now from Browsertime to avoid any confusion.

Blocks: 1700559

I'm going to move the currently attached patch to bug 1700559, and will work on a new patch that simply fixes the auto-detection for now and defaults to sdcard instead of app for unrooted devices.

Comment on attachment 9210847 [details]
Bug 1680407 - [geckodriver] Remove --android-storage argument and always use external storage for profile folder.

Revision D109407 was moved to bug 1700559. Setting attachment 9210847 [details] to obsolete.

Attachment #9210847 - Attachment is obsolete: true
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/da67aee25176 [geckodriver] Use $EXTERNAL_STORAGE environment variable to detect sdcard location. r=webdriver-reviewers,jgraham https://hg.mozilla.org/integration/autoland/rev/22a0870b71f8 [mozdevice-rust] Use sdcard as storage by default for rooted and unrooted devices. r=webdriver-reviewers,jgraham
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch
Blocks: 1700557
No longer blocks: 1686110
Regressions: 1703070

Andrew and Michael, would you mind testing the following geckodriver builds? Does it work for you now?

:whimboo mentioned this has already had testers: clearing NI.

Flags: needinfo?(michael.l.comella)
Regressions: 1703489
Regressions: 1721374
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: