882846 - Investigate if Marionette and Remote Agent have to handle their command line arguments themselves

Status: RESOLVED WONTFIX

(Remote Protocol :: Agent, task, P3)

Description

[Jonathan Griffin (:jgriffin)]

Bug 870445 introduced a -marionette arg to the browser, but according to :gavin, this should live in nsBrowserContent.js and be separately reviewed by a browser peer.  This bug will track that work.

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: Move -marionette arg handling code into browser,

Try run in progress: https://tbpl.mozilla.org/?tree=Try&rev=cc54a4afd520

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

I think this was addressed by https://bugzilla.mozilla.org/show_bug.cgi?id=870445?

Comment 3

[David Burns :automatedtester]

@MDas, we added it in bug 870445 but we did it wrong and it needs to be moved as per Gavin's request. See comment 0

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 763861 [details] [diff] [review]
Move -marionette arg handling code into browser,

Really sorry this patch sat in my review queue for so long.

Mark, can you give this a look at some point? I recall you and I discussing this long ago...

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 763861 [details] [diff] [review]
Move -marionette arg handling code into browser,

Review of attachment 763861 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine to me, although it seems we can probably remove the xpcom component completely, and that nsBrowserContentHandler.js should still have a check for #ifdef ENABLE_MARIONETTE and possibly for MARIONETTE_ENABLED_PREF (although an enabled pref doesn't sound that important if the cmdline arg is necessary to enable it)

::: testing/marionette/MarionetteLoader.jsm
@@ +13,5 @@
> +const MARIONETTE_PORT_PREF = 'marionette.defaultPrefs.port';
> +
> +Cu.import("resource://gre/modules/FileUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/services-common/log4moz.js");

Should use Log.jsm now

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Not reasonable to ask Jonathan to pick this back up after this long - Mark, perhaps you could have a go at addressing your feedback comments and flag me for review again?

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #6)
> Not reasonable to ask Jonathan to pick this back up after this long - Mark,
> perhaps you could have a go at addressing your feedback comments and flag me
> for review again?

Sure.  Jonathan, can you please answer the following for me:

* Are you aware of any need to keep the xpcom component?
* The old patch had a pref to disable marionette, but it seems it marionette was only activated when the cmdline arg was used - which would seem mean the pref isn't actually needed.  Is there something I'm missing here?

Comment 8

[Jonathan Griffin (:jgriffin)]

We don't specifically need the xpcom component; I added it because I wasn't sure you would want this stuff added to e.g., nsBrowserContentHandler.js.

The pref isn't useful for desktop Firefox, but is relevant for B2G (and maybe Android in the future).

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Sorry, but I don't think I'm going to get to this anytime soon and leaving it assigned to me is misleading.

Comment 10

[Andreas Tolfsen ❲:ato❳]

I just noticed this.  Is it still the case that it is bad practice
to handle flags in arbitrary XPCOM components?  Should this still
be moved to nsBrowserContentHandler.js?

Comment 11

[Mark Hammond [:markh] [:mhammond]]

(In reply to Andreas Tolfsen ‹:ato› from comment #10)
> I just noticed this.  Is it still the case that it is bad practice
> to handle flags in arbitrary XPCOM components?  Should this still
> be moved to nsBrowserContentHandler.js?

Gavin's no longer involved with Mozilla, but I see no reason to believe anything has changed in that time (but also no reason to believe it's a huge problem now that it has existed for a number of years)

Comment 12

[Andreas Tolfsen ❲:ato❳]

Increasing the priority of this because -marionette has now disappeared
from -h altogether.  See
https://bugzilla.mozilla.org/show_bug.cgi?id=1467453.

Comment 13

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

(In reply to Andreas Tolfsen ❲:ato❳ from comment #12)

Increasing the priority of this because -marionette has now disappeared
from -h altogether. See
https://bugzilla.mozilla.org/show_bug.cgi?id=1467453.

This was fixed in bug 1720676 for Firefox 92 and is no longer a problem.

Meanwhile the code of Marionette has been moved to /remote and we would also have to cover the Remote Agent which is used for WebDriver BiDi. As such we have the following command line arguments:

• --marionette (Marionette.jsm)
• --remote-debugging-port [<port>] (RemoteAgent.jsm)
• --remote-allow-hosts <hosts> (soon via bug 1759994)
• --remote-allow-origins <hosts> (soon via bug 1759994)

I don't think that moving the current command line handling code to browser/components/BrowserContentHandler.jsm is something that will work given that Marionette and Remote Agent is not only used by Firefox but also Fenix and Thunderbird. As such we would have to add the required code to several applications. Not sure if there is something in Toolkit which might be adequate?

Gijs, I would appreciate some feedback from your side, especially if it's really something that we can do or if the bug should be closed. Thanks.

Comment 14

[:Gijs (he/him)]

I am not aware of any reason to want to move this directly into the browser; if you also are not, I suggest we close the bug.

Comment 15

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

That sounds perfectly fine and we indeed should just close this bug. Thanks!