753022 - debug builds should have marionette enabled

Status: RESOLVED FIXED

(Release Engineering :: General, defect, P3)

Comment 1

[John Ford [:jhford] CET/CEST Berlin Time]

gah, hit the error key too soon.

B2G uses marionette for testing and it'd be great to test whether m-c changes break the marionette tool.  To test for this, we should build Firefox with Marionette and run some tests against it.  Because marionette opens up a control interface, enabling this on nightly builds is not acceptable.  Since we use the same mozconfig for nightly and per-checkin builds, let's enable marionette only on debug builds.

There are two distinct work items for this bug:

1) add ac_add_options --enable-marionette to debug mozconfigs
2) create mozilla-tests builders that run the marionette tests

Tracking both items in this bug since 1) is very, very small.

Comment 2

[Chris AtLee [:catlee]]

Is this only for b2g builds?
Does this require a new build type, or you want to modify the existing debug builds?

Comment 3

[Jonathan Griffin (:jgriffin)]

This is for desktop Firefox builds.  We want to modify existing builds atm.  Eventually we should have full b2g emulator builds as well, but we can track that in a separate bug.

Comment 4

[Chris AtLee [:catlee]]

what kind of harness will the tests use? do any of our existing builders match it, or do we need to write a new one?

please write a patch for 1) and get it attached/landed.

Comment 5

[Jonathan Griffin (:jgriffin)]

(In reply to Chris AtLee [:catlee] from comment #4)
> what kind of harness will the tests use? do any of our existing builders
> match it, or do we need to write a new one?
> 
> please write a patch for 1) and get it attached/landed.

The tests will be run using Marionette.  We don't have any existing builders that match it.  Is writing a mozharness script for Marionette the likely fastest/easieste way to proceed?

Comment 6

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

:jhford, :jgriffin: If I read comment#1, comment#3 correctly, 

1) Can you confirm you want us to modify the existing Firefox debug builds to add Marionette support (and not create two different debug builds)? If so, is there any negative to Firefox (non-B2G) developers who do use debug builds, but do not use Marionette? 

2) what OS do you want this for new Marionette-debug build on? All 32+64 bit desktop OSes?

3) Which branches? All? Only m-c?

Comment 7

[Jonathan Griffin (:jgriffin)]

(In reply to John O'Duinn [:joduinn] from comment #6)
> :jhford, :jgriffin: If I read comment#1, comment#3 correctly, 
> 
> 1) Can you confirm you want us to modify the existing Firefox debug builds
> to add Marionette support (and not create two different debug builds)? If
> so, is there any negative to Firefox (non-B2G) developers who do use debug
> builds, but do not use Marionette? 

We could go either way with this.  The downside to using existing Firefox debug builds is that enabling Marionette opens up a potential security hole.  We're probably better off making specific Marionette-enabled debug builds, which wouldn't be used by anyone except people who explicitly want to use Marionette.

> 
> 2) what OS do you want this for new Marionette-debug build on? All 32+64 bit
> desktop OSes?

Ideally, yes.

> 
> 3) Which branches? All? Only m-c?

I'd say m-c, m-i, and try so things that break Marionette or Marionette tests could be caught before they end up in m-c.  (This has occurred several times in the past few months because we don't have any Marionette tests in TBPL at present.)

Comment 8

[[e-mail] (:bhearsum)]

(In reply to Jonathan Griffin (:jgriffin) from comment #7)
> (In reply to John O'Duinn [:joduinn] from comment #6)
> > :jhford, :jgriffin: If I read comment#1, comment#3 correctly, 
> > 
> > 1) Can you confirm you want us to modify the existing Firefox debug builds
> > to add Marionette support (and not create two different debug builds)? If
> > so, is there any negative to Firefox (non-B2G) developers who do use debug
> > builds, but do not use Marionette? 
> 
> We could go either way with this.  The downside to using existing Firefox
> debug builds is that enabling Marionette opens up a potential security hole.
> We're probably better off making specific Marionette-enabled debug builds,
> which wouldn't be used by anyone except people who explicitly want to use
> Marionette.

Completely random fly-by: Can the Marionette bits be turned on/off at runtime? Eg, build them in but turn them off by default. This would save us a new build type without exposing a security hole.

Comment 9

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

Attachment: --enable-marionette build flag

This patch adds requirement 1, the marionette build flag. With this patch, you can add --enable-marionette to the mozconfig and marionette will be built.

Comment 10

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

(In reply to Ben Hearsum [:bhearsum] from comment #8)
> (In reply to Jonathan Griffin (:jgriffin) from comment #7)
> > (In reply to John O'Duinn [:joduinn] from comment #6)
> > > :jhford, :jgriffin: If I read comment#1, comment#3 correctly, 
> > > 
> > > 1) Can you confirm you want us to modify the existing Firefox debug builds
> > > to add Marionette support (and not create two different debug builds)? If
> > > so, is there any negative to Firefox (non-B2G) developers who do use debug
> > > builds, but do not use Marionette? 
> > 
> > We could go either way with this.  The downside to using existing Firefox
> > debug builds is that enabling Marionette opens up a potential security hole.
> > We're probably better off making specific Marionette-enabled debug builds,
> > which wouldn't be used by anyone except people who explicitly want to use
> > Marionette.
> 
> Completely random fly-by: Can the Marionette bits be turned on/off at
> runtime? Eg, build them in but turn them off by default. This would save us
> a new build type without exposing a security hole.

We don't have a way to shut down the Marionette server at runtime, but the Marionette server is only launched if a pref is on *at startup*. If that pref is not set, then no server will be started. If you enable that pref during runtime, nothing will happen. Only if that pref is set and you start the gecko process will marionette start.

Comment 11

[[e-mail] (:bhearsum)]

It sounds like we should be able to add --enable-marionette to the existing debug builds then, assuming that pref isn't the default.

Comment 12

[Jonathan Griffin (:jgriffin)]

(In reply to Ben Hearsum [:bhearsum] from comment #11)
> It sounds like we should be able to add --enable-marionette to the existing
> debug builds then, assuming that pref isn't the default.

Security review didn't like that approach, at least for release builds, since they feel it is too easy to flip that pref (via a malicious add-on, for example).  Perhaps for debug builds that are primarily used internally, it wouldn't matter.

Comment 13

[Jonathan Griffin (:jgriffin)]

Comment on attachment 631379 [details] [diff] [review]
--enable-marionette build flag

I'm passing this review request to ted.

Comment 14

[[e-mail] (:bhearsum)]

(In reply to Jonathan Griffin (:jgriffin) from comment #12)
> (In reply to Ben Hearsum [:bhearsum] from comment #11)
> > It sounds like we should be able to add --enable-marionette to the existing
> > debug builds then, assuming that pref isn't the default.
> 
> Security review didn't like that approach, at least for release builds,
> since they feel it is too easy to flip that pref (via a malicious add-on,
> for example).  Perhaps for debug builds that are primarily used internally,
> it wouldn't matter.

We're only talking about debug builds here, though - we don't "release" those in any way - I'd be happy to help clarify this with security. None of: nightly, aurora, beta, or release (or even dep builds build with an "opt" configuration) would be affected by this.

Comment 15

[Jonathan Griffin (:jgriffin)]

Gary (:gkw) is our security liaison; Gary, can you follow up with :bhearsum if you have any concerns?

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 631379 [details] [diff] [review]
--enable-marionette build flag

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

Do we expect that this is something we'd want people flipping in their mozconfigs? If not, we should probably not add it and just use ENABLE_MARIONETTE=1 in the mozconfig.

Comment 17

[Jonathan Griffin (:jgriffin)]

(In reply to Ted Mielczarek [:ted] from comment #16)
> Comment on attachment 631379 [details] [diff] [review]
> --enable-marionette build flag
> 
> Review of attachment 631379 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do we expect that this is something we'd want people flipping in their
> mozconfigs? If not, we should probably not add it and just use
> ENABLE_MARIONETTE=1 in the mozconfig.

More people are using Marionette now for non-B2G work, including the testing of the apps runtime, and so would want local builds that include Marionette, but I don't have a strong opinion about --enable-marionette vs ENABLE_MARIONETTE=1.

Comment 18

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

(In reply to Jonathan Griffin (:jgriffin) from comment #15)
> Gary (:gkw) is our security liaison; Gary, can you follow up with :bhearsum
> if you have any concerns?

As long as official release builds (optimized) do not have an --enable-marionette flag or something equivalent to turn on Marionette, it is fine.

Since we don't "officially release" debug builds I don't see any issue with turning marionette on by default for debug builds.

Comment 19

[[e-mail] (:bhearsum)]

Attachment: enable marionette for all desktop debug builds

I just pushed this + your patch to try, too, to see if there will be any build issues: https://tbpl.mozilla.org/?tree=Try&rev=1167f721c436

Comment 20

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #19)
> Created attachment 632217 [details] [diff] [review]
> enable marionette for all desktop debug builds
> 
> I just pushed this + your patch to try, too, to see if there will be any
> build issues: https://tbpl.mozilla.org/?tree=Try&rev=1167f721c436

Can someone look at the builds from this job and make sure they are as you would like them to be? If so, the two patches can be pushed to mozilla-central once the final review is done.

Comment 21

[Jonathan Griffin (:jgriffin)]

These builds still don't contain Marionette.  @mdas, I think we need to add the marionette files to http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in.

Comment 22

[[e-mail] (:bhearsum)]

(In reply to Jonathan Griffin (:jgriffin) from comment #21)
> These builds still don't contain Marionette.  @mdas, I think we need to add
> the marionette files to
> http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-
> manifest.in.

IIRC Malini told me that she was working on this. Once that's finalized I think we can land everything from this bug. I'm happy to do that if it's helpful.

Comment 23

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

Attachment: package marionette in debug builds

Spoke with Ted about this approach and instead we'll be using the ENABLE_MARIONETTE=1 flag in the mozconfig, instead of using --enable-marionette.

I also added the marionette bits to the package-manifest if ENABLE_MARIONETTE is set

Comment 24

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

Attachment: enable marionette for all desktop debug builds v0.2

Changed patch to use ENABLE_MARIONETTE instead of --enable-marionette

Comment 25

[(not currently active) Ted Mielczarek]

Comment on attachment 632777 [details] [diff] [review]
package marionette in debug builds

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

::: browser/installer/Makefile.in
@@ +148,5 @@
>  ifdef INSTALLER_DIR
>  	$(MAKE) -C $(INSTALLER_DIR)
>  endif
> +
> +ifneq (,$(ENABLE_MARIONETTE))

I'd just use ifdef ENABLE_MARIONETTE, that's more typical of our Makefile style. (Unless this matches the surrounding context already, I'm too lazy to check that.)

Comment 26

[[e-mail] (:bhearsum)]

Comment on attachment 632778 [details] [diff] [review]
enable marionette for all desktop debug builds v0.2

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

This might need to be exported, I'm not sure...

Comment 27

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

Attachment: package marionette in debug builds v0.2

heh, thanks, I copypasta'd the style from the base/Makefile.in. Changed it to an ifdef statement

Comment 28

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

Comment on attachment 632790 [details] [diff] [review]
package marionette in debug builds v0.2

rolling forward ted's r+. This new patch just adds his suggestion.

Comment 29

[[e-mail] (:bhearsum)]

Comment on attachment 632778 [details] [diff] [review]
enable marionette for all desktop debug builds v0.2

It's my understanding that these two patches are ready to go. I pushed them to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/b9159bdee7b6
https://tbpl.mozilla.org/?rev=85e31a4bdd41

Comment 30

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #29)
> Comment on attachment 632778 [details] [diff] [review]
> enable marionette for all desktop debug builds v0.2
> 
> It's my understanding that these two patches are ready to go. I pushed them
> to mozilla-central:
> http://hg.mozilla.org/mozilla-central/rev/b9159bdee7b6

Oops, wrong TBPL link: https://tbpl.mozilla.org/?rev=b9159bdee7b6

Comment 31

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

Attachment: remove unused xpcshell tests

These tests aren't being used or maintained and were causing build failures.

Comment 32

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

Attachment: remove unused xpcshell tests v0.2

forgot to change testing/xpcshell/xpcshell.ini

After rebuilding, I don't see marionette tests being added to the xpcshell tests

Comment 33

[Jonathan Griffin (:jgriffin)]

Comment on attachment 633167 [details] [diff] [review]
remove unused xpcshell tests v0.2

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

Cool, thanks.

Comment 34

[Jonathan Griffin (:jgriffin)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f99f6a959ed9

Comment 35

[[e-mail] (:bhearsum)]

Comment on attachment 632790 [details] [diff] [review]
package marionette in debug builds v0.2

Re-landed this on inbound.

Comment 36

[[e-mail] (:bhearsum)]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=501024a244ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/501024a244ee

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f99f6a959ed9
https://hg.mozilla.org/mozilla-central/rev/501024a244ee

Comment 38

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

We aren't quite done here, since, from comment #1, this bug is also tracking "2) create mozilla-tests builders that run the marionette tests"

Comment 39

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Per mtg w/ctalbert, I'm spinning off a separate bug#770769 to track enabling the marionette tests on these debug builds. 

The work here to set up the marionette-enabled debug builds is done and in production, so closing this bug as FIXED.