Closed Bug 1466857 Opened 2 years ago Closed 2 years ago
Ping .handle Update Success() does not respect toolkit .telemetry .update Ping .enabled
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).
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!
Assignee: nobody → chutten
Status: NEW → ASSIGNED
Priority: -- → P1
Annnd janerik points out it's just a patch, not a commit, so it'll need a commit message.
Assignee: chutten → jrediger
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/a5e6f8d6747c Avoid sending an update ping if Telemetry is disabled. r=chutten
Backed out for browser-chrome failures on /browser_UpdatePingSuccess.js Backout link: https://hg.mozilla.org/integration/autoland/rev/6e8579d91075af8245af42f26beb7de23997e934 Push link: https://hg.mozilla.org/integration/autoland/rev/a5e6f8d6747cd421303eed6e7942d19b715fb125 Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=182558158&repo=autoland&lineNumber=4812
"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  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.
Usage of `isTelemetryEnabled` is nearly non-existent (only Telemetry core-internal). I'll check again what we need to check to make sure Telemetry is actually initialized.
: https://searchfox.org/mozilla-central/search?q=isTelemetryEnabled&path= (of course I forget the link...)
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.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/0d0e171c6919 Avoid sending an update ping if Telemetry is disabled. r=chutten
You need to log in before you can comment on or make changes to this bug.