1363897 - Don't allow legacy extensions in beta/release builds post-57

Status: VERIFIED FIXED

(Infrastructure & Operations Graveyard :: CIDuty, task, P1)

Description

[Andrew Swan [:aswan]]

In bug 1359203 we will add a new preference that controls whether legacy extensions can be enabled.  Beginning in 57, this preference should only be honored on Nightly and on unbranded builds.  To do this, we need a build-time flag in AppConstants to indicate whether the preference is valid (suggested name MOZ_ALLOW_LEGACY_EXTENSIONS).  It should end up looking a lot like the results of bug 1186522.

Comment 1

[Andy McKay]

Not sure who to ask Kim, but you helped us out last time. Know where we should route this now?

Comment 2

[Kim Moir [:kmoir] ET]

I have too many other projects on my plate now, I'll ask in #releng if someone else has time to time to look at it.

Comment 3

[Andrew Swan [:aswan]]

Okay, bug 1359203 actually covered the build system part of this (adding a configure-time option for whether to allow legacy extensions).  But we still need the releng work to set that flag properly in beta and release builds beginning with the 57 release.  Re-targeting this bug to that effort.

Comment 4

[Chris Cooper [:coop] (he/him)]

(In reply to Andrew Swan [:aswan] from comment #3)
> Okay, bug 1359203 actually covered the build system part of this (adding a
> configure-time option for whether to allow legacy extensions).  But we still
> need the releng work to set that flag properly in beta and release builds
> beginning with the 57 release.  Re-targeting this bug to that effort.

Tagging :kmoir. Maybe this is something buildduty could handle now.

Comment 5

[Masatoshi Kimura [:emk]]

Does "Beta" include Developer Edition repack?

Comment 6

[Kim Moir [:kmoir] ET]

Andrei, is this something you can look at?

Comment 7

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

Sure Kim,I'll check to see what I can do here

Comment 8

[Kim Moir [:kmoir] ET]

Andrei, you asked this morning for hints on this bug

In this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1359203

and this patch

https://reviewboard.mozilla.org/r/139586/diff/3#index_header

you can see there is a new flag enabled called MOZ_ALLOW_LEGACY_EXTENSIONS

there are mozconfigs for different builds
example for win64 here

mozilla-inbound/browser/config/mozconfigs/win64

we need to implement patches in tree to only enable this value for nightly and unbranded builds beginning in 57.  

You can create patches, test on try and verify that this is true (try to install an old style extension).

Let me know if you have questions or need additional help!

Comment 9

[Andrew Swan [:aswan]]

I think I may have misunderstood project_flag() when I wrote the patch referenced in the previous comment, and we should probably use option() instead of project_flag().  I assume that can happen as part of patches for this bug...

Comment 10

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

Attachment: bug1363897_v1.patch

Mihai, is this the right direction?I'm not sure if I should define new flag or use the one that was created: MOZ_ALLOW_LEGACY_EXTENSIONS.

Comment 11

[Andrew Swan [:aswan]]

Andrei, see comment 9 -- I don't think your changes have any effect right now since bug 1359203 didn't actually create a configure-time settable switch.  A build peer (eg :glandium) should be able to help with the right way to set this up.

Comment 12

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to Andrew Swan [:aswan] from comment #11)
> Andrei, see comment 9 -- I don't think your changes have any effect right
> now since bug 1359203 didn't actually create a configure-time settable
> switch.  A build peer (eg :glandium) should be able to help with the right
> way to set this up.

(In reply to Andrei Obreja [:aobreja][:buildduty] from comment #10)
> Created attachment 8884832 [details] [diff] [review]
> bug1363897_v1.patch
> 
> Mihai, is this the right direction?I'm not sure if I should define new flag
> or use the one that was created: MOZ_ALLOW_LEGACY_EXTENSIONS.

Glanced at these patches but it's build stuff way out of my knowledge. I'll redirect to :glandium.
@glandium - can you please give us some advice as to how to tackle this here?

Thanks!

Comment 13

[Mihai Tabara [:mtabara]⌚️GMT]

:catlee - this just occurred to me, I just realized this might be needed before merging trains on the 2nd of August. Just to double-check, is this something high-priority to be done by 2 August or something we can work on while 57 stays in nightly train?

Comment 14

[Chris AtLee [:catlee]]

(In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #13)
> :catlee - this just occurred to me, I just realized this might be needed
> before merging trains on the 2nd of August. Just to double-check, is this
> something high-priority to be done by 2 August or something we can work on
> while 57 stays in nightly train?

Per comment #3, I think this is something we can work on once 57 is in Nightly. I don't think we need to do anything for the 56 beta/release cycle.

Comment 15

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to Chris AtLee [:catlee] from comment #14)
> (In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #13)
> > :catlee - this just occurred to me, I just realized this might be needed
> > before merging trains on the 2nd of August. Just to double-check, is this
> > something high-priority to be done by 2 August or something we can work on
> > while 57 stays in nightly train?
> 
> Per comment #3, I think this is something we can work on once 57 is in
> Nightly. I don't think we need to do anything for the 56 beta/release cycle.

Just to confirm, is this correct, can the work of this bug be completed *while* we have 57 in nightly?

Comment 16

[Andrew Swan [:aswan]]

Yes, it can be done during the 57 cycle.  We would very much like to have it early in the cycle though so that simulated beta build/test runs start getting us coverage of automated tests with legacy extensions disabled.

Comment 17

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to Andrew Swan [:aswan] from comment #16)
> Yes, it can be done during the 57 cycle.  We would very much like to have it
> early in the cycle though so that simulated beta build/test runs start
> getting us coverage of automated tests with legacy extensions disabled.

Sounds good. I'll bump it up in priority to be done early in the 56 beta cycle.
Thanks for confirming this!

Comment 18

[Mike Hommey [:glandium]]

(In reply to Andrew Swan [:aswan] from comment #0)
> In bug 1359203 we will add a new preference that controls whether legacy
> extensions can be enabled.  Beginning in 57, this preference should only be
> honored on Nightly and on unbranded builds.

What do you mean "unbranded builds"?

Comment 19

[Andrew Swan [:aswan]]

(In reply to Mike Hommey [:glandium] from comment #18)
> What do you mean "unbranded builds"?

We provide builds of release and beta that don't use the official Firefox branding but that do honor the preference that controls addon signing (ie so that you can turn it off if you wish).  We do a rather poor job of providing access to these builds but:
https://wiki.mozilla.org/Add-ons/Extension_Signing#Unbranded_Builds

I'm not certain but I think these builds are created with the "add-on-devel" configs in eg browser/config/mozconfigs/$platform/

The goal of this bug is to make a configure-time flag that controls whether legacy extensions can be enabled by flipping a pref.  My understanding is that releng will then take care of making sure that that flag is set properly in each relevant config file and that everything is handled properly on merge days, but could use some help with actually exposing the configure-time option.

Back in bug 1359203 I added this:
http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/toolkit/moz.configure#501-503
But I didn't understand the project_flag creates a fixed value.  What do we do to make this something that can be overridden from mozconfig?  Just switch from project_flag() to option() ?

Comment 20

[Mike Hommey [:glandium]]

> Just switch from project_flag() to option() ?

That seems sensible. And probably change the default to milestone.is_nightly.

Comment 21

[Andrew Swan [:aswan]]

Attachment: Bug 1363897 Switch legacy extensions handling to option()

Review commit: https://reviewboard.mozilla.org/r/169808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/169808/

Comment 22

[Andrew Swan [:aswan]]

Unless I botched something, the attached patch should make it possible to enable/disable legacy extensions with an addition to mozconfig like:
```
ac_add_options --disable-legacy-extensions
```

or

```
ac_add_options --enable-legacy-extensions
```

It should automatically default to allowing on Nightly and disallowing elsewhere, but we'll still need overrides for dev edition and unbranded builds.  Setting leave-open so we can get both parts in as part of this bug.

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8898408 [details]
Bug 1363897 Switch legacy extensions handling to option()

https://reviewboard.mozilla.org/r/169808/#review175256

::: toolkit/moz.configure:509
(Diff revision 1)
>  
>  project_flag('MOZ_ANDROID_HISTORY',
>               help='Enable Android History instead of Places',
>               set_as_define=True)
>  
> -project_flag('MOZ_ALLOW_LEGACY_EXTENSIONS',
> +option('--enable-legacy-extensions',

Any particular reason to change to a flag, rather than keep a variable (i.e. option(env='MOZ_ALLOW_LEGACY_EXTENSIONS'... )

Also, you can just set default=milestone.is_nightly

Comment 24

[Andrew Swan [:aswan]]

Comment on attachment 8898408 [details]
Bug 1363897 Switch legacy extensions handling to option()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169808/diff/1-2/

Comment 25

[Andrew Swan [:aswan]]

Comment on attachment 8898408 [details]
Bug 1363897 Switch legacy extensions handling to option()

https://reviewboard.mozilla.org/r/169808/#review175256

> Any particular reason to change to a flag, rather than keep a variable (i.e. option(env='MOZ_ALLOW_LEGACY_EXTENSIONS'... )
> 
> Also, you can just set default=milestone.is_nightly

As explained in the original bug, we (intend to) allow legacy extensions in dev edition and in unbranded builds, which I think requires a flag?

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8898408 [details]
Bug 1363897 Switch legacy extensions handling to option()

https://reviewboard.mozilla.org/r/169808/#review175288

An env variable will do.

Comment 27

[Andrew Swan [:aswan]]

Comment on attachment 8898408 [details]
Bug 1363897 Switch legacy extensions handling to option()

https://reviewboard.mozilla.org/r/169808/#review175288

That would have been a helpful thing to say back at https://bugzilla.mozilla.org/show_bug.cgi?id=1363897#c20

Comment 28

[Mike Hommey [:glandium]]

(In reply to Andrew Swan [:aswan] from comment #27)
> Comment on attachment 8898408 [details]
> Bug 1363897 Switch legacy extensions handling to option()
> 
> https://reviewboard.mozilla.org/r/169808/#review175288
> 
> That would have been a helpful thing to say back at
> https://bugzilla.mozilla.org/show_bug.cgi?id=1363897#c20

In that context, I was assuming that's what you meant: project_flag('MOZ_...') becoming option(env='MOZ_...').

Comment 29

[Andrew Swan [:aswan]]

I think I misunderstood comment 26, sorry for the frustrated outburst.
New patch coming in a bit.

Comment 30

[Andrew Swan [:aswan]]

I'm not sure what I'm doind gwrong, using this fragment in moz.configure:
```
option(env='MOZ_ALLOW_LEGACY_EXTENSIONS',
       default=milestone.is_nightly,
       help='Allow legacy browser extensions')
```

and this in .mozconfig:
```
MOZ_ALLOW_LEGACY_EXTENSIONS=0

```

I get this error from "mach configure":
```
 0:02.93 mozbuild.configure.options.InvalidOptionError: MOZ_INSTRUMENT_EVENT_LOOP takes 0 values
```

Help?

Comment 31

[Mike Hommey [:glandium]]

Set it to nothing.

Comment 32

[Andrew Swan [:aswan]]

Comment on attachment 8898408 [details]
Bug 1363897 Switch legacy extensions handling to option()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/169808/diff/2-3/

Comment 33

[Mike Hommey [:glandium]]

Comment on attachment 8898408 [details]
Bug 1363897 Switch legacy extensions handling to option()

https://reviewboard.mozilla.org/r/169808/#review176678

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/def77bfc578f
Switch legacy extensions handling to option() r=glandium

Comment 35

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

Thank you Mike and Andrew for updating the bug and come with fix patches.
I wish to ask you both if there is something else left for us buildduty to do,I think that in 20 september the release 57 will reach Beta so we may want to finish until then.Please let us know if we can help here with anything else.

Comment 36

[Andrew Swan [:aswan]]

Like I said in comment 22, we'll need overrides for dev edition and unbranded configs.
I set leave-open on this bug so that gets done before resolving, or if you prefer, that can happen in a separate bug.

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 38

[MarjaE]

I am photosensitive, and need more than 20 animation-blocking tools, including 8 animation-blocking extensions, to be able to use the internet. I am stuck in an arms race with advertisers and web designers. Luckily I just get migraines, not seizures.

Do you really want to disable people's safety and accessibility tools?

If you are going to do this, then either (a) clearly warn about the Firefox 57 update, and accept that some people are going to have to keep using Firefox 56 for the forseeable future, (b) make sure every accessibility extension has a 57-compatible version, or (c) make sure Firefox itself is accessible without accessibility extensions. (b) and (c) seem very impractical.

Comment 39

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

As spoken with kmoir yesterday:

>so there used to be add-on builds which were unbranded.  The builds have been replaced by devedition builds and this change is >riding the trains.  It is currently on beta.  On the next uplift (end of month), it will ride the trains to release and there >won't be any addon builds enabled. 

>so what I'm saying is that I think you just need to change the devedition mozconfigs
>not worry about unbranded builds because they are going away at the same time

>unbranded builds will be removed after the next merge

So we only need to add this change to the nightly and devedition as the unbranded builds will be removed after the next merge I think.

Comment 40

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

Attachment: bug1363897_v2.patch

Not sure if this is all that need to be changed.

Comment 41

[Andrew Swan [:aswan]]

Comment on attachment 8902182 [details] [diff] [review]
bug1363897_v2.patch

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

:glandium can give you the right exact syntax, my empirical experience is that you need either
MOZ_ALLOW_LEGACY_EXTENSIONS=''
or
MOZ_ALLOW_LEGACY_EXTENSIONS=1

But more importantly, the configure script defaults to allowing legacy extensions in nightly and not in any other builds, so you only really need to manually override it for dev edition and unbranded (if unbranded builds are still a thing?)

Comment 42

[Mike Hommey [:glandium]]

Comment on attachment 8902182 [details] [diff] [review]
bug1363897_v2.patch

You only need to add either

moz_add_options "export MOZ_ALLOW_LEGACY_EXTENSIONS=1"

or

ac_add_options "MOZ_ALLOW_LEGACY_EXTENSIONS=1"

to the devedition mozconfigs.

Nothing else, since the default is enabled for builds off mozilla-central and integration branches and disabled for all other builds.

Comment 43

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

Attachment: bug1363897_v3.patch

The new patch which enable the option to devedition in mozconfig.
I don't know if we still need maybe to integrate this part from the previous script or change it a little bit:

>diff --git a/browser/confvars.sh b/browser/confvars.sh
>--- a/browser/confvars.sh
>+++ b/browser/confvars.sh
>@@ -67,3 +67,10 @@ MOZ_ADDON_SIGNING=1
> 
> # Include the DevTools client, not just the server (which is the default)
> MOZ_DEVTOOLS=all
>+# Enable legacy extensions in nightly post 57 - Bug1363897
>+MOZ_ALLOW_LEGACY_EXTENSIONS=1
>+if test "$MOZ_UPDATE_CHANNEL" = "beta" -o \
>+        "$MOZ_UPDATE_CHANNEL" = "release" -o \
>+        "$MOZ_UPDATE_CHANNEL" = "esr"; then
>+  MOZ_ALLOW_LEGACY_EXTENSIONS=0
>+fi

Comment 44

[Mike Hommey [:glandium]]

(In reply to Andrei Obreja [:aobreja][:buildduty] from comment #43)
> Created attachment 8902643 [details] [diff] [review]
> bug1363897_v3.patch
> 
> The new patch which enable the option to devedition in mozconfig.
> I don't know if we still need maybe to integrate this part from the previous
> script or change it a little bit:
> 
> >diff --git a/browser/confvars.sh b/browser/confvars.sh
> >--- a/browser/confvars.sh
> >+++ b/browser/confvars.sh
> >@@ -67,3 +67,10 @@ MOZ_ADDON_SIGNING=1
> > 
> > # Include the DevTools client, not just the server (which is the default)
> > MOZ_DEVTOOLS=all
> >+# Enable legacy extensions in nightly post 57 - Bug1363897
> >+MOZ_ALLOW_LEGACY_EXTENSIONS=1
> >+if test "$MOZ_UPDATE_CHANNEL" = "beta" -o \
> >+        "$MOZ_UPDATE_CHANNEL" = "release" -o \
> >+        "$MOZ_UPDATE_CHANNEL" = "esr"; then
> >+  MOZ_ALLOW_LEGACY_EXTENSIONS=0
> >+fi

No, you don't need this.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2723b650460
enable MOZ_ALLOW_LEGACY_EXTENSIONS for devedition configs. r=glandium

Comment 46

[Mihai Tabara [:mtabara]⌚️GMT]

Comment on attachment 8902643 [details] [diff] [review]
bug1363897_v3.patch

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

Comment 47

[Andrei Obreja [:aobreja NOT AVAILABLE][:buildduty]]

Is there something else that we buildduty can help with ,on this bug.

Comment 48

[Andrew Swan [:aswan]]

(In reply to Andrei Obreja [:aobreja][:buildduty] from comment #47)
> Is there something else that we buildduty can help with ,on this bug.

I can't think of anything, but QA should definitely test that everything worked properly when 57 beta and dev edition builds come out.  CC krupa.

Comment 49

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 50

[Mihai Tabara [:mtabara]⌚️GMT]

Merge day is fast approaching, as I understand from RelMan, merge day could happen a week sooner. 


@aswan: Sorry to insist, but just want to double-check:
a) there's nothing else we need to do while 57 is still nightly
b) more testing is needed when this hits beta/devedition.

Re: testing: is this something you mention to QE or does this fall under RelEng's radar? 
Am doing bug triaging and am not sure if this is something to keep under Buildduty's plate or move elsewhere.

Thank you!

Comment 51

[Andrew Swan [:aswan]]

(In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #50)
> Re: testing: is this something you mention to QE or does this fall under
> RelEng's radar? 

I was assuming QE would do it.

Comment 52

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to Andrew Swan [:aswan] (PTO through 9/4) from comment #51)
> (In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #50)
> > Re: testing: is this something you mention to QE or does this fall under
> > RelEng's radar? 
> 
> I was assuming QE would do it.

Definitely. Sorry for confusion, what I meant is who's giving them a heads-up with respect to this? Not sure if testing this falls under regular testing they are performing. If this is already on their plate and they are aware of it, that's awesome and we're all good. Otherwise, if RelEng needs to heads-up them on this or somebody else, just wanted to make sure that's being tracked.

Comment 53

[Andrew Swan [:aswan]]

Krupa, see comment 48 above

Comment 54

[marius.santa]

Attachment: pref not work in beta.gif

This bug is verified on Firefox 57.0b7 (20171009192146) and under Windows 10 64bit/Mac OS X 10.12.3.

Please see the attached video.