Closed Bug 1154518 Opened 5 years ago Closed 5 years ago

Conflicting messages about telemetry

Categories

(Toolkit :: Telemetry, defect)

39 Branch
x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- fixed
firefox38.0.5 --- fixed
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: ekr, Assigned: Dexter)

References

Details

Attachments

(3 files, 1 obsolete file)

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

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

That leaves me confused.
Attached image preferences
Attached image about:telemetry
Blocks: 1120356
Flags: needinfo?(alessio.placitelli)
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
Flags: needinfo?(alessio.placitelli)
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.
Attached patch bug1154518.patch (obsolete) — Splinter Review
This patch manually sets the telemetry.enabled preference when FHR is being unticked.
Attachment #8592835 - Flags: review?(gijskruitbosch+bugs)
(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 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!
Attachment #8592835 - Flags: review?(gijskruitbosch+bugs) → review+
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?
Assignee: nobody → alessio.placitelli
Status: NEW → ASSIGNED
Eric, is your profile still in this state? What's the state of the preference in about:config ?
Flags: needinfo?(ekr)
(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? :-\
(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.
Attached patch bug1154518.patchSplinter Review
I've changed the commit message (added the reviewer).
Attachment #8592835 - Attachment is obsolete: true
Attachment #8592857 - Flags: review+
Gijs

Unfortunately, I reset it from about:telemetry
Flags: needinfo?(ekr)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ad181e1e0b53
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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?
Flags: needinfo?(alessio.placitelli)
Attachment #8592857 - Flags: approval-mozilla-aurora+
(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.
Flags: needinfo?(alessio.placitelli)
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.
Attachment #8592857 - Flags: approval-mozilla-beta?
Comment on attachment 8592857 [details] [diff] [review]
bug1154518.patch

[Triage Comment]
Sure, should be in 38 beta 8
Attachment #8592857 - Flags: approval-mozilla-beta? → approval-mozilla-release+
You need to log in before you can comment on or make changes to this bug.