1524673 - Start Marionette in GeckoView when remote debugging is enabled

Status: RESOLVED FIXED

(GeckoView :: General, enhancement, P2)

Description

[Nick Alexander :nalexander [he/him]]

I'm splitting this out of Bug 1496773. In that ticket I had a small patch to unconditionally start Marionette, but I think that's not what we want. I think we want the Marionette service to piggy back the remote debugging service that's already exposed in GV. This ticket tracks discussing this and making it happen.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1524673 - Pre: Allow to toggle remote debugging in the GeckoView example. r?snorp

This is just a testing convenience. Remote debugging is engine-wide,
not session-wide, so it doesn't fit the other actions exactly; but
the "multiprocess" option is also somewhat engine-wide, so it doesn't
seem wildly out of place.

Comment 2

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1524673 - Make Marionette part of remote debugging within GeckoView. r?snorp,whimboo

Functionally, we want Marionette to be enabled whenever remote
debugging enabled and disabled whenever remote debugging is enabled.

That's not particularly well supported by Gecko prefs, so we don't try
to handle all situations. We force the Marionette pref whenever the
remote debugging pref changes; if consumers get themselves into a bad
state by fiddling the Marionette pref independently, that's fine:
GeckoView will take back control eventually.

There are a couple of wrinkles here. The first is that GeckoView and
Marionette race to set themselves up in "profile-after-change". We
ensure that both are configured before GeckoView notifies
"marionette-startup-requested". The second is that the initial value
of the Marionette pref is taken from the environment variable
MOZ_MARIONETTE; therefore, we set that variable when starting the
Gecko thread.

Comment 3

[Nick Alexander :nalexander [he/him]]

whimboo, snorp: it's not clear to me that Phab will have correctly signalled that this series is completely new, so I'm flagging you to ensure you both take another look. Thanks!

Comment 4

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

I think there is nothing to check for me. It looks all like geckoview and Java code related which is outside of Marionette.

Comment 5

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

Nick, to be clear... we are still only waiting for the review of D17580 from James here, right?

Comment 6

[Nick Alexander :nalexander [he/him]]

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

Nick, to be clear... we are still only waiting for the review of D17580 from James here, right?

For this ticket, yes, I think so. Then we need to re-try the "turn it on" patches, and green them up. I haven't tried to dig into Fennec at all yet, but I think it will follow pretty quickly (if we want it to).

I have been bugging snorp, and somehow he cleared the NI but didn't (appear to) review.

I noticed that you didn't review, but I was hoping you could vet the basic approach and make sure that I'm doing reasonable things for starting Marionette. GeckoView:

• sets MOZ_MARIONETTE=1 if remote debugging is enabled (via the GeckoView settings) at Gecko startup
• always notifies 'marionette-startup-requested'
• after that, toggles 'marionette.enabled' to correspond to GeckoView's remote debugging enabled setting

Sensible?

Comment 7

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

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

I noticed that you didn't review, but I was hoping you could vet the basic approach and make sure that I'm doing reasonable things for starting Marionette. GeckoView:

• sets MOZ_MARIONETTE=1 if remote debugging is enabled (via the GeckoView settings) at Gecko startup
• always notifies 'marionette-startup-requested'
• after that, toggles 'marionette.enabled' to correspond to GeckoView's remote debugging enabled setting

Sensible?

Andreas is actually the one who did most of the build system stuff. I wasn't involved at all in that. So I think in this case he is the better person for giving you that feedback.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Sensible?

Looks fine to me, and I r+ed the patch, so hopefully I'm clearing the flag correctly this time :)

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df00bf94c807
Pre: Allow to toggle remote debugging in the GeckoView example. r=snorp
https://hg.mozilla.org/integration/autoland/rev/a971c53ea2c0
Make Marionette part of remote debugging within GeckoView. r=whimboo,snorp

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/df00bf94c807
https://hg.mozilla.org/mozilla-central/rev/a971c53ea2c0

Comment 11

[Andreas Tolfsen ❲:ato❳]

Sorry for my late reply here.

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

I noticed that you didn't review, but I was hoping you could vet
the basic approach and make sure that I'm doing reasonable things
for starting Marionette. GeckoView:

• sets MOZ_MARIONETTE=1 if remote debugging is enabled (via the
  GeckoView settings) at Gecko startup
• always notifies 'marionette-startup-requested'
• after that, toggles 'marionette.enabled' to correspond to
  GeckoView's remote debugging enabled setting

Sensible?

We originally introduced MOZ_MARIONETTE to serve in-process
restarts, as flags passed are not preserved across restarts. This
ensures that Marionette remains enabled after e.g. Services.startup.quit
has been invoked with nsIAppStartup.eRestart.

Your patch checks for FLAG_ENABLE_MARIONETTE and then sets
MOZ_MARIONETTE, in turn enabling the Marionette server. The
servers starts listening for marionette-startup-requested, and
when it is received, the server it initialised.

It should be fine to reuse MOZ_MARIONETTE for this purpose. I’m
happy with the patch, too.