1699627 - Require MOZ_MARIONETTE=1 to enable Marionette

Status: RESOLVED WONTFIX

(Remote Protocol :: Marionette, defect, P3)

Description

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

Follow-up from bug 1593343 since then we only enable Marionette when MOZ_MARIONETTE or --marionette is specified.

Currently it is still possible to enable Marionette by setting a falsy value for MOZ_MARIONETTE. Similar to other environment variables we should check the actual value for truthiness.

Comment 1

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

I think to make it clear we should only allow MOZ_MARIONETTE=1 to enable Marionette and not any other values.

Comment 2

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

Attachment: Bug 1699627 - [marionette] Only enable Marionette when MOZ_MARIONETTE is set to "1".

Comment 3

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

Attachment: Bug 1699627 - Use nsIMarionette service to get enabled state of Marionette within the Content Security Manager.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/270e0f9613a6
Use nsIMarionette service to get enabled state of Marionette within the Content Security Manager. r=freddyb
https://hg.mozilla.org/integration/autoland/rev/02e74b497dfc
[marionette] Only enable Marionette when MOZ_MARIONETTE is set to "1". r=marionette-reviewers,freddyb

Comment 5

[Cosmin Sabou [:CosminS]]

Backed out for bc failures on browser_startup_content.js.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=02e74b497dfc9d2a551fa0935df6664145fc26ce&searchStr=bc&selectedTaskRun=fFfMwuC4QCOEYDDf7Vt4Mg.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=334020404&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/a65fd6b25ce3c77a6799fa31597cf92dd334a96d

Comment 6

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

So there are two failures:

Unexpected modules loaded during content process startup: resource://gre/modules/ComponentUtils.jsm
Unexpected modules loaded during content process startup: chrome://marionette/content/prefs.js

The changes to dom/security/nsContentSecurityManager.cpp were causing that. Given that this code runs within a content process, getting the Marionette service and it's running property causes the marionette.js module loaded in the content process.

Given that ComponentUtils and both imports from prefs.js aren't lazily loaded, we see the above two failures.

Comment 7

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

Attachment: Bug 1699627 - [marionette] Always lazy-load dependent modules for the Marionette component.

Depends on D109363

Comment 8

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

The recent fix was not enough because ChromeUtils is always used at the end of the module in this line:

this.NSGetFactory = ComponentUtils.generateNSGetFactory([Marionette])

It looks like that this is very old code and nowadays could be avoided. I will see if a refactoring of the component will help here.

Comment 9

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

Without the xpconnect knowledge it's actually hard to figure out if this code is still needed the way it has been implemented years ago. Nika, maybe you could give some advice? I would kinda appreciate.

So as I get the nsIMarionette service can also be queried / used by code that runs in a child process. In those cases we create a limited instance of the Marionette component that communicates with the instance in the parent class to just query the running state:

https://searchfox.org/mozilla-central/rev/cc4a17cea338fe236626d838ca96e9bf6a775675/testing/marionette/components/marionette.js#560-593

Is there a way to get rid of the ComponentUtils usage here? We still get a test failure in the test browser/base/content/test/performance/browser_startup_content.js.

Comment 10

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

After the discussion with Nika yesterday it turned out that we should avoid creating an instance of the Marionette JS component in content. See details on https://phabricator.services.mozilla.com/D109363.

Right now I checked the behavior of MOZ_HEADLESS and it also accepts any value including 0, and doesn't check for truthiness. So whenever it is set, it will cause Firefox to start in headless mode. To keep consistency with all the existent MOZ_* environments and their behavior I would propose that we actually also follow that behavior with Marionette.

@jdescottes, are you fine with that?

Comment 11

[Julian Descottes [:jdescottes]]

Yes fine with me!

Comment 12

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

As discussed marking the bug as wontfix. I'll move the one remaining patch to a new bug for clarity.

Comment 13

[Phabricator Automation]

Comment on attachment 9211106 [details]
Bug 1699627 - [marionette] Always lazy-load dependent modules for the Marionette component.

Revision D109538 was moved to bug 1700959. Setting attachment 9211106 [details] to obsolete.