1348250 - Change TelemetryStorage to load crash pending pings outside of the profile

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

Bug 1343504 will save pending crash pings outside of the profile directory, to prevent sending duplicate pings.

This bug is about changing TelemetryStorage.jsm so that we can load them and send them through Telemetry if the pingsender failed to send/remove them.

Comment 1

[Alessio Placitelli [:Dexter]]

Note to self:

- Pending crash pings live in the User App Data (e.g. %AppData%/Mozilla/Firefox/Pending Pings -> Services.dirsvc.get("UAppData", Ci.nsIFile).path)
- We need to check for them when Firefox starts and migrate them over the Firefox pending pings directory from TelemetryStorage.

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: Bug 1348250 - Change TelemetryStorage to load crash pending pings outside of the profile.

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

Comment 3

[Alessio Placitelli [:Dexter]]

Comment on attachment 8853403 [details]
Bug 1348250 - Change TelemetryStorage to load crash pending pings outside of the profile.

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

Comment 4

[Chris H-C :chutten]

Comment on attachment 8853403 [details]
Bug 1348250 - Change TelemetryStorage to load crash pending pings outside of the profile.

https://reviewboard.mozilla.org/r/125512/#review128076

So, in a multi-profile case (rare as they are) crash pings assembled in one profile will be sent by another. That's kinda neat.

::: toolkit/components/telemetry/tests/unit/test_MigratePendingPings.js:39
(Diff revision 2)
> +  yield createFakeAppDir();
> +  // Make sure we don't generate unexpected pings due to pref changes.
> +  yield setEmptyPrefWatchlist();
> +});
> +
> +add_task(function* test_migrateUsentPings() {

"Usent"?

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8853403 [details]
Bug 1348250 - Change TelemetryStorage to load crash pending pings outside of the profile.

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9f9515d8bca
Change TelemetryStorage to load crash pending pings outside of the profile. r=chutten

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 8

[Alessio Placitelli [:Dexter]]

Abe, as we discussed, this bug handles the pending crash pings saved by 1343504 outside of the user profile when the pingsender fails. 

With this patch, the crash pings are removed from the "Pending Pings" directory and dispatched through the normal Telemetry workflow. This means that, after Firefox and Telemetry is initialized (60 seconds from the startup), the pings should be sent.

Would you please validate it? :-)

Comment 9

[Abe - QA (:Abe_LV)]

With the exception of Bug 1353192, all are green here. 

Here is the link to test cases and runs: https://public.etherpad-mozilla.org/p/bug1341349

Comment 10

[Abe - QA (:Abe_LV)]

QA verified as fixed. Please see comment 9 for test cases.

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8853403 [details]
Bug 1348250 - Change TelemetryStorage to load crash pending pings outside of the profile.

Approval Request Comment
[Feature/Bug causing the regression]: 1310703
[User impact if declined]: If we fail to send the "crash" ping using the pingsender, this patch will migrate them to the user profile and try to send them again. If we don't uplift this, we won't be resending the pings.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes, see comment 10
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: This patch should be applied after bug 1345153. After we can apply 1345108.
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: This is making Telemetry migrate pings saved outside of the profile directory (where the crashreporter data is) to the user profile. If we fail the migration, we just won't be sending these crash pings. Nothing else should break.
[String changes made/needed]: None

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8853403 [details]
Bug 1348250 - Change TelemetryStorage to load crash pending pings outside of the profile.

Fix crash ping sending issue and was verified by QA. Aurora54+.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Holding off on this uplift until we get a rebased patch for bug 1345153.

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: [AURORA-REBASED] Make telemetry migrate pings outside of the profile

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/119d04506d97