1120379 - Telemetry needs to send ping deletion messages to the server when FHR is deactivated

Status: VERIFIED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Georg Fritzsche [:gfritzsche]]

When Telemetry is turned off by a user, the client needs to send a deletion message to the server, requesting that all associated data for the user will be deleted.

Comment 1

[Georg Fritzsche [:gfritzsche]]

Clarification: bug 1075055 makes FHR & Telemetry dependent pref-wise. This only needs to send the deletion ping when FHR is turned off.

Comment 2

[Georg Fritzsche [:gfritzsche]]

For this TelemetryPing should add a pref observer for PREF_FHR_UPLOAD_ENABLED.
That observer would trigger sending a deletion ping with the client id, no environment and an empty payload:

> this.send(PING_TYPE_DELETION, {}, { addClientId: true });

We need to update the documentation here:
https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/toolkit/components/telemetry/telemetry/pings.html#ping-types
... and should add a simple deletion-ping.rst similar to main-ping.rst describing the motivation and format.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Note that we need to init the pref observer early, but can't send the ping until the delayed initialization finished.
If this._initialized==false we should set a flag, say _sendDeletionPing. The _delayedInitTask should check this once things are set up and send one.

Edge-case: The pref changed between Firefox sessions (e.g. manual profile modification or 3rd party).

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] [away Feb 27 - March 15] from comment #3)
> Edge-case: The pref changed between Firefox sessions (e.g. manual profile
> modification or 3rd party).

Do we need to care about this?

Comment 5

[Benjamin Smedberg]

Changing from outside Firefox? No.

Comment 6

[Alessio Placitelli [:Dexter]]

Should we delete the pings saved locally when this preference is switched to false?

Comment 7

[Benjamin Smedberg]

No. In fact, we should be saving the pings locally always, no matter what the preference is set to. But I forgot to list that as a requirement, so I'll file that as a followup.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Send DELETE PING when disabling FHR upload

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage for the delete pings.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8584508 [details] [diff] [review]
part 1 - Send DELETE PING when disabling FHR upload

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

::: toolkit/components/telemetry/TelemetryPing.jsm
@@ +888,5 @@
> +   * pings are not sent to the server (Telemetry is a sub-feature of FHR). If trying
> +   * to send a deletion ping, don't block it.
> +   *
> +   * @param {Object} [ping=null] A ping to be checked.
> +   * @return {Boolean} True if we are allwed to send pings to the server, false otherwise.

Nit: allwed vs. allowed.

@@ +893,3 @@
>     */
> +  _canSend: function(ping = null) {
> +    // Deletion pings are sent when

when...?

@@ +893,5 @@
>     */
> +  _canSend: function(ping = null) {
> +    // Deletion pings are sent when
> +    let deletionPing = !!ping && isNewPingFormat(ping) &&
> +                       (ping.type == PING_TYPE_DELETION);

const isDeletionPing = ...
Would probably be cleaner though as just a helper isDeletionPing(ping).

@@ +898,2 @@
>      return (Telemetry.isOfficialTelemetry || this._testMode) &&
> +           (Preferences.get(PREF_FHR_UPLOAD_ENABLED, false) || deletionPing);

This would be more readable if its not all in one condition.
E.g. early-out:
if (isDeletionPing(ping)) {
  return true;
}

...

@@ +923,5 @@
> +   * Called whenever the FHR Upload preference changes (e.g. when user disables FHR from
> +   * the preferences panel), this triggers sending the deletion ping.
> +   */
> +  _onUploadPrefChange: function() {
> +    let uploadEnabled = Preferences.get(PREF_FHR_UPLOAD_ENABLED, false);

const

@@ +931,5 @@
> +    }
> +
> +    if (!this._initialized) {
> +      this._sendDeletionPing = true;
> +      this._log.trace("_onUploadPrefChange - Scheduling deletion ping after the initialization.");

In bug 1149754, we need to make at least TelemetryPing.send() work before init anyway (i.e. allowing to add pending pings before full TelemetryPing init).

Maybe we should just base this bug on that and drop juggling with the flag here?

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #10)
> @@ +931,5 @@
> > +    }
> > +
> > +    if (!this._initialized) {
> > +      this._sendDeletionPing = true;
> > +      this._log.trace("_onUploadPrefChange - Scheduling deletion ping after the initialization.");
> 
> In bug 1149754, we need to make at least TelemetryPing.send() work before
> init anyway (i.e. allowing to add pending pings before full TelemetryPing
> init).
> 
> Maybe we should just base this bug on that and drop juggling with the flag
> here?

Actually, what does fail right now if we try to send early? Did you check?

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> > In bug 1149754, we need to make at least TelemetryPing.send() work before
> > init anyway (i.e. allowing to add pending pings before full TelemetryPing
> > init).
> > 
> > Maybe we should just base this bug on that and drop juggling with the flag
> > here?
> 
> Actually, what does fail right now if we try to send early? Did you check?

That's a good point. Yes, I tried that, we might not have the client id before we complete the initialisation (unless it was cached).

Comment 13

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #12)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> > > In bug 1149754, we need to make at least TelemetryPing.send() work before
> > > init anyway (i.e. allowing to add pending pings before full TelemetryPing
> > > init).
> > > 
> > > Maybe we should just base this bug on that and drop juggling with the flag
> > > here?
> > 
> > Actually, what does fail right now if we try to send early? Did you check?
> 
> That's a good point. Yes, I tried that, we might not have the client id
> before we complete the initialisation (unless it was cached).

Ok, that's a general issue that we should solve independently of this bug (e.g. a ping could also be submitted early that wants the environment submitted too).
Maybe what we need is to keep track of all ping submissions before full init (with the payload, options, ...) and process them after init?

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8584509 [details] [diff] [review]
part 2 - Add test coverage for the delete pings.

Cancelling until next round.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Unassigning for now as it is blocked.

Comment 16

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Send DELETE PING when disabling FHR upload - v2

Comment 17

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage for the delete pings. - v2

This really just updates the patch.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8612335 [details] [diff] [review]
part 1 - Send DELETE PING when disabling FHR upload - v2

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

This is missing a documentation update, adding a deletion-ping.rst that documents what this ping is used for, when it is sent and the data it contains.

If you haven't already, please take this through some manual testing with different pref-value scenarios.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +143,5 @@
> + * @param {Object} aPing The ping to check.
> + * @return {Boolean} True if the ping is a deletion ping, false otherwise.
> + */
> +function isDeletionPing(aPing) {
> +  return !!aPing && isNewPingFormat(aPing) && (aPing.type == PING_TYPE_DELETION);

I don't think this makes sense to be called without an argument.
Lets do the check for the ping object being non-null at the caller.

@@ +998,5 @@
>      }
>  
> +    if (IS_UNIFIED_TELEMETRY) {
> +      // Watch the FHR upload setting to trigger deletion pings.
> +      Preferences.observe(PREF_FHR_UPLOAD_ENABLED, this._onUploadPrefChange, this);

It's odd to have a separate function for unregistering, but not registering.
Make it _attach/_detachObservers or *Listeners?

@@ +1171,5 @@
>      // With unified Telemetry, the FHR upload setting controls whether we can send pings.
>      // The Telemetry pref enables sending extended data sets instead.
>      if (IS_UNIFIED_TELEMETRY) {
> +      // Deletion pings are sent even if the upload is disabled.
> +      if (isDeletionPing(ping)) {

Only call this if we have a valid ping instance.

@@ +1225,5 @@
>  
>    /**
> +   * Remove the preference observer to avoid leaks.
> +   */
> +  _uninstall: function() {

_uninstall is very generic, _attach/_detachObservers?

Comment 19

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8612336 [details] [diff] [review]
part 2 - Add test coverage for the delete pings. - v2

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

::: toolkit/components/telemetry/tests/unit/test_TelemetryController.js
@@ +177,5 @@
> +add_task(function* test_deletionPing() {
> +  // Disable FHR upload: this should trigger a deletion ping.
> +  Preferences.set(PREF_FHR_UPLOAD_ENABLED, false);
> +
> +  let request = yield gRequestIterator.next();

You check isUnified below but not here?
This will probably blow up on try.

Comment 20

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage for the delete pings. - v3

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Send DELETE PING when disabling FHR upload - v3

Thanks Georg, I've addressed you comments. The documentation will follow up as a separate patch.

Comment 22

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Update the documentation

Comment 23

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8614098 [details] [diff] [review]
part 1 - Send DELETE PING when disabling FHR upload - v3

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

r=me with the nit addressed and proper manual testing to ensure this doesn't get sent unexpectedly.

::: toolkit/components/telemetry/TelemetryController.jsm
@@ +1152,5 @@
>  
>    /**
>     * Check if pings can be sent to the server. If FHR is not allowed to upload,
> +   * pings are not sent to the server (Telemetry is a sub-feature of FHR). If trying
> +   * to send a deletion ping, don't block it.

Nit: That should probably be written clearer.
"Pings of type 'deletion' are an exception, we should always be able to send them to enable clearing out profile data on the server." ?

Comment 24

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8614111 [details] [diff] [review]
part 3 -  Update the documentation

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

::: toolkit/components/telemetry/docs/deletion-ping.rst
@@ +1,5 @@
> +
> +"deletion" ping
> +===============
> +
> +This ping is generated if a user decides to turn off FHR/Telemetry reporting from the Preferences panel. The server will start the user data removal procedure upon reception.

"... when a user turns off FHR upload ..." & mention the associated pref?
+ "This requests that all associated data from that user be deleted." (-> let's not spec out server details here)

@@ +3,5 @@
> +===============
> +
> +This ping is generated if a user decides to turn off FHR/Telemetry reporting from the Preferences panel. The server will start the user data removal procedure upon reception.
> +
> +This ping contains the client id and no environment data.

A ping example would be nice, see e.g. https://reviewboard.mozilla.org/r/9683/diff/1/#5

::: toolkit/components/telemetry/docs/pings.rst
@@ +31,5 @@
>  * :doc:`main <main-ping>` - contains the information collected by Telemetry (Histograms, hang stacks, ...)
>  * :doc:`saved-session <main-ping>` - has the same format as a main ping, but it contains the *"classic"* Telemetry payload with measurements covering the whole browser session. This is only a separate type to make storage of saved-session easier server-side.
>  * ``activation`` - *planned* - sent right after installation or profile creation
>  * ``upgrade`` - *planned* - sent right after an upgrade
> +* :doc:`deletion <deletion-ping>` - sent when user disables FHR/Telemetry reporting

"sent when FHR upload is disabled, requesting deletion of the data associated with this user"

Comment 25

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Update the documentation - v2

Comment 26

[Alessio Placitelli [:Dexter]]

I've covered the following test cases, manually:

- Disabling "Share additional data" (extended telemetry recording) through the Preferences panel doesn't trigger a deletion ping
- Enabling "Share additional data" (extended telemetry recording) through the Preferences panel  doesn't trigger a deletion ping
- Disabling FHR Upload through the Preferences panel triggers a deletion ping
- Enabling FHR Upload through the Preferences panel doesn't triggers a deletion ping
- Disabling FHR Upload by setting "datareporting.healthreport.uploadEnabled" to false triggers a deletion ping
- Enabling FHR Upload by setting "datareporting.healthreport.uploadEnabled" to true doesn't trigger a deletion ping
- Starting Firefox with FHR upload disable doesn't trigger a new deletion ping

Please feel free to let me know if you can think of any other cases that I'm missing.

Comment 27

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage for the delete pings. - v4

I've updated the patch to fix the fallout from this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a604f72d825b

This changes the healthreport_testRemoteCOmnands.html to filter out pings which are not test pings (so that "deletion" pings don't make the test fail).

Comment 28

[Alessio Placitelli [:Dexter]]

Try push (along with bug 1173709): https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f427e390220

Comment 29

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Send DELETE PING when disabling FHR upload - v4

Rebased the patch.

Comment 30

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Add test coverage for the delete pings. - v5

Rebased.

Comment 31

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Update the documentation - v3

Rebased

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/fbf0fdd5face
https://hg.mozilla.org/integration/fx-team/rev/113c09b2fd55
https://hg.mozilla.org/integration/fx-team/rev/6142099b3260

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fbf0fdd5face
https://hg.mozilla.org/mozilla-central/rev/113c09b2fd55
https://hg.mozilla.org/mozilla-central/rev/6142099b3260

Comment 34

[Andrei Vaida [:avaida]]

This bug is verified fixed on Firefox 41.0b6 (20150831172306), using Windows 10 x64, Mac OS X 10.10.4 and Ubuntu 14.04 x64.

Confirmed that:
• there's no deletion ping triggered for disabling "Share additional data (i.e., Telemetry)" from about:preferences#advanced → Data Choices
• there's no deletion ping triggered for enabling "Share additional data (i.e., Telemetry)" from about:preferences#advanced → Data Choices
• a deletion ping is triggered for disabling "Enable Firefox Health Report" from about:preferences#advanced → Data Choices
• there's no deletion ping triggered for enabling "Enable Firefox Health Report" from about:preferences#advanced → Data Choices
• a deletion ping is triggered for setting "datareporting.healthreport.uploadEnabled" to false from about:config
• there's no deletion ping triggered for setting "datareporting.healthreport.uploadEnabled" to true from about:config
• there's no deletion ping triggered for starting the browser with "datareporting.healthreport.uploadEnabled" already set to false