870445 - Add --marionette CLI to enable Marionette on all Firefox builds

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

We need a way for developers to use Marionette on arbitrary builds.  Currently, it's only enabled on TBPL debug builds.  Our current case-in-point is a SocialAPI app that dmose's team needs to test using nightly builds (dmose can provide further details if needed).

One of the suggestions that was floated at a recent security meeting is:

* add a --marionette CLI to Firefox to enable Marionette
* prevent any Marionette prefs from appearing in about:config if Marionette isn't enabled

The code to implement this is pretty easy; but we'd need security signoff.  Gary, can you take care of the necessary reviews?  According to dmose, the need for this is rather pressing.

Comment 1

[Jonathan Griffin (:jgriffin)]

Pinging Gary for security review of this concept.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

To add a bit more context, my team is working on a webapp called Talkilla, which we're just pivoting to move much of the front-end code to run in the Social API code context, which we're planning on have some sort of release around the time of Firefox 22.  

We're at the point where we need to choose a functional-testing strategy, and at this point, the options seem to be "use Selenium WebDriver by creating a FirefoxDriver fork and hack it to understand the SocialAPI and use WebDriver" or "use Marionette, hack it to understand the SocialAPI, and make it available to our developers and our continuous integration for Firefox 22".

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

gkw and I spoke briefly about this today, and my understanding of the upshot is that once the things above are implemented, that may well be good enough, but the security team will want to do a deeper review before giving it the go-ahead.  He said that once the code is implemented, he can help move that review forward...

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

But before gkw can help move said review forward, it'll need to be filed at <https://wiki.mozilla.org/Security/Reviews/Review_Request_Form>.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Gary says that it could be possible to do a review as soon as May 13th at 1pm Pacific.  

It's not yet clear to me whether I'll end up being able to write this patch or not,  as I have some other stuff competing for my time.

So while that time won't work for me personally (I'll be in Europe, terribly jetlagged), I don't think my presence at that sec review is particularly necessary to move this forward.  It's probably mostly about whether anyone is able to sign up to write that code between now and then, as well as whether that time would work for the Marionette team.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

To be clear, this is about enabling Marionette in Firefox Desktop (with required runtime params). David Burns has mentioned that mobile is a different issue altogether, and dmose needs this on Desktop for the moment.

Let's get this on the schedule first by having the review request form filled out.

Comment 7

[Jonathan Griffin (:jgriffin)]

Code that illustrates how to add a command-line is at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/jsconsole-clhandler.js; that file adds the -jsconsole argument.

Code for doing this for Marionette should be added to http://mxr.mozilla.org/mozilla-central/source/testing/marionette/components/marionettecomponent.js.

As far as prefs go, we need to make sure that no Marionette prefs are defined (so they don't show up in about:config) unless --marionette || ENABLE_MARIONETTE.  This is already true for desktop Firefox.

We should make it so that if not --marionette || ENABLE_MARIONETTE, there is no set of pref(s) that can be set to enable Marionette.

We'll want to deprecate ENABLE_MARIONETTE for --marionette, I believe, for desktop Firefox, but we should do so in a separate patch, because this will require some changes to the mozharness scripts that run Marionette tests, and those changes cannot be landed at the same time as gecko changes.

So, for the purposes of this patch, Marionette should be enabled either if --marionette is passed, OR if ENABLE_MARIONETTE is defined in mozconfig (like it is today).

We can file subsequent bugs for getting rid of ENABLE_MARIONETTE, which we can do at our leisure.

Comment 8

[David Burns :automatedtester]

* Who is/are the point of contact(s) for this review? David Burns :AutomatedTester

* Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.): 

Marionette allows remote execution of tasks in the browser that allow end users to be able to test web applications. This is a port of FirefoxDriver from the Selenium[1] project with a number of added pieces to fit in with Mozilla's needs as well. Currently used on FirefoxOS automation and is now required for SocialAPI testing as in comment 0
Marionette is based on the W3C WebDriver specification[2] currently being drafted and implemented by numerous vendors.

* Please provide links to additional information (e.g. feature  page, wiki) if available and not yet included in feature description: 

https://developer.mozilla.org/en-US/docs/Marionette/Marionette

* Does this request block another bug? If so, please indicate the bug number 
 
* This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review? 

dmose's team needs this landed ASAP so they can write tests for Talkilla, which is scheduled to be released late June.  Without this, they'd have to use a hacked up webdriver solution instead, which is probably less ideal, and may require extra work later to port to Marionette.

* To help prioritize this work request, does this project support  a goal specifically listed on this quarter's goal list?  If so, which  goal? 

Yes, this is needed to support the SocialAPI/Talkilla goals for this quarter

*Please answer the following few questions: (Note: If you are asked to describe anything, 1-2 sentences shall suffice.) 

    Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users? 

    Yes

    Are there any portions of the project that interact with 3rd party services? 

    No

    Will your application/service collect user data? If so, please describe 

    No

*If you feel something is missing here or you would like to  provide other kind of feedback, feel free to do so here (no limits on  size): 

Chromium already ships[3] with their implementation of the spec and uses similar technique as discussed above of CLI flags and no prefs.
Opera also has their implementation[4], again using the technique described earlier of CLI flags and no prefs as standard

* Desired Date of review (if known from https://mail.mozilla.com/home/[e-mail]/Security%20Review.html) and whom to invite. 

May 13, please invite dmose, dburns, jgriffin, mdas

[1] http://selenium.googlecode.com/
[2] http://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html
[3] http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/webdriver/
[4] https://github.com/operasoftware/operadriver

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

It's great to see that this is moving forward.  As mentioned in comment 5, I can't make that specific time, but I don't think I'm a critical participant here, merely a potential consumer, so I look forward to learning about the outcome.

Comment 10

[David Burns :automatedtester]

Attachment: refactored for handler for feedback

Comment 11

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

Comment on attachment 749015 [details] [diff] [review]
refactored for handler for feedback

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

::: testing/marionette/components/MarionetteComponents.manifest
@@ +1,5 @@
>  # Marionette
>  component {786a1369-dca5-4adc-8486-33d23c88010a} marionettecomponent.js
>  contract @mozilla.org/marionette;1 {786a1369-dca5-4adc-8486-33d23c88010a}
> +category command-line-handler b-marionette @mozilla.org/marionette;1
> +category final-ui-startup MarionetteComponent @mozilla.org/marionette;1

This is really late in the process. Why have you changed it from profile-after-change?

Comment 12

[Jonathan Griffin (:jgriffin)]

Comment on attachment 749015 [details] [diff] [review]
refactored for handler for feedback

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

The mistake here is specifying cmdLine.preventDefault = true.  The default action is opening the browser, so preventDefault = true prevents that.

I think we'll need to make this coexist with ENABLE_MARIONETTE=1 at least in the short term.  What I envision is something like this:

handle: function mc_handle(cmdLine) {
  // if -marionette, start Marionette
},

observe: function mc_observe(aSubject, aTopic, aData) {
#ifdef ENABLE_MARIONETTE
  // if marionette.defaultPrefs.enable == true, start Marionette
#endif
},

This way, if !ENABLE_MARIONETTE, no prefs can be used to start it without -marionette.  But, if ENABLE_MARIONETTE is defined in the build, then we wouldn't need -marionette.

This is necessary to support the way that we're running tests in buildbot; we currently don't pass -marionette on startup, and we won't be able to land that change at the same time as adding -marionette to gecko.  After we land this, we can make the corresponding mozharness change, then land another gecko change to remove support for ENABLE_MARIONETTE for Firefox only (we'll still want it for B2G).

Comment 13

[David Burns :automatedtester]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> Comment on attachment 749015 [details] [diff] [review]
> refactored for handler for feedback
> 
> Review of attachment 749015 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/marionette/components/MarionetteComponents.manifest
> @@ +1,5 @@
> >  # Marionette
> >  component {786a1369-dca5-4adc-8486-33d23c88010a} marionettecomponent.js
> >  contract @mozilla.org/marionette;1 {786a1369-dca5-4adc-8486-33d23c88010a}
> > +category command-line-handler b-marionette @mozilla.org/marionette;1
> > +category final-ui-startup MarionetteComponent @mozilla.org/marionette;1
> 
> This is really late in the process. Why have you changed it from
> profile-after-change?


It appears that profile-after-change happens before command line handers. The profile after change code has been moved to the handler. If you look at the component code Marionette initialization happens in final-ui-startup so its all pretty much happening in the same way except we dont have a race between profile-after-change and command-line-handler

Comment 14

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

(In reply to David Burns :automatedtester from comment #13)
> The profile after change code has been moved to the handler. If you look at
> the component code Marionette initialization happens in final-ui-startup so
> its all pretty much happening in the same way except we dont have a race
> between profile-after-change and command-line-handler

If all initialization code is in final-ui-startup it should be fine. Thanks for pointing that out.

Comment 15

[David Burns :automatedtester]

Attachment: 15-05-2013 patch with ifdef

This works for enabling via the handler but unfortunately the final-ui-startup observer never fires for some reason.

I suspect its simple but I can't see it

Comment 16

[Jonathan Griffin (:jgriffin)]

Attachment: Add -marionette command-line arg to Firefox,

Will ask mdas to review this, since dburns and I have both worked on this patch.  With this patch:

if not ENABLE_MARIONETTE, Marionette can only be enabled by passing -marionette.  There are no prefs that can be set that will enable it without this argument.

if ENABLE_MARIONETTE, Marionette is enabled if marionette.defaultPrefs.enabled is set; in this case you don't need -marionette.


The intention is to have Marionette built into all Firefox builds, and enabled only by -marionette, but to continue to support ENABLE_MARIONETTE for b2g/android.  For the short term, debug builds of Firefox will also define ENABLE_MARIONETTE; as soon as this patch lands, we can update the mozharness script that invokes Marionette tests in TBPL to pass -marionette, and then we can remove ENABLE_MARIONETTE from debug Firefox configs.

Comment 17

[Jonathan Griffin (:jgriffin)]

try run: https://tbpl.mozilla.org/?tree=Try&rev=ec6523bf07ca

Once this completes, I'll download all the things and make sure they behave as we expect.

Comment 18

[Jonathan Griffin (:jgriffin)]

Comment on attachment 750542 [details] [diff] [review]
Add -marionette command-line arg to Firefox,

Flagging gkw who can check for any security concerns.

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 750542 [details] [diff] [review]
Add -marionette command-line arg to Firefox,

> as soon as this patch lands, we can update the
> mozharness script that invokes Marionette tests in TBPL to pass -marionette,
> and then we can remove ENABLE_MARIONETTE from debug Firefox configs.

Please file bugs for these. (or link them here) Note bug 741812 as well.

Comment 20

[Jonathan Griffin (:jgriffin)]

The try run looked good.  I will download the builds and verify:

- on opt Firefox builds, Marionette cannot be enabled without -marionette
- on B2G builds, Marionette cannot be enabled for user builds (opt or otherwise)
- on Android opt builds, Marionette is not enabled

Comment 21

[Jonathan Griffin (:jgriffin)]

Attachment: Add -marionette command-line arg to Firefox,

One change to prevent us from writing logging info to stdout on opt Firefox.

Comment 22

[Jonathan Griffin (:jgriffin)]

Comment on attachment 751165 [details] [diff] [review]
Add -marionette command-line arg to Firefox,

Carry r+ forward.

Comment 23

[Jonathan Griffin (:jgriffin)]

new try run: https://tbpl.mozilla.org/?tree=Try&rev=2a9f354305f5

Comment 24

[Jonathan Griffin (:jgriffin)]

Attachment: Add -marionette command-line arg to Firefox,

One more change.  The command-line handler and final-ui-startup do not occur in a predictable order, so I had to do a bit of refactoring.

Comment 25

[Jonathan Griffin (:jgriffin)]

new try run: https://tbpl.mozilla.org/?tree=Try&rev=3fd6636fe332

Comment 26

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

Comment on attachment 751280 [details] [diff] [review]
Add -marionette command-line arg to Firefox,

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

::: testing/marionette/components/marionettecomponent.js
@@ +77,2 @@
>    observe: function mc_observe(aSubject, aTopic, aData) {
> +    this.logger.info("observe: " + aTopic);

Do we want to keep this logging line here?

Comment 27

[Jonathan Griffin (:jgriffin)]

Attachment: Add -marionette command-line arg to Firefox,

Deleted extra logging statement, and added a comment mdas and I discussed on IRC.

Comment 28

[Jonathan Griffin (:jgriffin)]

Comment on attachment 752360 [details] [diff] [review]
Add -marionette command-line arg to Firefox,

Carry r+ forward.

Comment 29

[Jonathan Griffin (:jgriffin)]

This will be bitrotted by bug 797529 as soon as that lands; I'll update this patch when it does.

Comment 30

[Jonathan Griffin (:jgriffin)]

Attachment: Add -marionette command-line arg to Firefox,

Fixed bitrot that was due to bug 797529 landing, will do a last try run as soon as that's merged to m-c.

Comment 31

[Jonathan Griffin (:jgriffin)]

Comment on attachment 759498 [details] [diff] [review]
Add -marionette command-line arg to Firefox,

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

Mind giving these changes a quick sanity check?

Comment 32

[David Burns :automatedtester]

Comment on attachment 759498 [details] [diff] [review]
Add -marionette command-line arg to Firefox,

ship it!

Comment 33

[Jonathan Griffin (:jgriffin)]

Looks like bug 797529 will be hung up in reviews in the short term, so I've pushed the original patch to try (https://tbpl.mozilla.org/?tree=Try&rev=a563675e6ba5) and if that goes green, I'll land it.

Comment 34

[Jonathan Griffin (:jgriffin)]

try was good, landed on m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9508c92f3cc

Comment 35

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d9508c92f3cc

Comment 36

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

Looks like this added a feature to Firefox without getting review from a Firefox peer.

I'm concerned about the overhead of the marionette code (marionettecomponent.js et al) on Firefox release builds (memory, startup). This looks like it wholesale enabled all of the marionnette code for all desktop Firefox builds?

Comment 37

[Jonathan Griffin (:jgriffin)]

Whoops, I didn't realize we needed a Firefox peer review; who should do that?

Comment 38

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

In general, anyone on https://wiki.mozilla.org/Modules/Firefox.

But since I'm already here and interested, I can review! Is my read in comment 36 correct? This made us ship Marionnette code for all Firefox builds?

Comment 39

[Jonathan Griffin (:jgriffin)]

Indeed it does.  The rationale is that we'd like to expose Marionette to test developers who want to test shipping builds; requiring a custom build is a high barrier to use for many people who are interested in only testing their web properties.

Comment 40

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

OK. The flip side of that is that you're now shipping Firefox code, rather than test-only code, and so I think the quality bar is a bit higher and you have more constraints (performance concerns, code review, etc.).

Have you done any measurements of memory/performance impact of having this code be on by default?

Comment 41

[Jonathan Griffin (:jgriffin)]

We have not, but the amount of code loaded *unless* you specify -marionette is quite small.  How would you suggest making those memory/performance measurements?  (i.e., would the lack of Talos regressions be suitable evidence that no negative perf impacts had occurred?)

Comment 42

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

(In reply to Jonathan Griffin (:jgriffin) from comment #41)
> We have not, but the amount of code loaded *unless* you specify -marionette
> is quite small.

It probably makes sense to put that code under browser/, then, to make the separation a bit clearer. In fact, it may be optimal to add this logic to Firefox's existing command line handlers, rather than rolling your own.

> How would you suggest making those memory/performance measurements?
> (i.e., would the lack of Talos regressions be suitable
> evidence that no negative perf impacts had occurred?)

Unfortunately talos doesn't have complete coverage of the metrics we care about. Memory use for marionettecomponent.js can be measured at a high level with about:memory. I'm not sure about the potential startup-time impact of an extra (trivial) JS app-startup component, CCing some perf folks who might have a better sense (or advice on how to measure).

Comment 43

[Jonathan Griffin (:jgriffin)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #42)
> (In reply to Jonathan Griffin (:jgriffin) from comment #41)
> > We have not, but the amount of code loaded *unless* you specify -marionette
> > is quite small.
> 
> It probably makes sense to put that code under browser/, then, to make the
> separation a bit clearer. In fact, it may be optimal to add this logic to
> Firefox's existing command line handlers, rather than rolling your own.
> 
Ok, I'll look for somewhere appropriate to put it.

Comment 44

[Jonathan Griffin (:jgriffin)]

The only pre-existing browser command-line handler that we could really piggyback off of is nsBrowserContentHandler.js; would you like us to move the Marionette command-line code into that file, or add a new one under browser/components?

Comment 45

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

Adding minimal handling for -marionette there makes sense, yes. The bulk of the logic for initialization and such should probably stay out of that file, but that seems relatively straightforward to factor out.