1399433 - Verify that the pingsender is not used when data upload is disabled

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

From bug 1336360 comment 66, the following reddit comment was referenced [1].

> I'm on the regular release channel and pingsender wants to phone home evey now and then.
> This doesn't look good. I don't even have any data choices enabled (no helath report, no telemetry, etc.).

The user also reports to "probably" have one instance of Firefox with FHR/telemetry enabled:

> They are all false. I may have one FF profile which has them on, there's ~20% chance that it was running at that time.

We should investigate this to rule out any problem.

[1] - https://www.reddit.com/r/firefox/comments/68c59k/nightly_pingsenderexe/dms26zx/

Comment 1

[Georg Fritzsche [:gfritzsche]]

If we test this, we probably need to test the separate invocation paths:
- content crashes
- parent crashes
- main ping on shutdown

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #1)
> If we test this, we probably need to test the separate invocation paths:
> - content crashes
> - parent crashes
> - main ping on shutdown

Let me audit the "main ping on shutdown" path. Here's what's going on:

1) The shutdown ping is submitted to Telemetry at [1], where the API is also instructed to use the pingsender (see the |options|).
2) The flow continues in |submitExternalPing| [2] which forwards the call to the internal |_submitPingLogic|. Nothing too surprising is happening here.
3) Within |_submitPingLogic| the rest of the ping is assembled and the ping data us submitted using |TelemetrySend.submitPing| [3]. Please note that the |usePingSender| option is passed in as well.
4) In |submitPing| every single ping is checked and cleared for sending [4]. We check for the prefs to be correctly set, for the policy to be respected and for the option to use the pingsender.
5) The prefs seem to be correctly checked at [5], where we make sure that "datareporting.healthreport.uploadEnabled" is respected.

Nothing weird seems to be happening on this code path.

Gabriele, would you kindly make sure nothing weird is happening on the crashes paths as well?

[1] - http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/toolkit/components/telemetry/TelemetrySession.jsm#1701,1717
[2] - http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/toolkit/components/telemetry/TelemetryController.jsm#503
[3] - http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/toolkit/components/telemetry/TelemetryController.jsm#458
[4] - http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/toolkit/components/telemetry/TelemetrySend.jsm#846,860,869
[5] - http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/toolkit/components/telemetry/TelemetrySend.jsm#1232,1244,1248

Comment 3

[Alessio Placitelli [:Dexter]]

I'll verify the crash pings paths!

Comment 4

[Alessio Placitelli [:Dexter]]

For the "crash" pings, we have the two cases mentioned in comment 1. Let's focus on the "parent crash" first.

When the parent process crashes:

1) If the minidump analyzer can be run, we generate a crash ping [1].
2) We look for a "TelmetryServerURL" annotation in the crash data [2]. If the annotation is not there or is empty, we bail out.
3) The "TelemetryServerURL" annotation is controlled by |TelemetrySend._annotateCrashReport| [3] which clears or sets it according to the values of the user preferences.
4) |_annotateCrashReport| is called as soon as Firefox starts, then on the delayed setup and again every time the preferences are changed [4].
5) We also have test coverage [5] to make sure that the crash is correctly annotated when the preferences as set to false.

[1] - http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/toolkit/crashreporter/client/crashreporter.cpp#738
[2] - http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/toolkit/crashreporter/client/ping.cpp#316,332
[3] - http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/toolkit/components/telemetry/TelemetrySend.jsm#700,702-703
[4] - http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/toolkit/components/telemetry/TelemetrySend.jsm#642,668,803,828
[5] - http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/toolkit/crashreporter/test/unit/test_crashreporter_crash.js#90-130

Comment 5

[Alessio Placitelli [:Dexter]]

As far as I can tell, we're not specifically handling "crash" pings coming from content/gpu children processes with the pingsender: when a crash in the content happens, it gets handled by CrashManager.jsm [1] which relies on TelemetryController to submit the crash ping. We're confident in the abilities of that function to respect the user preferences.

Gabriele, are this and the previous comment reasonable? Did I overlook anything?

[1] - http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/toolkit/components/crashes/CrashManager.jsm#475,663

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

(In reply to Alessio Placitelli [:Dexter] from comment #5)
> Gabriele, are this and the previous comment reasonable? Did I overlook
> anything?

This is fine, there's nothing missing and we have integration tests covering both paths to ensure we don't regress and send the ping when we shouldn't be sending it.

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> (In reply to Alessio Placitelli [:Dexter] from comment #5)
> > Gabriele, are this and the previous comment reasonable? Did I overlook
> > anything?
> 
> This is fine, there's nothing missing and we have integration tests covering
> both paths to ensure we don't regress and send the ping when we shouldn't be
> sending it.

Thanks Gabriele! Closing this as FIXED since this bug was about auditing the code.