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

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Mark Smith [:mcs]]

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

Comment 1

[Mark Smith [:mcs]]

Attachment: UpdatePing.patch

I attached the patch we are using in Tor Browser (mozilla-esr60 branch).

Comment 2

[Chris H-C :chutten]

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!

Comment 3

[Chris H-C :chutten]

Annnd janerik points out it's just a patch, not a commit, so it'll need a commit message.

Comment 4

[Phabricator Automation]

Attachment: Bug 1466857 - Avoid sending an update ping if Telemetry is disabled. r?chutten

Comment 5

[Phabricator Automation]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5e6f8d6747c
Avoid sending an update ping if Telemetry is disabled. r=chutten

Comment 7

[Narcis Beleuzu [:NarcisB]]

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

Comment 8

[Jan-Erik Rediger [:janerik]]

"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?

Comment 9

[Chris H-C :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.

Comment 10

[Jan-Erik Rediger [:janerik]]

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.

Comment 11

[Jan-Erik Rediger [:janerik]]

[1]: https://searchfox.org/mozilla-central/search?q=isTelemetryEnabled&path=

(of course I forget the link...)

Comment 12

[Jan-Erik Rediger [:janerik]]

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.

Comment 13

[Chris H-C :chutten]

r+

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d0e171c6919
Avoid sending an update ping if Telemetry is disabled. r=chutten

Comment 15

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0d0e171c6919