debug builds should have marionette enabled

RESOLVED FIXED

Status

Release Engineering
General
P3
normal
RESOLVED FIXED
6 years ago
2 months ago

People

(Reporter: jhford, Assigned: mdas)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tests][b2g])

Attachments

(3 attachments, 4 obsolete attachments)

Comment hidden (empty)
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.
Summary: debug builds should have marionette → debug builds should have marionette enabled and tested
Depends on: 753038
Is this only for b2g builds?
Does this require a new build type, or you want to modify the existing debug builds?
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.
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.
Priority: -- → P3
Whiteboard: [tests][b2g]
(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?
: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?
(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.)
(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.
Created attachment 631379 [details] [diff] [review]
--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.
Attachment #631379 - Flags: review?(jgriffin)
(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.
It sounds like we should be able to add --enable-marionette to the existing debug builds then, assuming that pref isn't the default.
(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 on attachment 631379 [details] [diff] [review]
--enable-marionette build flag

I'm passing this review request to ted.
Attachment #631379 - Flags: review?(jgriffin) → review?(ted.mielczarek)
(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.
Gary (:gkw) is our security liaison; Gary, can you follow up with :bhearsum if you have any concerns?
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.
(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.
(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.
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
Attachment #632217 - Flags: review?(mdas)
Attachment #632217 - Flags: review?(mdas) → review+
(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.
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.
(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.
Assignee: nobody → mdas
Created attachment 632777 [details] [diff] [review]
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
Attachment #631379 - Attachment is obsolete: true
Attachment #631379 - Flags: review?(ted.mielczarek)
Attachment #632777 - Flags: review?(ted.mielczarek)
Created attachment 632778 [details] [diff] [review]
enable marionette for all desktop debug builds v0.2

Changed patch to use ENABLE_MARIONETTE instead of --enable-marionette
Attachment #632217 - Attachment is obsolete: true
Attachment #632778 - Flags: review?(bhearsum)
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.)
Attachment #632777 - Flags: review?(ted.mielczarek) → review+
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...
Attachment #632778 - Flags: review?(bhearsum) → review+
Created attachment 632790 [details] [diff] [review]
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
Attachment #632777 - Attachment is obsolete: true
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.
Attachment #632790 - Flags: review+
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
Attachment #632778 - Flags: checked-in+
Attachment #632790 - Flags: checked-in+
(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
Created attachment 633127 [details] [diff] [review]
remove unused xpcshell tests

These tests aren't being used or maintained and were causing build failures.
Attachment #633127 - Flags: review?(jgriffin)
Attachment #632778 - Flags: checked-in+ → checked-in-
Attachment #632790 - Flags: checked-in+ → checked-in-
Created attachment 633167 [details] [diff] [review]
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
Attachment #633127 - Attachment is obsolete: true
Attachment #633127 - Flags: review?(jgriffin)
Attachment #633167 - Flags: review?(jgriffin)
Comment on attachment 633167 [details] [diff] [review]
remove unused xpcshell tests v0.2

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

Cool, thanks.
Attachment #633167 - Flags: review?(jgriffin) → review+
Comment on attachment 632790 [details] [diff] [review]
package marionette in debug builds v0.2

Re-landed this on inbound.
Attachment #632790 - Flags: checked-in-
Attachment #632778 - Flags: checked-in-
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"
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW
Depends on: 753490, 761202
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.
Status: NEW → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Summary: debug builds should have marionette enabled and tested → debug builds should have marionette enabled
Product: mozilla.org → Release Engineering
Component: General Automation → General
Product: Release Engineering → Release Engineering
You need to log in before you can comment on or make changes to this bug.