Closed Bug 792094 Opened 7 years ago Closed 7 years ago

TelemetryPing unnecessarily defers opening ping files when writing them.

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox17 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

(Whiteboard: [qa-])

Attachments

(3 files)

+++ 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
Attached patch patchSplinter Review
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Attachment #662223 - Flags: review?(taras.mozilla)
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().
Attachment #662223 - Flags: review?(taras.mozilla) → review+
As requested.
Attachment #663024 - Flags: review?(taras.mozilla)
Comment on attachment 663024 [details] [diff] [review]
ignore errors from .write

thanks
Attachment #663024 - Flags: review?(taras.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/a6d43f472e46
https://hg.mozilla.org/mozilla-central/rev/f6c2ecef45b9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
[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.
Attachment #668473 - Flags: review+
Attachment #668473 - Flags: approval-mozilla-aurora?
(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?
(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 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.
Attachment #668473 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Is there STR QA can use to verify this fixed? Alternatively, can you confirm this is fixed Nathan?
Whiteboard: [qa?]
I can confirm this is fixed.
Whiteboard: [qa?] → [qa-]
(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.
You need to log in before you can comment on or make changes to this bug.