792094 - TelemetryPing unnecessarily defers opening ping files when writing them.

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Nathan Froyd [:froydnj]]

+++ This bug was initially created as a clone of Bug #791765 +++

From bug 791765 comment 7:

Here's another twist to the story - on forced shutdown, I see the following:

shuting down due to close gesture.
[JavaScript Error: "[Exception... "Component returned failure code: 0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS) [nsIFileOutputStream.write]"  nsresult: "0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS)"  location: "JS frame :: file:///T:/Mozilla/ELM-REL/dist/bin/components/TelemetryPing.js :: savePingToFile :: line 845"  data: no]" {file: "file:///T:/Mozilla/ELM-REL/dist/bin/components/TelemetryPing.js" line: 845}]
[71A5390] MetroWidget::Destroy mWnd=2505C2 type=0
MetroAppShell::Exit
mozilla::widget::winrt::MetroApp::ShutdownXPCOM: IsMainThread:1 ThreadId:7F8

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: patch

Comment 2

[(dormant account)]

Comment on attachment 662223 [details] [diff] [review]
patch

This seems like a good fix, but that .write codepath still needs an error check on .write().

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: ignore errors from .write

As requested.

Comment 4

[(dormant account)]

Comment on attachment 663024 [details] [diff] [review]
ignore errors from .write

thanks

Comment 5

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d43f472e46
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c2ecef45b9

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a6d43f472e46
https://hg.mozilla.org/mozilla-central/rev/f6c2ecef45b9

Comment 7

[Nathan Froyd [:froydnj]]

Attachment: aurora rolled-up patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): telemetry changes
User impact if declined: Saved ping files may accumulate unnecessarily on disk due to errors while reading them.
Testing completed (on m-c, etc.): On m-c for ~2 weeks 
Risk to taking this patch (and alternatives if risky): Low risk, telemetry has continued to work on nightly. 
String or UUID changes made by this patch: None.

Comment 8

[Alex Keybl [:akeybl]]

(In reply to Nathan Froyd (:froydnj) from comment #7)
> Created attachment 668473 [details] [diff] [review]
> aurora rolled-up patch
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): telemetry changes
> User impact if declined: Saved ping files may accumulate unnecessarily on
> disk due to errors while reading them.
> Testing completed (on m-c, etc.): On m-c for ~2 weeks 
> Risk to taking this patch (and alternatives if risky): Low risk, telemetry
> has continued to work on nightly. 
> String or UUID changes made by this patch: None.

Does this have privacy implications like bug 791765? Or other user impact?

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Alex Keybl [:akeybl] from comment #8)
> > [Approval Request Comment]
> > User impact if declined: Saved ping files may accumulate unnecessarily on
> > disk due to errors while reading them.
> 
> Does this have privacy implications like bug 791765? Or other user impact?

Doh, I'm sorry; I was nominating too many things and got my wires crossed.

This patch does not have privacy implications or other user impact.  It does mean that we may not be able to persist telemetry data to send during the next session.  So it impacts our ability to collect data, but the user should not notice any difference.

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 668473 [details] [diff] [review]
aurora rolled-up patch

[Triage Comment]
Thanks for the context. Approving this low risk fix in support of Telemetry data collection. Please land early Monday to make it in before the next merge.

Comment 11

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/65680665ffb4

Comment 12

[u279076]

Is there STR QA can use to verify this fixed? Alternatively, can you confirm this is fixed Nathan?

Comment 13

[Nathan Froyd [:froydnj]]

I can confirm this is fixed.

Comment 14

[u279076]

(In reply to Nathan Froyd (:froydnj) from comment #13)
> I can confirm this is fixed.

Nathan, what version of Firefox did you use to test? Ideally, I'd like this to be verified with Firefox 17.0 Beta. Thanks.