Closed Bug 1524673 Opened 7 months ago Closed 6 months ago

Start Marionette in GeckoView when remote debugging is enabled

Categories

(GeckoView :: General, enhancement, P2)

enhancement

Tracking

(firefox67 fixed)

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.

Component: web-platform-tests → General
Product: Testing → GeckoView
Version: Version 3 → unspecified
Blocks: 1525126

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.

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.

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!

Flags: needinfo?(snorp)
Flags: needinfo?(hskupin)
Priority: -- → P2
See Also: → 1501562
Flags: needinfo?(snorp)

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

Flags: needinfo?(hskupin)

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

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

Flags: needinfo?(snorp)
Flags: needinfo?(hskupin)

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

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

(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 :)

Flags: needinfo?(snorp)
Pushed by nalexander@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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.

Flags: needinfo?(ato)
No longer depends on: 1533704
Regressions: 1533704
You need to log in before you can comment on or make changes to this bug.