1750726 - Marionette will be enabled in Multi-Process browser toolbox when MOZ_MARIONETTE is set

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

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

Noticed while working on bug 1726465.

When running Marionette tests with the MOZ_MARIONETTE=1 environment variable explicitly set for the mach command, or after a restart of Firefox any attempt to open the Multi-Process Browser Toolbox will show that it inherits the environment variable and will also enable Marionette.

For bug 1726465 this is problematic because it will happen while Marionette is still initializing and as such the marionette-startup-requested notification is also received. This triggers an error because the Marionette port is already in use, and causes the debugger to quit immediately.

Maybe we should reset the environment variable as soon as Marionette has access to it, and only set it again when the application is going to shutdown - means when handling the quit-application notification.

Comment 1

[Julian Descottes [:jdescottes]]

For reference the BT is started from https://searchfox.org/mozilla-central/rev/8d108a59d067ce37671090b0b1972ee8adfb7196/devtools/client/framework/browser-toolbox/Launcher.jsm and you can find some documentation about the overall architecture at https://searchfox.org/mozilla-central/source/devtools/client/framework/browser-toolbox/README.md

I think technically we reuse the current environment because we pass environmentAppend to SubProcess. We can override specific environment variables in the environment object, so we might want to set MOZ_MARIONETTE: null in this object here: https://searchfox.org/mozilla-central/rev/8d108a59d067ce37671090b0b1972ee8adfb7196/devtools/client/framework/browser-toolbox/Launcher.jsm#274

Comment 2

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

I had a look at nsIEnvironment but it does not allow to remove an environment variable at least via JS, and Marionette gets enabled if the environment variable exists but not on its value. We decided to use this approach on purpose to be in sync with all the other MOZ_* related environment variables like MOZ_HEADLESS.

(In reply to Julian Descottes [:jdescottes] from comment #1)

I think technically we reuse the current environment because we pass environmentAppend to SubProcess. We can override specific environment variables in the environment object, so we might want to set MOZ_MARIONETTE: null in this object here: https://searchfox.org/mozilla-central/rev/8d108a59d067ce37671090b0b1972ee8adfb7196/devtools/client/framework/browser-toolbox/Launcher.jsm#274

Thank you for the details Julian! Adding the entry there seems to indeed fix the problem. I'll try a bit more and if that's the case come up with a patch.

Comment 3

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

Attachment: Bug 1750726 - [devtools] Never enable Marionette for the multi-process browser toolbox process.

If the browser toolbox is started from a Marionette enabled process,
Marionette should never be started for the browser toolbox process.
If we do as right now the browser toolbox process will immediately
shutdown because Marionette cannot listen on the already in-use
socket port and forces a process shutdown.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5edfcaa0b50d
[devtools] Never enable Marionette for the multi-process browser toolbox process. r=jdescottes

Comment 5

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/5edfcaa0b50d