1346196 - URLCLASSIFIER_UPDATE_REMOTE_STATUS2 records values using empty keys

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement)

Description

[Alessio Placitelli [:Dexter]]

With bug 1334469 we're disallowing using empty keys to record data in Telemetry keyed histograms.

It seems that this histogram is recording values using empty keys (see bug 1334469 comment 20).

For some reason, |mTelemetryProvider| is empty [1]. We should fix the problem by using a non-empty key here and investigate why and empty key is used in the first place.

[1] - http://searchfox.org/mozilla-central/rev/7cb75d87753de9103253e34bc85592e26378f506/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#694

Comment 1

[Dimi Lee [:dimi]]

After discussing with Thomas, I think thomas make a good point that this may cause by CancelUpdate because in that case |mTelemetryProvider| is reset to empty[1] but onStartRequest will still be called.

[1] http://searchfox.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#102

Comment 2

[Dimi Lee [:dimi]]

Attachment: Bug 1346196 - URLCLASSIFIER_UPDATE_REMOTE_STATUS2 records values using empty keys.

mTelemetryProvider might be empty if |CancelUpdate| is called during update.
In that scenario, the call flow will look like:
CancelUpdate -> UpdateError -> DownloadDone -> set mTelemetryProvider to empty ->
OnStartRequest -> record telemetry with empty key.

So in this patch, move mTelemetryProvider.Truncate() to OnStopRequest.

Review commit: https://reviewboard.mozilla.org/r/120504/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120504/

Comment 3

[François Marier [:francois]]

Comment on attachment 8847510 [details]
Bug 1346196 - URLCLASSIFIER_UPDATE_REMOTE_STATUS2 records values using empty keys.

https://reviewboard.mozilla.org/r/120504/#review122628

While there are probably edge cases where we will have an old telemetry provider lying around after an error, this looks fine because as far as I can see, we always get the right provider before we submit a telemetry probe.

::: commit-message-f9362:2
(Diff revision 1)
> +Bug 1346196 - URLCLASSIFIER_UPDATE_REMOTE_STATUS2 records values using empty keys. r?francois
> +mTelemetryProvider might be empty if |CancelUpdate| is called during update.

Not sure if this is required by mercurial, but in git, you need a blank line between the short description and the long description.

Comment 4

[Dimi Lee [:dimi]]

Comment on attachment 8847510 [details]
Bug 1346196 - URLCLASSIFIER_UPDATE_REMOTE_STATUS2 records values using empty keys.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120504/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aecbad803701
URLCLASSIFIER_UPDATE_REMOTE_STATUS2 records values using empty keys. r=francois

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/aecbad803701