UpdatePing.handleUpdateSuccess() does not respect toolkit.telemetry.updatePing.enabled

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: mcs, Assigned: janerik)

Tracking

Trunk
mozilla62
Points:
---

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [tor][tor 25909])

Attachments

(2 attachments)

After a successful application update, code inside browser/components/nsBrowserContentHandler.js directly calls UpdatePing.handleUpdateSuccess(). The handleUpdateSuccess() function should consult the toolkit.telemetry.updatePing.enabled preference but it does not.

When telemetry is disabled (as it is by default in Tor Browser), this leads to errors inside the telemetry code because it has not been initialized. If telemetry is enabled but toolkit.telemetry.updatePing.enabled = false, presumably a ping is sent that should not be.

For the corresponding Tor Browser bug report, see https://trac.torproject.org/projects/tor/ticket/25909
I attached the patch we are using in Tor Browser (mozilla-esr60 branch).
Component: Application Update → Telemetry
Comment on attachment 8983420 [details] [diff] [review]
UpdatePing.patch

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

This looks like exactly what we need. Thank you!
Attachment #8983420 - Flags: review+
Assignee: nobody → chutten
Status: NEW → ASSIGNED
Keywords: checkin-needed
Priority: -- → P1
Annnd janerik points out it's just a patch, not a commit, so it'll need a commit message.
Assignee: chutten → jrediger
Keywords: checkin-needed
Comment on attachment 8984500 [details]
Bug 1466857 - Avoid sending an update ping if Telemetry is disabled. r?chutten

Chris H-C :chutten has approved the revision.

https://phabricator.services.mozilla.com/D1596
Attachment #8984500 - Flags: review+
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a5e6f8d6747c
Avoid sending an update ping if Telemetry is disabled. r=chutten
"toolkit.telemetry.enabled", which `isTelemetryEnabled` is querying, is a locked preference in the test (rowser_UpdatePingSuccess.js), so I can't set it to enabled to make the code work.
In addition the documentation [1] lists it as kinda deprecated, so I'm not even sure this would be the right preference to check.

Is there another of the preference we should use instead (e.g. `datareporting.healthreport.uploadEnabled`) or is `toolkit.telemetry.updatePing.enabled` all we actually want?
Flags: needinfo?(jrediger) → needinfo?(chutten)
Wait, isTelemetryEnabled is asking toolkit.telemetry.enabled? Eurgh. That's more or less fine, but we need to be careful about who uses this and what for, since it will be true in instances when you shouldn't send data and false in instances where we send limited data. (it isn't descriptively named)

Hrm, what do we want to actually query... It could be canRecordBase/canRecordPrereleaseData, it could be the pref datareporting.healthreport.uploadEnabled... 

Actually, the bug is around the update ping being called when it isn't enabled (or possibly even init'd). We should ensure it is init and enabled before doing anything on this (or any other) API function.
Flags: needinfo?(chutten)
Usage of `isTelemetryEnabled` is nearly non-existent[1] (only Telemetry core-internal).
I'll check again what we need to check to make sure Telemetry is actually initialized.
I am confused by phabricator and have no idea how to flag you for review again. I changed the code to only check the enabled flag. Please review again.
Flags: needinfo?(chutten)
r+
Flags: needinfo?(chutten)
Pushed by jrediger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d0e171c6919
Avoid sending an update ping if Telemetry is disabled. r=chutten
https://hg.mozilla.org/mozilla-central/rev/0d0e171c6919
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.