Eric Rescorla (:ekr) Reporter
	Description • 9 years ago

I have telemetry turned off in:
about:preferences#advanced

However about:telemetry says:
"Telemetry is enabled."

That leaves me confused.

	Alessio Placitelli [:Dexter] Assignee
	Comment 3 • 9 years ago

I was able to reproduce the issue: this only happens when both FHR and Telemetry are enabled and FHR is then disabled (this should automatically disable Telemetry as well).

Apparently, the line at [1] is not triggering the preference change even though the checkbox gets unticked.

[1] - https://hg.mozilla.org/mozilla-central/annotate/de27ac2ab94f/browser/components/preferences/in-content/advanced.js#l278

	Eric Rescorla (:ekr) Reporter
	Comment 4 • 9 years ago

So you mean that even though we tell the user in the interface that they have disabled telemetry we are actually reporting back? That seems like a fairly seriously issue.

	Alessio Placitelli [:Dexter] Assignee
	Comment 5 • 9 years ago

This patch manually sets the telemetry.enabled preference when FHR is being unticked.

	Alessio Placitelli [:Dexter] Assignee
	Comment 6 • 9 years ago

(In reply to Eric Rescorla (:ekr) from comment #4)
> So you mean that even though we tell the user in the interface that they
> have disabled telemetry we are actually reporting back? That seems like a
> fairly seriously issue.

Reporting itself is controlled by the Health Report setting.

	:Gijs (he/him)
	Comment 7 • 9 years ago

Comment on attachment 8592835 [details] [diff] [review]
bug1154518.patch

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

Nice and simple, with tests - excellent, thanks for the patch!

	:Gijs (he/him)
	Comment 8 • 9 years ago

Should probably consider uplifting something this simple.

That said, this would never have continued to show like this after a restart and/or reopening the preferences... are we sure this is the issue that :ekr was running into?

	:Gijs (he/him)
	Comment 9 • 9 years ago

Eric, is your profile still in this state? What's the state of the preference in about:config ?

	:Gijs (he/him)
	Comment 10 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #8)
> That said, this would never have continued to show like this after a restart
> and/or reopening the preferences... are we sure this is the issue that :ekr
> was running into?

Hmm... unless this code manually unchecks the checkbox every time the prefs are opened, which might be the case? :-\

	Alessio Placitelli [:Dexter] Assignee
	Comment 11 • 9 years ago

(In reply to :Gijs Kruitbosch from comment #8)
> Should probably consider uplifting something this simple.

Yes, I'll try-push this and try uplifting.

> That said, this would never have continued to show like this after a restart
> and/or reopening the preferences... are we sure this is the issue that :ekr
> was running into?

I'm fairly confident that's the issue, as I was able to reproduce it locally: the state of the preference doesn't change in about:config if you follow the steps in comment 3.

	Alessio Placitelli [:Dexter] Assignee
	Comment 12 • 9 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c5fa62a87c6

	Alessio Placitelli [:Dexter] Assignee
	Comment 13 • 9 years ago

I've changed the commit message (added the reviewer).

	Eric Rescorla (:ekr) Reporter
	Comment 14 • 9 years ago

Gijs

Unfortunately, I reset it from about:telemetry

	Pulsebot
	Comment 15 • 9 years ago

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

	Carsten Book [:Tomcat]
	Comment 16 • 9 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 9 years ago

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

	Lawrence Mandel [:lmandel] (use needinfo)
	Comment 18 • 9 years ago

Comment on attachment 8592857 [details] [diff] [review]
bug1154518.patch

Approved for Aurora. For approval request see bug 1139460 comment 42. For approval comments see bug 1139460 comment 43.

Dexter - 38 is marked as affected. This is a simple patch. Do you want to nom this for uplift to Beta as well?

	Georg Fritzsche [:gfritzsche]
	Comment 19 • 9 years ago

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #18)
> Dexter - 38 is marked as affected. This is a simple patch. Do you want to
> nom this for uplift to Beta as well?

I assumed that it is too late. If we can take this, let's do it.

	Georg Fritzsche [:gfritzsche]
	Comment 20 • 9 years ago

Comment on attachment 8592857 [details] [diff] [review]
bug1154518.patch

Approval Request Comment
[Feature/regressing bug #]: FHR/Telemetry unification, bug 1075055.
[User impact if declined]: Telemetry enabled state doesn't match the settings shown in preferences.
[Describe test coverage new/current, TreeHerder]: Manually verified & added automated test-coverage.
[Risks and why]: Low-risk, just explicitly setting prefs to match the displayed state.
[String/UUID change made/needed]: None.

	Georg Fritzsche [:gfritzsche]
	Comment 21 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/a9933a4e1014

	Sylvestre Ledru [:Sylvestre]
	Comment 22 • 9 years ago

Comment on attachment 8592857 [details] [diff] [review]
bug1154518.patch

[Triage Comment]
Sure, should be in 38 beta 8

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 9 years ago

https://hg.mozilla.org/releases/mozilla-release/rev/cb2725c612b2

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 9 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/cb2725c612b2