1154518 - Conflicting messages about telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Eric Rescorla (:ekr)]

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

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

That leaves me confused.

Comment 1

[Eric Rescorla (:ekr)]

Attachment: preferences

Comment 2

[Eric Rescorla (:ekr)]

Attachment: about:telemetry

Comment 3

[Alessio Placitelli [:Dexter]]

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

Comment 4

[Eric Rescorla (:ekr)]

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.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: bug1154518.patch

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

Comment 6

[Alessio Placitelli [:Dexter]]

(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.

Comment 7

[:Gijs (he/him)]

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!

Comment 8

[:Gijs (he/him)]

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?

Comment 9

[:Gijs (he/him)]

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

Comment 10

[:Gijs (he/him)]

(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? :-\

Comment 11

[Alessio Placitelli [:Dexter]]

(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.

Comment 12

[Alessio Placitelli [:Dexter]]

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

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: bug1154518.patch

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

Comment 14

[Eric Rescorla (:ekr)]

Gijs

Unfortunately, I reset it from about:telemetry

Comment 15

[Pulsebot]

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

Comment 16

[Carsten Book [:Tomcat]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 19

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 20

[Georg Fritzsche [:gfritzsche]]

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.

Comment 21

[Georg Fritzsche [:gfritzsche]]

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

Comment 22

[Sylvestre Ledru [:Sylvestre]]

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

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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