Auto detection of --android-storage cause crashes of Firefox for unrooted Android devices
Categories
(Testing :: geckodriver, defect, P3)
Tracking
(firefox-esr78 unaffected, firefox86 wontfix, firefox87 wontfix, firefox88 wontfix, firefox89 fixed)
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.
Reporter | ||
Comment 1•4 years ago
|
||
I get the perma-crash behavior on my Pixel 2 with geckodriver 0.28 too.
Reporter | ||
Comment 2•4 years ago
|
||
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.
Reporter | ||
Comment 3•4 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.
Reporter | ||
Comment 4•4 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.
Reporter | ||
Comment 5•4 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.
Reporter | ||
Comment 6•4 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.
Reporter | ||
Comment 7•4 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.
Reporter | ||
Comment 8•4 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.
Reporter | ||
Comment 10•4 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.
Reporter | ||
Comment 11•4 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
Comment 12•4 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•4 years ago
|
Reporter | ||
Comment 13•4 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.
Reporter | ||
Comment 14•4 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")
Comment 15•4 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.
Reporter | ||
Comment 16•4 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 implyAndroid/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!
Comment 17•4 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 requiresManifest.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?
Updated•4 years ago
|
Comment 18•4 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 requiresManifest.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.
Comment 19•4 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:/ $
Comment 20•4 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?
Comment 21•4 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 $
Comment 22•4 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?
Comment 23•4 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 $
Comment 24•4 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.
Updated•4 years ago
|
Assignee | ||
Comment 25•4 years ago
|
||
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 | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 27•4 years ago
|
||
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!
Assignee | ||
Comment 28•4 years ago
|
||
Andrew and Michael, would you mind testing the following geckodriver builds? Does it work for you now?
Thanks
Assignee | ||
Comment 29•4 years ago
|
||
Comment 30•4 years ago
|
||
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.
Assignee | ||
Comment 32•4 years ago
|
||
(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.
Comment 33•4 years ago
|
||
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
Comment 34•4 years ago
|
||
Backed out for causing linux build bustage in makefiles/rust.mk
Backout link: https://hg.mozilla.org/integration/autoland/rev/20bfc68a9fef1d0ee4d4fbab2ea15eed057ddcf8
Assignee | ||
Comment 35•4 years ago
|
||
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.
Comment 36•4 years ago
|
||
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
Comment 37•4 years ago
|
||
Backed out for causing gecko browsertime failures
Backout link: https://hg.mozilla.org/integration/autoland/rev/35f132446d615a2310c5778c237658a810068829
Assignee | ||
Comment 38•4 years ago
|
||
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?
Comment 39•4 years ago
|
||
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.
Comment 40•4 years ago
|
||
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?
Assignee | ||
Comment 41•4 years ago
|
||
(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?
Assignee | ||
Comment 42•4 years ago
|
||
(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.
Comment 43•4 years ago
|
||
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.
Comment 44•4 years ago
|
||
(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.
Comment 45•4 years ago
|
||
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.
Assignee | ||
Comment 47•4 years ago
|
||
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 48•4 years ago
|
||
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.
Assignee | ||
Comment 49•4 years ago
|
||
Comment 51•4 years ago
|
||
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
Comment 52•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/da67aee25176
https://hg.mozilla.org/mozilla-central/rev/22a0870b71f8
Updated•4 years ago
|
Reporter | ||
Comment 53•4 years ago
|
||
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.
Description
•