1245277 - pocket and loop .enabled preferences can no longer be set via distribution.ini/autoconf/esr/etc

Status: VERIFIED FIXED

(Firefox :: Pocket, defect)

Description

[Mike Kaply [:mkaply]]

This is important for enterprise.

Setting browser.pocket.enabled to false doesn't appear to work anymore in distribution.ini or the CCK2.

Going into about:config shoes the preferences as false, but pocket is still there and working.

To recreate, create a distribution/distribution.ini file:

[Global]
id=test
version=1.0
about=test

[Preferences]
# Don't use pocket
browser.pocket.enabled=false

Start Firefox with a new profile.

Pocket is enabled.

Comment 1

[Shane Caraveo (:mixedpuppy)]

What version of Firefox?  As of 46, pocket is a system addon.  The addon should be upgrading that pref value to the new pref name extensions.pocket.enabled if the user previously set it.  I would have assumed that distributions.js beats addons so the pref should be upgraded.  In any case, it is probably useful to also set extensions.pocket.enabled=false

Comment 2

[Mike Kaply [:mkaply]]

It was Firefox 45 actually.

What's odd is that it's not enabled, but the icon still shows up.

When you click on the icon, it doesn't work.

I'll do some more testing.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Not terribly surprised.  The question becomes, do we fix for 45?  Not sure it could even be fixed for 45 at this stage.

Comment 4

[Mike Kaply [:mkaply]]

The pref still works fine, just not in distribution.ini and other places.

I'd like to at least understand the why on this. Hopefully something simple.

Especially since FF45 is the ESR.

Comment 5

[Mike Kaply [:mkaply]]

I think we might have a bigger problem here.

Previously, when this code was executed:

http://mxr.mozilla.org/mozilla-beta/source/browser/components/customizableui/CustomizableWidgets.jsm#1222

distribution.ini had been read in. For some reason now it is not.

This could mean differences in partner distribution loading.

I'm researching.

Comment 6

[Mike Kaply [:mkaply]]

This regressed between:

2015-11-30-03-02-28-mozilla-central
and
2015-12-01-03-02-26-mozilla-central

2015-11-30-03-02-28-mozilla-central was built from:
https://hg.mozilla.org/mozilla-central/rev/47b49b0d32360fab04b11ff9120970979c426911

2015-12-01-03-02-26-mozilla-central was built from:
https://hg.mozilla.org/mozilla-central/rev/66a6d7ec9534b9d7847b665142fef0dd87623768

Comment 7

[Mike Kaply [:mkaply]]

The first bad revision is:
changeset:   274524:722d3a01dcfc
user:        Mark Banner <[e-mail]>
date:        Sun Nov 29 17:08:35 2015 +0000
summary:     Bug 1223573 - Part 7. Add support in bootstrap.js for starting Loop and displaying the button. Also get all tests passing again. r=mikedeboer

Comment 8

[Mike Kaply [:mkaply]]

So specifically, this call:

http://mxr.mozilla.org/mozilla-beta/source/browser/extensions/loop/bootstrap.js#815

Is causing CustomizableUI to get initialized much earlier than it used to.

Comment 9

[:Gijs (out for now; he/him)]

(In reply to Mike Kaply [:mkaply] from comment #8)
> So specifically, this call:
> 
> http://mxr.mozilla.org/mozilla-beta/source/browser/extensions/loop/bootstrap.
> js#815
> 
> Is causing CustomizableUI to get initialized much earlier than it used to.

It might make sense to have CUI dispatch an observer notification add-ons can listen to to add their widget? Not sure how that deals with installing restartless add-ons post-startup. I guess they could detect whether CUI is loaded using Cu.isModuleLoaded?

Comment 10

[Shane Caraveo (:mixedpuppy)]

Conversing with Mike in irc. seems to be multiple issues related to this.

- CUI initialized prior to distribution customization modifying prefs
- In 46, Pocket addon doesn't respect a previously set default pref
- Loop addon doesn't appear to respect a previously set default pref
- Loop creates the widget even if loop.enabled=false
- Loop problem exists in 45, which is next ESR, unknown if enterprises are disabling loop via autoconf

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: fix pocket pref use

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: fix loop pref use

This particular patch would need uplift to beta to fix ESR/autoconf/distribution.ini

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Regression for Loop and Pocket, looks like a regression from last November/December in 45. 
Tracking and marking affected.

Comment 14

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs Kruitbosch from comment #9)
> (In reply to Mike Kaply [:mkaply] from comment #8)
> > So specifically, this call:
> > 
> > http://mxr.mozilla.org/mozilla-beta/source/browser/extensions/loop/bootstrap.
> > js#815
> > 
> > Is causing CustomizableUI to get initialized much earlier than it used to.
> 
> It might make sense to have CUI dispatch an observer notification add-ons
> can listen to to add their widget? Not sure how that deals with installing
> restartless add-ons post-startup. I guess they could detect whether CUI is
> loaded using Cu.isModuleLoaded?

I think it would be good to ensure the distribution pref customization prior to addonmanager loading addons.  Seems that would address one part of the problem.  The second part is that an addon can set a default pref overwriting what distribution wants.  Maybe the distribution prefs could be locked until after addon startup is complete.

Comment 15

[Mike Kaply [:mkaply]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #14)

> I think it would be good to ensure the distribution pref customization prior
> to addonmanager loading addons.  Seems that would address one part of the
> problem.  The second part is that an addon can set a default pref
> overwriting what distribution wants.  Maybe the distribution prefs could be
> locked until after addon startup is complete.

Even if they were locked, an add-on could unlock and set, so I don't know that would buy us much.

Unfortunately we really never built a good default prefs story for restartless add-ons, but we're stuck with what we have.

The key for us is when we switch built in stuff to be add-ons, we need to be smart about our default prefs (and encourage add-on others to do the same).

And we should make sure we have a good default pref story in WebExtensions.

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to Mike Kaply [:mkaply] from comment #15)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #14)
> 
> > I think it would be good to ensure the distribution pref customization prior
> > to addonmanager loading addons.  Seems that would address one part of the
> > problem.  The second part is that an addon can set a default pref
> > overwriting what distribution wants.  Maybe the distribution prefs could be
> > locked until after addon startup is complete.
> 
> Even if they were locked, an add-on could unlock and set, so I don't know
> that would buy us much.

I think that's ok.  It becomes an explicit decision to do that, rather than an oversight.

Comment 17

[Mike de Boer [:mikedeboer]]

Comment on attachment 8723219 [details] [diff] [review]
fix loop pref use

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

I'm moving back to desktop - deferring to Mark, if you don't mind, Shane!

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 8723219 [details] [diff] [review]
fix loop pref use

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

::: browser/extensions/loop/bootstrap.js
@@ +955,5 @@
>  function loadDefaultPrefs() {
>    var branch = Services.prefs.getDefaultBranch("");
>    Services.scriptloader.loadSubScript("chrome://loop/content/preferences/prefs.js", {
>      pref: (key, val) => {
> +      // if a previously set default exists, don't overwrite (e.g. distribution.ini)

nit: please make the comments into proper sentences (capital & full stop).

@@ +957,5 @@
>    Services.scriptloader.loadSubScript("chrome://loop/content/preferences/prefs.js", {
>      pref: (key, val) => {
> +      // if a previously set default exists, don't overwrite (e.g. distribution.ini)
> +      if (branch.getPrefType(key) != branch.PREF_INVALID)
> +        return;

nit: please include braces for all of the if statements you've added as per the prevailing style.

@@ +1037,5 @@
> +
> +  loadDefaultPrefs();
> +
> +  // watch pref change and enable/disable if necessary
> +  Services.prefs.addObserver("loop.enabled", prefObserver, false);

We need a way to remove the observer on real shutdown of the add-on. Otherwise we might have references left hanging around that'll confuse things on upgrade.

@@ +1038,5 @@
> +  loadDefaultPrefs();
> +
> +  // watch pref change and enable/disable if necessary
> +  Services.prefs.addObserver("loop.enabled", prefObserver, false);
> +  if (Services.prefs.getPrefType("loop.enabled")  == Services.prefs.PREF_INVALID ||

nit: only one space before ==

Comment 19

[:Gijs (out for now; he/him)]

Comment on attachment 8723218 [details] [diff] [review]
fix pocket pref use

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

::: browser/extensions/pocket/bootstrap.js
@@ +522,5 @@
>        Services.prefs.clearUserPref("browser.pocket.enabled");
>      }
>      // watch pref change and enable/disable if necessary
>      Services.prefs.addObserver("extensions.pocket.enabled", prefObserver, false);
> +    if (Services.prefs.getPrefType("extensions.pocket.enabled")  == Services.prefs.PREF_INVALID ||

This could only happen if something removed the pref from the default branch in between us adding it and us now checking it, but all that code is sync, right? So presumably we could just remove this check?

If there's some reason to keep it, then nit: one space before "==".

Comment 20

[Shane Caraveo (:mixedpuppy)]

Attachment: fix loop pref use, smaller patch for uplift

This is a minimal patch to support autoconfig and distribution.ini prefs.  Changing loop.enabled requires a restart unlike the larger patch.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: fix loop pref use

address earlier review comments.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 8723268 [details] [diff] [review]
fix loop pref use

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

::: browser/extensions/loop/bootstrap.js
@@ +1041,5 @@
> +
> +  // Watch pref changes and enable/disable if necessary.
> +  Services.prefs.addObserver("loop.enabled", prefObserver, false);
> +  if (Services.prefs.getPrefType("loop.enabled") == Services.prefs.PREF_INVALID ||
> +      !Services.prefs.getBoolPref("loop.enabled"))

nit: missing braces for the if statement.

@@ +1079,5 @@
>    // it fade away...
>    if (reason == APP_SHUTDOWN) {
>      return;
>    }
> +  Services.prefs.removeObserver("loop.enabled", prefObserver);

We probably want to make this conditional on if shutdown is being called from the prefObserver or not - otherwise the loop.enabled would toggle off in restartless fashion, but not toggle on again.

Comment 23

[Shane Caraveo (:mixedpuppy)]

per irc, landing the smaller loop patch in case it is accepted for uplift.  we'll work out the larger patch later.

Comment 24

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/53b10ff5a66259400a440e03a135395a397f9962
Bug 1245277 pocket addon needs to respect prior set default prefs, r=Gijs

Comment 25

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/f184bfcbbc460851820579855b40d64ac621384c
Bug 1245277 loop addon needs to respect prior set default prefs, not restartless, r=Standard8

Comment 26

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8723267 [details] [diff] [review]
fix loop pref use, smaller patch for uplift

uplift for aurora, beta and ESR45 which is already branched but no flag here to request it.

Approval Request Comment
[Feature/regressing bug #]:loop system addon
[User impact if declined]:autoconfig, distriubtion.ini, ESR etc. are unable to pref off our system addons
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: late beta is always high.  low for aurora.
[String/UUID change made/needed]:none

Comment 27

[Shane Caraveo (:mixedpuppy)]

Attachment: fix pocket pref use

This is the patch that landed with prior review comments addressed.

Comment 28

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8723282 [details] [diff] [review]
fix pocket pref use

Approval Request Comment
[Feature/regressing bug #]:pocket system addon
[User impact if declined]:autoconfig, distriubtion.ini, ESR etc. are unable to pref off our system addons
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: low for aurora.
[String/UUID change made/needed]:none

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53b10ff5a662
https://hg.mozilla.org/mozilla-central/rev/f184bfcbbc46

Comment 30

[Mark Banner (:standard8)]

I pushed the Loop patch to the loop repo as well. Shane, sorry I forgot to attribute you on there:

https://github.com/mozilla/loop/commit/07fac0925ec62b54c8d9e900f20df07f734be4eb

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8723267 [details] [diff] [review]
fix loop pref use, smaller patch for uplift

We indeed want this in 45, taking it.
Should be in 45 beta 10

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f29d0f7b017e
https://hg.mozilla.org/releases/mozilla-beta/rev/30883d70b037

Comment 33

[Mike Kaply [:mkaply]]

Does ESR 45 still use the beta branch or has it branched separately?

Comment 34

[:Gijs (out for now; he/him)]

(In reply to Mike Kaply [:mkaply] from comment #33)
> Does ESR 45 still use the beta branch or has it branched separately?

It hasn't branched yet. I think it only branches when mozilla-beta gets merged into mozilla-release. In any case, for questions like this, ask on IRC, mailing lists, or needinfo someone. Just commenting in bugs isn't an effective way of asking a question like that.

Comment 35

[Shane Caraveo (:mixedpuppy)]

Standard8, I think the larger patch I initially proposed should still be considered for loop, though the shutdown aspect needs a bit more thought.  Can that just pass on to someone one the loop team?

Comment 36

[Mark Banner (:standard8)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #35)
> Standard8, I think the larger patch I initially proposed should still be
> considered for loop, though the shutdown aspect needs a bit more thought. 
> Can that just pass on to someone one the loop team?

That would be fine, can I talk you into filing a bug in the Loop product with an outline, we can then schedule it in.

Comment 37

[Bogdan Maris, Desktop Test Engineering]

I used distribution.ini file based on comment 0 (with loop.enabled pref added as false) but I can see that both old Nightly (2015-12-01 as reported in comment 6) and Firefox 45 beta 10 behave the same: Pocket does not go away from Toolbar but Hello does, even though both prefs are successfully modified to 'false'.
Am I missing something?

Comment 38

[Shane Caraveo (:mixedpuppy)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #37)
> I used distribution.ini file based on comment 0 (with loop.enabled pref
> added as false) but I can see that both old Nightly (2015-12-01 as reported
> in comment 6) and Firefox 45 beta 10 behave the same: Pocket does not go
> away from Toolbar but Hello does, even though both prefs are successfully
> modified to 'false'.
> Am I missing something?

a) I dont see that the pocket patch got uplifted, so only verify pocket on nightly
b) the pref for pocket is extensions.pocket.enabled

Comment 39

[Mike Kaply [:mkaply]]

> the pref for pocket is extensions.pocket.enabled

To clarify, this is starting with Firefox 46

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8723282 [details] [diff] [review]
fix pocket pref use

OK for uplift

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

Sorry, looks like we missed the loop patch.  Did that make it into esr?   Esr did already branch by the way.

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Oh,  not the loop one. Your 2nd pocket patch.  approving it now, anyway, for aurora.

Comment 43

[Shane Caraveo (:mixedpuppy)]

I beleive the loop patch made it, just the pocket patch needs to get into 46

Comment 44

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Based on comment 29, this is fixed in Fx 47 as well (when it was on Nightly channel).

Comment 45

[Loic]

Is it fixed in the new release 46?

A user reported the same issue with extensions.pocket.enabled ignored in /usr/lib/firefox/browser/defaults/preferences/prefs.js

Comment 46

[Mark Banner (:standard8)]

It looks like what happened was that the Loop patches landed in comment 32, and set the status to fixed.

The pocket patch was later approved in comment 40, but hasn't been landed (and was missed due to the previous "fixed" status).

I'm guessing the leave-open was also forgotten about, which is why this bug doesn't have status "fixed".

Liz/Shane, we probably need some discussion/decision here.

Comment 47

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking this as affected for 46/47, then. Is this still an issue in ESR too?
Shane, can you fix this outside of a point release using the system addon?

Comment 48

[Shane Caraveo (:mixedpuppy)]

This does not affect fx45 [pocket was not a system addon in that], iirc 45 is esr, so this does not affect esr.

The primary issue here is with distributions, this is not a user-facing problem.  ie. the pref still works if a user goes into about:config and turns off pocket.

This also makes the fix more difficult, distributions need the fix "up front" not as a later upgrade to the addon.

@mkpaly do you know who/if any distributions are turning off pocket and are affected by this with fx46?  If so, can they live with it for a version?

In the meantime, this should get landed on beta.

Comment 49

[Mike Kaply [:mkaply]]

> do you know who/if any distributions are turning off pocket and are affected by this with fx46?  If so, can they live with it for a version?

Not distributions specifically, but enterprise customizations. The bug that found this (bug 1267946) seems to indicate that the preference doesn't work in any of the normal locations except about:config.

For 46, I would probably just recommend folks remove the XPI from their distribution. That's the easiest way to disable it...

I don't think it's worth trying to get Pocket updated for this one thing...

Comment 50

[Shane Caraveo (:mixedpuppy)]

Ok, so wontfix for 46, but lets uplift through beta?

Comment 51

[Liz Henry (:lizzard) (relman/hg->git project)]

whatever patch you need to uplift, please nominate it for the appropriate branch.

Comment 52

[Shane Caraveo (:mixedpuppy)]

hrm.  it's in mozilla-beta (47) already, we'll just leave it at that.

Comment 53

[Bogdan Maris, Desktop Test Engineering]

I used distribution.ini file with loop.enabled and extensions.pocket.enabled/browser.pocket.enabled for ESR prefs added as false, and I can confirm that both Pocket and Loop are disabled. I used Firefox 47 beta 9 and latest pre ESR 45.1.2 build on Windows 7 64bit, Ubuntu 14.04 64bit and Mac OS X 10.10.5.

Comment 54

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.