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

RESOLVED FIXED

Status

()

Toolkit
Telemetry
P1
normal
RESOLVED FIXED
2 months ago
2 months ago

People

(Reporter: Dexter, Assigned: Dexter)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox57 affected)

Details

(Assignee)

Description

2 months ago
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/
(Assignee)

Updated

2 months ago
Blocks: 1336360
Priority: -- → P1
If we test this, we probably need to test the separate invocation paths:
- content crashes
- parent crashes
- main ping on shutdown
(Assignee)

Comment 2

2 months ago
(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
Flags: needinfo?(gsvelto)
(Assignee)

Updated

2 months ago
Assignee: nobody → alessio.placitelli
(Assignee)

Comment 3

2 months ago
I'll verify the crash pings paths!
Flags: needinfo?(gsvelto)
(Assignee)

Comment 4

2 months ago
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
(Assignee)

Comment 5

2 months ago
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
Flags: needinfo?(gsvelto)
(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.
Flags: needinfo?(gsvelto)
(Assignee)

Comment 7

2 months ago
(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.
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.