1698968 - Enable Marionette and Remote Agent without having to enable remote debugging

Status: RESOLVED FIXED

(GeckoView :: General, defect, P2)

Description

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

When trying to run tests via geckodriver for Firefox on Android, Marionette doesn't accept connections until the "Remote debugging via USB" option has been enabled in the preferences of Firefox.

Not sure if that maps to devtools.debugger.remote-enabled, and why this is necessary now. For Fennec I've never had to set this preference to true.

Agi, could you help and shed some light on that changed behavior?

Comment 1

[Julian Descottes [:jdescottes]]

(as mentioned on elements)

If I understand correctly, the marionette listener is set from the onInit of GeckoViewRemoteDebugger (https://searchfox.org/mozilla-central/source/mobile/android/modules/geckoview/GeckoViewRemoteDebugger.jsm#57-65)

But GeckoViewRemoteDebugger is lazy-loaded at: https://searchfox.org/mozilla-central/rev/1a47a74bd5ba89f2474aa27c40bd478e853f3276/mobile/android/components/geckoview/GeckoViewStartup.jsm#188-201

        GeckoViewUtils.addLazyGetter(this, "GeckoViewRemoteDebugger", {
          module: "resource://gre/modules/GeckoViewRemoteDebugger.jsm",
          init: gvrd => gvrd.onInit(),
        });

        GeckoViewUtils.addLazyPrefObserver(
          {
            name: "devtools.debugger.remote-enabled",
            default: false,
          },
          {
            handler: _ => this.GeckoViewRemoteDebugger,
          }
        );

It isn't clear how this gets enabled in tests at the moment though.

Comment 2

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

This is kinda unfortunate given that both protocols don't have anything in common. I'm happy to just add the preference to Marionette's recommended preferences so that users of geckodriver can start testing Fenix, but I would like to get some feedback first how safe it would be. I don't want to enable services that actually aren't needed by Marionette, and might cause security issues.

Comment 3

[[ex-Mozilla] Agi Sferro | :agi]

GeckoView enables remote Debugging only when remoteDebuggingEnabled is set from the app, and that maps to devtools.debugger.remote-enabled https://searchfox.org/mozilla-central/rev/1a47a74bd5ba89f2474aa27c40bd478e853f3276/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java#479-480

How do you start Firefox with geckodriver? do you pass any special data in the intent? you will have to pass something to Firefox to enable remote debugging, although we have to figure out the security consequences of that.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

One thing that would be easy to do is to enable debugging in non-signed builds on the Fenix side, as I'm assuming you don't run tests on the official build.

Comment 5

[[ex-Mozilla] Agi Sferro | :agi]

Actually we can handle this as part of Bug 1547717! After that's done you will be able to set the pref in the yml configuration file without affecting end users.

Comment 6

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

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #3)

GeckoView enables remote Debugging only when remoteDebuggingEnabled is set from the app, and that maps to `devtools.debugger.remote-

Please note that Remote Debugging is used for the Firefox DevTools. Marionette's remote debugging protocol is completely separate, and has nothing to do with RDP.

How do you start Firefox with geckodriver? do you pass any special data in the intent? you will have to pass something to Firefox to enable remote debugging, although we have to figure out the security consequences of that.

To start Firefox (or any other app) on Android we pass in the --marionette argument as part of the .yml config file:
https://searchfox.org/mozilla-central/rev/f07a609a76136ef779c65185165ff5ac513cc172/testing/geckodriver/src/android.rs#254-262

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #4)

One thing that would be easy to do is to enable debugging in non-signed builds on the Fenix side, as I'm assuming you don't run tests on the official build.

Official builds need to be supported because these are used by webdevs for testing their websites via Selenium/geckodriver. So we need a solution that fits all kinds of builds that we offer.

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #5)

Actually we can handle this as part of Bug 1547717! After that's done you will be able to set the pref in the yml configuration file without affecting end users.

Ideally we do not want to set this preference at all. Note that we also haven't had to do it for Fennec before. So I assume it was just a misinterpretation that this pref controls Marionette's socket server.

Comment 7

[[ex-Mozilla] Agi Sferro | :agi]

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

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #3)

GeckoView enables remote Debugging only when remoteDebuggingEnabled is set from the app, and that maps to `devtools.debugger.remote-

Please note that Remote Debugging is used for the Firefox DevTools. Marionette's remote debugging protocol is completely separate, and has nothing to do with RDP.

I see, that makes sense! OK so maybe this is just a misunderstanding on our part.

How do you start Firefox with geckodriver? do you pass any special data in the intent? you will have to pass something to Firefox to enable remote debugging, although we have to figure out the security consequences of that.

To start Firefox (or any other app) on Android we pass in the --marionette argument as part of the .yml config file:
https://searchfox.org/mozilla-central/rev/f07a609a76136ef779c65185165ff5ac513cc172/testing/geckodriver/src/android.rs#254-262

OK this should be enough.

Ideally we do not want to set this preference at all. Note that we also haven't had to do it for Fennec before. So I assume it was just a misinterpretation that this pref controls Marionette's socket server.

Yep I understand now. Let me take a look at what's going on in the code.

Comment 8

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

Thanks for the feedback Agi and having a look into it! We appreciate.

While we do not have a solution for it yet, and it will need to ride the train to release too, I would like to add this behavior to the known issues section of the geckodriver release notes. So my question would be (just to be 100% sure) is it enough to set the devtools.debugger.remote-enabled pref in the user.js of the Firefox profile before Firefox gets started?

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

I mentioned on matrix, but I'm not sure, I think the best way is to test it. IIUC it should work.

Comment 10

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

Yes, that works. As such I made a comment on bug 1699066.

Agi, given that this is more a bug in Fenix, should we move it to some Fenix component? That's the place where we would have to fix it anyway.

Comment 11

[[ex-Mozilla] Agi Sferro | :agi]

Mh so the change to start marionette even when remote debugging is not enabled is a GeckoView change, so we should move the bug there, unless I'm misunderstanding.

Comment 12

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

Ok, if it's GeckoView then fine with me. It's interesting that geckoview_example works, and as such I thought it's Fenix related.

Would someone has the time to help us getting this fixed?

Comment 13

[[ex-Mozilla] Agi Sferro | :agi]

Nick, I see that enabling marionette only when the remote debugging pref is set was done intentionally in Bug 1524673, I cannot find the reasoning behind this, do you remember?

I'm assuming is to avoid exposing normal users to attacks through the marionette API? or is there another reason?

Comment 14

[Nick Alexander :nalexander [he/him]]

(In reply to Agi Sferro | :agi | ni? for questions | ⏰ PST | he/him from comment #13)

Nick, I see that enabling marionette only when the remote debugging pref is set was done intentionally in Bug 1524673, I cannot find the reasoning behind this, do you remember?

The details? No, and a quick skim of relevant tickets reveals no hints.

I'm assuming is to avoid exposing normal users to attacks through the marionette API? or is there another reason?

To the best of my recollection, yes. What I recall is:

1. we didn't have a way to set prefs for GV consumers, since the YAML config work didn't exist at that time. (The ticket in question is 02/2019; the YAML stuff is 04/2019.)
2. we didn't want to have a new UI setting for Marionette/remote control;
3. we didn't feel that starting Marionette unconditionally was sensible. (Desktop takes some pains to ensure it's not running; at the time, I think Android only shipped Marionette on Nightly, mostly for security reasons.)

Out of that context we decided to use the existing remote debugging setting, since it provides similar capability to an attacker: connect to a port, drive the browser to execute privileged actions.

Comment 15

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

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

1. we didn't have a way to set prefs for GV consumers, since the YAML config work didn't exist at that time. (The ticket in question is 02/2019; the YAML stuff is 04/2019.)

Ok, so since the YAML config support is available we push a config file with the --marionette argument.

3. we didn't feel that starting Marionette unconditionally was sensible. (Desktop takes some pains to ensure it's not running; at the time, I think Android only shipped Marionette on Nightly, mostly for security reasons.)

No, Android didn't ship only on Nightly. With the original Android support for geckodriver the Firefox for Android (Fennec) package was very well testable for WebDriver clients.

Out of that context we decided to use the existing remote debugging setting, since it provides similar capability to an attacker: connect to a port, drive the browser to execute privileged actions.

So it indeed looks like it's not necessary at all anymore, and that we can finally unbundle it from the remote debugger settings. Thanks!

Comment 16

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

Agi, will you be able to help us with this required patch? Thanks a lot.

Comment 17

[[ex-Mozilla] Agi Sferro | :agi]

Sure! I added it to the bugs proposed for next sprint (starts on the 14th)

Comment 18

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

On bug 1676803 we will enable the Remote Agent for Android. This component currently handles our partial implementation of CDP and will be used soon for our WebDriver BiDi implementation. It's similarly initialized and adds its own observer notification right now to Marionette.

This means the change on this bug will require to move both entries to a different spot.

Agi, if you could tell me where we should call the observers I could come up with a patch.

Comment 19

[[ex-Mozilla] Agi Sferro | :agi]

Sure! I think the easiest way to support this in GV would be to add a package protected GeckoViewRuntimeSettings marionetteEnabled, similar to this: https://searchfox.org/mozilla-central/rev/0b90e582d2f592a30713bafc55bfeb0e39e1a1fa/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java#1253-1256

although instead of a pref it can just be a boolean, like this: https://searchfox.org/mozilla-central/rev/0b90e582d2f592a30713bafc55bfeb0e39e1a1fa/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntimeSettings.java#534

when -marionette is passed in, you can flip the setting that you added above, here: https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoRuntime.java#297

And then fire the event here (when ModuleManager.settings.marionetteEnabled === true) https://searchfox.org/mozilla-central/rev/0b90e582d2f592a30713bafc55bfeb0e39e1a1fa/mobile/android/chrome/geckoview/geckoview.js#760

Alternatively, you could make GeckoViewStartup a command line handler, and read the command line from there, although I'm not sure if the marionette event needs to be called after the first window is created, which could get hairy, as we don't really have a way to set stuff in GeckoViewStartup to be read by geckoview.js.

Does this make sense?

Comment 20

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Bug 1698968 - Always send marionette-startup-requested

This also sets the pref on GeckoView whenever --marionette is passed in as a
command line argument.

Comment 21

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

Attachment: test_geckodriver.py

Agi, with the attached script you can verify that both Marionette and Remote Agent are active and running. I hope that helps you to get the remaining issue more easily fixed.

Comment 22

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

Attachment: test_geckodriver.py

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c1f68f8e17c
Always send marionette-startup-requested r=whimboo,aklotz,webdriver-reviewers

Comment 24

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/3c1f68f8e17c