Closed Bug 1466857 Opened 4 years ago Closed 4 years ago

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


(Toolkit :: Telemetry, enhancement, P1)




Tracking Status
firefox62 --- fixed


(Reporter: mcs, Assigned: janerik)


(Whiteboard: [tor][tor 25909])


(2 files)

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
Attached patch UpdatePing.patchSplinter Review
I attached the patch we are using in Tor Browser (mozilla-esr60 branch).
Component: Application Update → Telemetry
Comment on attachment 8983420 [details] [diff] [review]

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

This looks like exactly what we need. Thank you!
Attachment #8983420 - Flags: review+
Assignee: nobody → chutten
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.
Attachment #8984500 - Flags: review+
Pushed by
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)
Flags: needinfo?(chutten)
Pushed by
Avoid sending an update ping if Telemetry is disabled. r=chutten
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.