TelemetryPing unnecessarily defers opening ping files when writing them.

RESOLVED FIXED in Firefox 17

Status

()

Toolkit
Telemetry
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

Trunk
mozilla18
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox17 fixed)

Details

(Whiteboard: [qa-])

Attachments

(3 attachments)

(Assignee)

Description

5 years ago
+++ 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
(Assignee)

Comment 1

5 years ago
Created attachment 662223 [details] [diff] [review]
patch
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Attachment #662223 - Flags: review?(taras.mozilla)

Comment 2

5 years ago
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+
(Assignee)

Comment 3

5 years ago
Created attachment 663024 [details] [diff] [review]
ignore errors from .write

As requested.
Attachment #663024 - Flags: review?(taras.mozilla)

Comment 4

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

thanks
Attachment #663024 - Flags: review?(taras.mozilla) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d43f472e46
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6c2ecef45b9
https://hg.mozilla.org/mozilla-central/rev/a6d43f472e46
https://hg.mozilla.org/mozilla-central/rev/f6c2ecef45b9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Assignee)

Comment 7

5 years ago
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.
Attachment #668473 - Flags: review+
Attachment #668473 - Flags: approval-mozilla-aurora?

Comment 8

5 years ago
(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?
(Assignee)

Comment 9

5 years ago
(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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/65680665ffb4
status-firefox17: --- → fixed
Is there STR QA can use to verify this fixed? Alternatively, can you confirm this is fixed Nathan?
Whiteboard: [qa?]
(Assignee)

Comment 13

5 years ago
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.