Closed Bug 1577850 Opened 5 months ago Closed 4 months ago

Use GeckoView configuration YAML file in geckodriver

Categories

(Testing :: geckodriver, enhancement, P1)

Version 3
enhancement

Tracking

(firefox71 fixed)

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: nalexander, Assigned: whimboo)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(2 files)

This ticket tracks making geckodriver configure GeckoView vehicles using a configuration YAML file. This is required to make browsertime and geckodriver target Fenix: right now, we target Fenix using Intent arguments and a special activity. Unfortunately, it no longer works and the BrowserPerformanceTestActivity is slated to go away.

This is Point 4 of https://bugzilla.mozilla.org/show_bug.cgi?id=1525126#c20 but I want to be able to refer to by URL.

See also Bug 1552519, which did this switch for existing test suites, and Bug 1574849, which is adding it to mozdevice.

Priority: -- → P3

This makes geckodriver configure GeckoView vehicles using a
configuration YAML file. This is the blessed way to configure such
vehicles
.

The old way based on intent arguments is kept for Fennec/Firefox for
Android and as an opt-in. Fennec/Firefox is defined as: any Android
package with name starting with "org.mozilla.{fennec,firefox}".

The configuration file is still pushed in this case for simplicity and
transparency, but it will be ignored by the relevant vehicle.

jesup: the geckodriver binaries produced by https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a42bbdfc9a490ae4147aa62100fd7568c8e4c37 should be able to drive Fenix. Use --firefox.geckodriverPath=... to override the path. Let me know if they work!

Flags: needinfo?(rjesup)

acreskey: just FYI, this stack (or the binaries in the try push above) are what's needed to drive Fenix from browsertime. Not sure what you and Colin arranged, but this might help.

Flags: needinfo?(acreskey)

nalexander: the geckodriver binary in your try push does indeed drive Fenix (org.mozilla.fenix.performancetest) from browsertime. Thank you.

Flags: needinfo?(acreskey)

(In reply to Andrew Creskey from comment #4)

nalexander: the geckodriver binary in your try push does indeed drive Fenix (org.mozilla.fenix.performancetest) from browsertime. Thank you.

Sweet! Thanks for confirming!

(In reply to Nick Alexander :nalexander [he/him] from comment #5)

(In reply to Andrew Creskey from comment #4)

nalexander: the geckodriver binary in your try push does indeed drive Fenix (org.mozilla.fenix.performancetest) from browsertime. Thank you.

Sweet! Thanks for confirming!

Oh! Just FYI, this should drive org.mozilla.fenix as well, as long as you set it to be the current Android "debug app". See https://mozilla.github.io/geckoview/consumer/docs/automation#controlling-configuration-from-a-file.

Blocks: 1573798

Nick, to be able to get started on this bug I would need some additional information first:

  1. How does such a config file look like? Mind attaching one which you make use of most of the time? If it's hard to understand maybe you can even add inline comments?

  2. I assume we want to take the non-debuggable app path in informing GeckoView to read the configuration file? Means there is nothing specifically to set?

  3. if intent arguments AND a config file is set, I assume the intent arguments aren't obeyed at all? Or will we have to push them anyway through the command line options?

Flags: needinfo?(rjesup) → needinfo?(nalexander)
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P3 → P1
Depends on: 1584400

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

Nick, to be able to get started on this bug I would need some additional information first:

Hey! My original patch worked but needed significant rebasing (and a little reworking), which I just submitted. Take a look; this one probably doesn't need you to commandeer it. (But you're welcome to if you want to make changes yourself.)

  1. How does such a config file look like? Mind attaching one which you make use of most of the time? If it's hard to understand maybe you can even add inline comments?

Sure. The documentation is at https://mozilla.github.io/geckoview/consumer/docs/automation, but such a file looks like:

$ adb shell cat /data/local/tmp/org.mozilla.fenix.performancetest-geckoview-config.yaml
## GeckoView configuration YAML auto-generated by geckodriver.
## See https://mozilla.github.io/geckoview/consumer/docs/automation.
args:
    - --marionette
    - --profile
    - "/mnt/sdcard/org.mozilla.fenix.performancetest-geckodriver-profile"

You can also set environment variables: env: {"VAR": "VAL"} and prefs: prefs: {"pref": val}. (GeckoView itself knows how to handle these.) So MOZ_LOG could be configured with this configuration, allowing to get logs from the device at some point in the future.

  1. I assume we want to take the non-debuggable app path in informing GeckoView to read the configuration file? Means there is nothing specifically to set?

I think we do want to adb shell am set-debug-app; otherwise one can't work with release Fenix.

  1. if intent arguments AND a config file is set, I assume the intent arguments aren't obeyed at all? Or will we have to push them anyway through the command line options?

This is vehicle dependent, but at this time there is no conflict, and in the future no vehicle should use intent arguments. (My patch sets them only for Fenix and Fennec, or when explicitly requested as an option.). Fennec only knows about intent arguments. Fenix only knows about configuration files. GVE (and TRA, I think) know about both but then GV itself merges the two methods.

We should remove both those accommodations to Fennec: Bug 1584400. In any case, this works (albeit with duplicated arguments) so we don't need to block landing on that ticket.

Finally, to test this in an x86 emulator:

$ ./mach android-emulator --version=x86-7.0 &
$ wget https://index.taskcluster.net/v1/task/project.mobile.fenix.v2.performance-test.latest/artifacts/public/build/x86/geckoNightly/target.apk
$ adb install target.apk
$ ./mach browsertime -- --firefox.geckodriverPath /Users/nalexander/Mozilla/gecko/target/debug/geckodriver --android https://reddit.com -n 1 -v --firefox.android.package 'org.mozilla.fenix.performancetest' --firefox.android.activity='org.mozilla.fenix.IntentReceiverActivity' -vv --firefox.android.intentArgument=-d --firefox.android.intentArgument='about:blank' --firefox.android.intentArgument=-a --firefox.android.intentArgument=android.intent.action.VIEW

We must use org.mozilla.fenix.performancetest in order to have remote debugging enabled out of the box. (I'm not 100% on this; I usually test against the performance variant.)

The activity is required because Fenix handles intents in such a way that the autodetect fails:

$ adb shell cmd package resolve-activity --brief org.mozilla.fenix.performancetest
priority=0 preferredOrder=0 match=0x108000 specificIndex=-1 isDefault=false
org.mozilla.fenix.performancetest/org.mozilla.fenix.HomeActivity

(HomeActivity doesn't handle intents in Fenix.).

All the other intent arguments are needed to work around Fenix not actually displaying a tab out of the box. By using VIEW with a URL, we get a functioning Gecko, which we can then redirect via Marionette.

Flags: needinfo?(nalexander)

(In reply to Nick Alexander :nalexander [he/him] from comment #8)

  1. How does such a config file look like? Mind attaching one which you make use of most of the time? If it's hard to understand maybe you can even add inline comments?

Sure. The documentation is at https://mozilla.github.io/geckoview/consumer/docs/automation, but such a file looks like:

$ adb shell cat /data/local/tmp/org.mozilla.fenix.performancetest-geckoview-config.yaml
## GeckoView configuration YAML auto-generated by geckodriver.
## See https://mozilla.github.io/geckoview/consumer/docs/automation.
args:
    - --marionette
    - --profile
    - "/mnt/sdcard/org.mozilla.fenix.performancetest-geckodriver-profile"

Ok, so it means that we basically duplicate the application arguments, and write out this file with some name to the temp folder? How is eg. Fenix instructed to load the config from this specific path? In your updated patch I can only see format!("am set-debug-app --persistent {}", process.package), but that has no path nor the suffix from above.

You can also set environment variables: env: {"VAR": "VAL"} and prefs: prefs: {"pref": val}. (GeckoView itself knows how to handle these.) So MOZ_LOG could be configured with this configuration, allowing to get logs from the device at some point in the future.

Does it mean that we should also also write out all the prefs which are getting set via moz:firefoxOptions.prefs? For env we have some crashreporter variables which might also be helpful to set, or do those not work?

https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/testing/geckodriver/src/marionette.rs#250

  1. I assume we want to take the non-debuggable app path in informing GeckoView to read the configuration file? Means there is nothing specifically to set?

I think we do want to adb shell am set-debug-app; otherwise one can't work with release Fenix.

Ok.

  1. if intent arguments AND a config file is set, I assume the intent arguments aren't obeyed at all? Or will we have to push them anyway through the command line options?

This is vehicle dependent, but at this time there is no conflict, and in the future no vehicle should use intent arguments. (My patch sets them only for Fenix and Fennec, or when explicitly requested as an option.). Fennec only knows about intent arguments. Fenix only knows about configuration files. GVE (and TRA, I think) know about both but then GV itself merges the two methods.

Just to make the patch easier, could we always write the config AND use the intent arguments for the time being? If it doesn't give a conflict we shouldn't run into problems that way.

In terms of generating the file I'm not that happy to have it in geckodriver itself. mozrunner-rust might be a better place to handle the Android application logic, and would make it re-usable for other consuming code. But that package doesn't have Android support yet. For simplicity we could land in geckodriver for now, and then have a follow-up to move all the Android specific logic into mozrunner-rust.

> $ ./mach android-emulator --version=x86-7.0 &
> $ wget https://index.taskcluster.net/v1/task/project.mobile.fenix.v2.performance-test.latest/artifacts/public/build/x86/geckoNightly/target.apk
> $ adb install target.apk
> $ ./mach browsertime -- --firefox.geckodriverPath /Users/nalexander/Mozilla/gecko/target/debug/geckodriver --android https://reddit.com -n 1 -v --firefox.android.package 'org.mozilla.fenix.performancetest' --firefox.android.activity='org.mozilla.fenix.IntentReceiverActivity' -vv --firefox.android.intentArgument=-d --firefox.android.intentArgument='about:blank' --firefox.android.intentArgument=-a --firefox.android.intentArgument=android.intent.action.VIEW
> ```
> 
> We must use `org.mozilla.fenix.performancetest` in order to have remote debugging enabled out of the box.  (I'm not 100% on this; I usually test against the performance variant.)
> 
> The `activity` is required because Fenix handles intents in such a way that the autodetect fails:
> ```
> $ adb shell cmd package resolve-activity --brief org.mozilla.fenix.performancetest
> priority=0 preferredOrder=0 match=0x108000 specificIndex=-1 isDefault=false
> org.mozilla.fenix.performancetest/org.mozilla.fenix.HomeActivity
> ```
> (`HomeActivity` doesn't handle intents in Fenix.).
>
> All the other intent arguments are needed to work around [Fenix not actually displaying a tab out of the box](https://github.com/mozilla-mobile/fenix/issues/3247).  By using `VIEW` with a URL, we get a functioning Gecko, which we can then redirect via Marionette.

Above you wrote that Fenix doesn't support intent arguments at all, so why do we request setting those via this activity, and not only via the configuration file?
Flags: needinfo?(nalexander)

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

(In reply to Nick Alexander :nalexander [he/him] from comment #8)

  1. How does such a config file look like? Mind attaching one which you make use of most of the time? If it's hard to understand maybe you can even add inline comments?

Sure. The documentation is at https://mozilla.github.io/geckoview/consumer/docs/automation, but such a file looks like:

$ adb shell cat /data/local/tmp/org.mozilla.fenix.performancetest-geckoview-config.yaml
## GeckoView configuration YAML auto-generated by geckodriver.
## See https://mozilla.github.io/geckoview/consumer/docs/automation.
args:
    - --marionette
    - --profile
    - "/mnt/sdcard/org.mozilla.fenix.performancetest-geckodriver-profile"

Ok, so it means that we basically duplicate the application arguments, and write out this file with some name to the temp folder? How is eg. Fenix instructed to load the config from this specific path? In your updated patch I can only see format!("am set-debug-app --persistent {}", process.package), but that has no path nor the suffix from above.

This functionality is built into GeckoView. See Bug 1533385 and this code.

This is exactly the approach that Chrome and WebView on Android take.

You can also set environment variables: env: {"VAR": "VAL"} and prefs: prefs: {"pref": val}. (GeckoView itself knows how to handle these.) So MOZ_LOG could be configured with this configuration, allowing to get logs from the device at some point in the future.

Does it mean that we should also also write out all the prefs which are getting set via moz:firefoxOptions.prefs? For env we have some crashreporter variables which might also be helpful to set, or do those not work?

https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/testing/geckodriver/src/marionette.rs#250

We could do so, but it's not clear that there's an advantage. Were we trying to not push a Gecko profile to device at all, then yes, this would make sense, but I don't see us getting there any time soon. There's a lot of geckodriver configuration in the profile, and even more (things like certificates) that geckodriver doesn't manage; it's not worth growing some Android-specific way to specify it. Environment variables are special, since they have to be handled by GeckoView for lifecycle reasons. Prefs is just a convenience thing, so that devs (and things like mach run) can set a few prefs without having to use mozprofile explicitly.

I don't know about the crashreporter variables: I expect setting them on Android might be helpful in some situations.

  1. if intent arguments AND a config file is set, I assume the intent arguments aren't obeyed at all? Or will we have to push them anyway through the command line options?

This is vehicle dependent, but at this time there is no conflict, and in the future no vehicle should use intent arguments. (My patch sets them only for Fenix and Fennec, or when explicitly requested as an option.). Fennec only knows about intent arguments. Fenix only knows about configuration files. GVE (and TRA, I think) know about both but then GV itself merges the two methods.

Just to make the patch easier, could we always write the config AND use the intent arguments for the time being? If it doesn't give a conflict we shouldn't run into problems that way.

Yes. Take out the guard option/variable if you like to do this.

In terms of generating the file I'm not that happy to have it in geckodriver itself. mozrunner-rust might be a better place to handle the Android application logic, and would make it re-usable for other consuming code. But that package doesn't have Android support yet. For simplicity we could land in geckodriver for now, and then have a follow-up to move all the Android specific logic into mozrunner-rust.

I agree, this isn't really a geckodriver responsibility. I considered making a launch_geckoview sibling to launch that knew about this stuff in mozdevice; maybe that's a better place for the moment? Your call.

> $ ./mach android-emulator --version=x86-7.0 &
> $ wget https://index.taskcluster.net/v1/task/project.mobile.fenix.v2.performance-test.latest/artifacts/public/build/x86/geckoNightly/target.apk
> $ adb install target.apk
> $ ./mach browsertime -- --firefox.geckodriverPath /Users/nalexander/Mozilla/gecko/target/debug/geckodriver --android https://reddit.com -n 1 -v --firefox.android.package 'org.mozilla.fenix.performancetest' --firefox.android.activity='org.mozilla.fenix.IntentReceiverActivity' -vv --firefox.android.intentArgument=-d --firefox.android.intentArgument='about:blank' --firefox.android.intentArgument=-a --firefox.android.intentArgument=android.intent.action.VIEW
> ```
> 
> We must use `org.mozilla.fenix.performancetest` in order to have remote debugging enabled out of the box.  (I'm not 100% on this; I usually test against the performance variant.)
> 
> The `activity` is required because Fenix handles intents in such a way that the autodetect fails:
> ```
> $ adb shell cmd package resolve-activity --brief org.mozilla.fenix.performancetest
> priority=0 preferredOrder=0 match=0x108000 specificIndex=-1 isDefault=false
> org.mozilla.fenix.performancetest/org.mozilla.fenix.HomeActivity
> ```
> (`HomeActivity` doesn't handle intents in Fenix.).
>
> All the other intent arguments are needed to work around [Fenix not actually displaying a tab out of the box](https://github.com/mozilla-mobile/fenix/issues/3247).  By using `VIEW` with a URL, we get a functioning Gecko, which we can then redirect via Marionette.

Above you wrote that Fenix doesn't support intent arguments at all, so why do we request setting those via this activity, and not only via the configuration file?

Fenix doesn't support intent arguments for configuring GeckoView. The intent arguments above are partially processed by Android (-a, -d) and partially processed by Fenix (showing Web Content instead of the Home Activity).

Depends on: 1533385
Flags: needinfo?(nalexander)

(In reply to Nick Alexander :nalexander [he/him] from comment #10)

You can also set environment variables: env: {"VAR": "VAL"} and prefs: prefs: {"pref": val}. (GeckoView itself knows how to handle these.) So MOZ_LOG could be configured with this configuration, allowing to get logs from the device at some point in the future.

Does it mean that we should also also write out all the prefs which are getting set via moz:firefoxOptions.prefs? For env we have some crashreporter variables which might also be helpful to set, or do those not work?

https://searchfox.org/mozilla-central/rev/f1e99da78fe6c3c68696358dac06aed90f8112d3/testing/geckodriver/src/marionette.rs#250

We could do so, but it's not clear that there's an advantage. Were we trying to not push a Gecko profile to device at all, then yes, this would make sense, but I don't see us getting there any time soon. There's a lot of geckodriver configuration in the profile, and even more (things like certificates) that geckodriver doesn't manage; it's not worth growing some Android-specific way to specify it. Environment variables are special, since they have to be handled by GeckoView for lifecycle reasons. Prefs is just a convenience thing, so that devs (and things like mach run) can set a few prefs without having to use mozprofile explicitly.

Please note that there are still preferences which cannot be changed, even when those are part of the profile. See bug 1547717 for details. What if eg. media.autoplay.default should be overwritten? Would that be possible by using the config file? If that would be the case we should indeed write out preferences to that file. In that regards I also wonder about other preferences geckodriver sets by default, and which of those cannot be set via the profile.

I don't know about the crashreporter variables: I expect setting them on Android might be helpful in some situations.

I could try to run a crash test with geckodriver similar to what we have for Marionette in test_crash.py and check if the minidump is successfully created and not crashreporter is shown.

  1. if intent arguments AND a config file is set, I assume the intent arguments aren't obeyed at all? Or will we have to push them anyway through the command line options?

This is vehicle dependent, but at this time there is no conflict, and in the future no vehicle should use intent arguments. (My patch sets them only for Fenix and Fennec, or when explicitly requested as an option.). Fennec only knows about intent arguments. Fenix only knows about configuration files. GVE (and TRA, I think) know about both but then GV itself merges the two methods.

Just to make the patch easier, could we always write the config AND use the intent arguments for the time being? If it doesn't give a conflict we shouldn't run into problems that way.

Yes. Take out the guard option/variable if you like to do this.

Great. So I will do so.

In terms of generating the file I'm not that happy to have it in geckodriver itself. mozrunner-rust might be a better place to handle the Android application logic, and would make it re-usable for other consuming code. But that package doesn't have Android support yet. For simplicity we could land in geckodriver for now, and then have a follow-up to move all the Android specific logic into mozrunner-rust.

I agree, this isn't really a geckodriver responsibility. I considered making a launch_geckoview sibling to launch that knew about this stuff in mozdevice; maybe that's a better place for the moment? Your call.

It's not also mozdevice-rust. Given the problems in bug 1584530 we have to release a new version of geckodriver soon. So I would say we take the simply approach for now, and have it all in android.rs, and later move all that code to mozrunner-rust.

> $ ./mach android-emulator --version=x86-7.0 &
> $ wget https://index.taskcluster.net/v1/task/project.mobile.fenix.v2.performance-test.latest/artifacts/public/build/x86/geckoNightly/target.apk
> $ adb install target.apk
> $ ./mach browsertime -- --firefox.geckodriverPath /Users/nalexander/Mozilla/gecko/target/debug/geckodriver --android https://reddit.com -n 1 -v --firefox.android.package 'org.mozilla.fenix.performancetest' --firefox.android.activity='org.mozilla.fenix.IntentReceiverActivity' -vv --firefox.android.intentArgument=-d --firefox.android.intentArgument='about:blank' --firefox.android.intentArgument=-a --firefox.android.intentArgument=android.intent.action.VIEW
> ```
> 
> We must use `org.mozilla.fenix.performancetest` in order to have remote debugging enabled out of the box.  (I'm not 100% on this; I usually test against the performance variant.)
> 
> The `activity` is required because Fenix handles intents in such a way that the autodetect fails:
> ```
> $ adb shell cmd package resolve-activity --brief org.mozilla.fenix.performancetest
> priority=0 preferredOrder=0 match=0x108000 specificIndex=-1 isDefault=false
> org.mozilla.fenix.performancetest/org.mozilla.fenix.HomeActivity
> ```
> (`HomeActivity` doesn't handle intents in Fenix.).
>
> All the other intent arguments are needed to work around [Fenix not actually displaying a tab out of the box](https://github.com/mozilla-mobile/fenix/issues/3247).  By using `VIEW` with a URL, we get a functioning Gecko, which we can then redirect via Marionette.

Above you wrote that Fenix doesn't support intent arguments at all, so why do we request setting those via this activity, and not only via the configuration file?

Fenix doesn't support intent arguments for configuring GeckoView. The intent arguments above are partially processed by Android (-a, -d) and partially processed by Fenix (showing Web Content instead of the Home Activity).

Hm, ok. So those are two levels then. One for the operating system, and then the embedding package (activity), but not GV. Thanks!

Attachment #9090241 - Attachment description: Bug 1577850 - Use GeckoView configuration YAML file in geckodriver. r?whimboo → Bug 1577850 - Use GeckoView configuration YAML file in geckodriver.
Blocks: 1585120

I retested the latest patch (which just got r+ from jgraham).

Targeting Fenix (org.mozilla.fenix.performancetest) succeeded, via

$ ./mach browsertime -- --firefox.geckodriverPath /Users/nalexander/Mozilla/gecko/target/debug/geckodriver --android https://reddit.com -n 2 -v --firefox.android.package 'org.mozilla.fenix.performancetest' --firefox.android.activity='org.mozilla.fenix.IntentReceiverActivity' -vv --firefox.android.intentArgument=-d --firefox.android.intentArgument='about:blank' --firefox.android.intentArgument=-a --firefox.android.intentArgument=android.intent.action.VIEW

Targeting Fennec from esr68, namely:

$ wget https://queue.taskcluster.net/v1/task/H58QzPYdS3yu6V5JfobwrQ/runs/0/artifacts/public/build/target.apk -Ofennec.esr68.x86.apk
$ adb install fennec.esr68.x86.apk
$ ./mach browsertime -- --firefox.geckodriverPath /Users/nalexander/Mozilla/gecko/target/debug/geckodriver --android https://reddit.com -n 2 -v --firefox.android.package 'org.mozilla.firefox' --firefox.android.activity 'org.mozilla.gecko.BrowserApp' -vv

also succeeded.

Bombs away!

I just noticed that the last try build was failing with:

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=269123987&repo=try&lineNumber=346

error: no matching package named serde_yaml found

Turned out that I missed to vendor the third party packages, and here specifically the dependencies of serde_yaml. I will push a follow-up revision with the appropriate changes in a moment. Here the appropriate try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=749d222469d0d5f8ed5d969c12a82dc41a780daf

serde_yaml has been added to geckodriver, and dependencies
under third_party/rust needs to be updated.

Depends on D44578

Blocks: 1585484
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22097cc6f281
Use GeckoView configuration YAML file in geckodriver. r=nalexander,webdriver-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/55e516f123f7
Vendor dependencies for serde_yaml crate. r=webdriver-reviewers,jgraham
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.