Georg Fritzsche [:gfritzsche] Reporter
	Description • 9 years ago

After creating the "shutdown" ping, we wait for (potentially long) saving operations to finish before collecting the "saved-session" payload.
 
https://hg.mozilla.org/mozilla-central/annotate/2ddec2dedced/toolkit/components/telemetry/TelemetrySession.jsm#l1617

We should not wait on this before collecting the saved-session payload, instead kicking off both collections synchronously and afterwards waiting on both to be saved.

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 2 • 9 years ago

Comment on attachment 8637911 [details] [diff] [review]
bug1186871.patch

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

r=me with the below fixed.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1613,5 @@
>  
>     /**
>      * Save both the "saved-session" and the "shutdown" pings to disk.
>      */
>    saveShutdownPings: Task.async(function*() {

This doesn't need to be a Task now.
Just make it a plain function and |return Promise.all(...)|.

	Alessio Placitelli [:Dexter] Assignee
	Comment 3 • 9 years ago

Thanks! Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f157d696aa5f

	Pulsebot
	Comment 4 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/a286f65dd231

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 5 • 9 years ago

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

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 6 • 9 years ago

Comment on attachment 8637953 [details] [diff] [review]
bug1186871.patch

Approval Request Comment
[Feature/regressing bug #]:
This is part of the uplift3 batch for the unified Telemetry project: http://bit.ly/1TCl4r8
This is targetting 41 now and these are the last minor "feature" patches blocking shipping - any further patches will be fixes for data quality etc. with very limited scope & impact.
[User impact if declined]:
This affects data quality, potentially increasing data mismatch between "old" and "new" Telemetry data, which could potentially break data validation.
[Describe test coverage new/current, TreeHerder]:
This has good automated test-coverage.
[Risks and why]:
Low-risk, well understood change that just changes when ping data is generated.
[String/UUID change made/needed]:
No string changes.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 7 • 9 years ago

Tracked for 41. Also requesting QE team to verify this fix along all Unified Telemetry fixes. Thanks!

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 8 • 9 years ago

Comment on attachment 8637953 [details] [diff] [review]
bug1186871.patch

Let's uplift to Aurora, has been in m-c for a few days now.

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 9 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/790d04f41b26

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 9 years ago

This appeared to have caused an xpcshell failure on at least Windows: https://treeherder.mozilla.org/logviewer.html#?job_id=996634&repo=mozilla-aurora


Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/047d1e5ffaa3

	Alessio Placitelli [:Dexter] Assignee
	Comment 11 • 9 years ago

This updated patch fixes the problem for aurora.

	Alessio Placitelli [:Dexter] Assignee
	Comment 12 • 9 years ago

Pushed to aurora. Treeherder:  https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=a6899db62129

 https://hg.mozilla.org/releases/mozilla-aurora/rev/a6899db62129

	Georg Fritzsche [:gfritzsche] Reporter
	Comment 13 • 9 years ago

I don't think this bug needs separate QA attention.