897121 - Defect - Browser error: FlyoutPanelsUI.PrefsFlyout is undefined

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Flyouts, defect, P2)

Description

[Jim Mathies [:jimm]]

[JavaScript Error: "FlyoutPanelsUI.PrefsFlyout is undefined" {file: "chrome://mozapps/content/extensions/setting.xml" line: 71}]

I see this a lot while running tests.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

The constructor for the XBL binding, "setting," calls its "preferenceChanged" handler [1].

In browser.xul, there is a setting whose "preferenceChanged" handler tries to call a function defined in our prefs flyout [2].

FlyoutPanelsUI.PrefsFlyout is not initialized until FlyoutPanelsUI.init is called, so when the "setting" constructor from [1] runs and tries to call the "preferenceChanged" handler from [2], we see that FlyoutPanelsUI.PrefsFlyout is undefined.

I'm not yet sure what the best approach to fixing this would be. Perhaps we should hold off on setting the "preferenceChanged" handler until the initialization of the prefs flyout. At that point we could also check the value of the pref and make sure we set its initial value correctly.

[1] https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/setting.xml?rev=f56958fbcc5b#23
[2] https://mxr.mozilla.org/mozilla-central/source/browser/metro/base/content/browser.xul?rev=bf035bca7f65#707

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Based on the analysis above, I'm betting the user impact of this bug is that the "do not track" item in the prefs flyout doesn't accurately reflect the "do not track" preference when the metro browser starts up.

That would mean that this is causing bug 898641 and possibly also bug 898643.

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: 897121.patch

Turns out we were trying to do too much. If we just let the <setting>s do their thing, they work perfectly.

This also fixes bug 898641 and bug 898643.

Comment 4

[Marco Mucci [:MarcoM]]

Hey Tim, will bug 898641 and bug 898643 become resolved after Bug 897121 lands or will any work need to be done to them?

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Marco Mucci [:MarcoM] from comment #4)
> Hey Tim, will bug 898641 and bug 898643 become resolved after Bug 897121
> lands or will any work need to be done to them?

From my testing, it looks like no additional work will be needed.

Comment 6

[Marco Mucci [:MarcoM]]

That's great.  Thanks for the update Tim.

Comment 7

[Matt Brubeck (:mbrubeck)]

Comment on attachment 783851 [details] [diff] [review]
897121.patch

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

::: browser/metro/base/content/flyoutpanels/PrefsFlyoutPanel.js
@@ -46,5 @@
> -  onDNTPreferenceChanged: function onDNTPreferenceChanged() {
> -    let selected = this._elements.dntNoPref.selected;
> -
> -    // When "tell sites nothing about my preferences" is selected, disable do not track.
> -    Services.prefs.setBoolPref("privacy.donottrackheader.enabled", !selected);

I think this is still needed.  Note that there are two different privacy.donottrackheader.* preferences, and in some cases we need to update both.  The <setting> only updates one of them.

Comment 8

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 783851 [details] [diff] [review]
897121.patch

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

Thanks for looking into this! The only issue is what Matt pointed out.

::: browser/metro/base/content/flyoutpanels/PrefsFlyoutPanel.js
@@ -46,5 @@
> -  onDNTPreferenceChanged: function onDNTPreferenceChanged() {
> -    let selected = this._elements.dntNoPref.selected;
> -
> -    // When "tell sites nothing about my preferences" is selected, disable do not track.
> -    Services.prefs.setBoolPref("privacy.donottrackheader.enabled", !selected);

Yes, this is still needed. The "enabled" pref enables/disables the header while "value" pref is for your tracking options. You can do the binding on init. You can use http://donottrack.us/ to verify.

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

(In reply to Matt Brubeck (:mbrubeck) from comment #7)
> I think this is still needed.  Note that there are two different
> privacy.donottrackheader.* preferences, and in some cases we need to update
> both.  The <setting> only updates one of them.

Turns out I had 'privacy.donottrackheader.enabled' set to true, so modifying 'privacy.donottrackheader.value' was working perfectly (as tested on http://donottrack.us and also in the flyout UI).

From looking at the way 'privacy.donottrackheader.enabled' and 'privacy.donottrackheader.value' are used [1], I don't see a reason for having both of them. ISTM that we have 3 distinct desired behavioral outcomes (send a 0 in the dnt header, send a 1 in the dnt header, don't send the dnt header) and that we have 2 prefs which are both capable of representing 3 states (-1, 0, 1 for privacy.donottrackheader.value and true,false,non-existent for privacy.donottrackheader.enabled).

Any insight into why we have these two separate prefs?

[1] https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp?rev=14bd6018f287#418


In any case, the attached patch keeps the two prefs in sync with each other and with the UI in our prefs flyout.

Comment 10

[Asa Dotzler [:asa]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #9)
> Created attachment 783950 [details] [diff] [review]
> Patch v2
> 
> (In reply to Matt Brubeck (:mbrubeck) from comment #7)
> > I think this is still needed.  Note that there are two different
> > privacy.donottrackheader.* preferences, and in some cases we need to update
> > both.  The <setting> only updates one of them.
> 
> Turns out I had 'privacy.donottrackheader.enabled' set to true, so modifying
> 'privacy.donottrackheader.value' was working perfectly (as tested on
> http://donottrack.us and also in the flyout UI).
> 
> From looking at the way 'privacy.donottrackheader.enabled' and
> 'privacy.donottrackheader.value' are used [1], I don't see a reason for
> having both of them. ISTM that we have 3 distinct desired behavioral
> outcomes (send a 0 in the dnt header, send a 1 in the dnt header, don't send
> the dnt header) and that we have 2 prefs which are both capable of
> representing 3 states (-1, 0, 1 for privacy.donottrackheader.value and
> true,false,non-existent for privacy.donottrackheader.enabled).
> 
> Any insight into why we have these two separate prefs?
> 
> [1]
> https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpHandler.cpp?rev=14bd6018f287#418
> 
> 
> In any case, the attached patch keeps the two prefs in sync with each other
> and with the UI in our prefs flyout.

Maybe Sid knows? Cc'ing him.

Comment 11

[Rodrigo Silveira [:rsilveira] [:rodms]]

Comment on attachment 783950 [details] [diff] [review]
Patch v2

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

Nice solution using prefs observer!

::: browser/metro/base/content/flyoutpanels/PrefsFlyoutPanel.js
@@ +38,5 @@
>        SanitizeUI.init();
>        this._hasShown = true;
> +
> +      Services.prefs.addObserver("privacy.donottrackheader.value",
> +                                 this._prefObserver.bind(this),

nit: The bind call is fine, but you're not using 'this' at _prefObserver so it's unnecessary.

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v3

(In reply to Rodrigo Silveira [:rsilveira] from comment #11)
> Comment on attachment 783950 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 783950 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice solution using prefs observer!

Thanks!

> ::: browser/metro/base/content/flyoutpanels/PrefsFlyoutPanel.js
> @@ +38,5 @@
> >        SanitizeUI.init();
> >        this._hasShown = true;
> > +
> > +      Services.prefs.addObserver("privacy.donottrackheader.value",
> > +                                 this._prefObserver.bind(this),
> 
> nit: The bind call is fine, but you're not using 'this' at _prefObserver so
> it's unnecessary.

Oops, good catch. Fixed in the attached patch.


Note1: Carrying forward r+.
Note2: Some of the logic in this patch is implemented in various places in fennec, Firefox desktop, and b2g. I'm not sure how, but it'd be really nice if we could centralize the logic (maybe in the pref service itself?)

Comment 13

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

https://hg.mozilla.org/integration/fx-team/rev/959c593c6bd3

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/959c593c6bd3