Last Comment Bug 792094 - TelemetryPing unnecessarily defers opening ping files when writing them.
: TelemetryPing unnecessarily defers opening ping files when writing them.
Status: RESOLVED FIXED
[qa-]
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla18
Assigned To: Nathan Froyd [:froydnj]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-18 09:53 PDT by Nathan Froyd [:froydnj]
Modified: 2012-10-18 11:21 PDT (History)
5 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch (1.01 KB, patch)
2012-09-18 10:48 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Review
ignore errors from .write (1.36 KB, patch)
2012-09-20 08:27 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review+
Details | Diff | Review
aurora rolled-up patch (1.15 KB, patch)
2012-10-05 09:09 PDT, Nathan Froyd [:froydnj]
nfroyd: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Nathan Froyd [:froydnj] 2012-09-18 09:53:28 PDT
+++ 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] 2012-09-18 10:48:41 PDT
Created attachment 662223 [details] [diff] [review]
patch
Comment 2 (dormant account) 2012-09-19 02:08:35 PDT
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] 2012-09-20 08:27:20 PDT
Created attachment 663024 [details] [diff] [review]
ignore errors from .write

As requested.
Comment 4 (dormant account) 2012-09-21 04:45:13 PDT
Comment on attachment 663024 [details] [diff] [review]
ignore errors from .write

thanks
Comment 7 Nathan Froyd [:froydnj] 2012-10-05 09:09:05 PDT
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.
Comment 8 Alex Keybl [:akeybl] 2012-10-05 15:37:58 PDT
(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] 2012-10-05 16:07:17 PDT
(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] 2012-10-07 14:24:13 PDT
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 12 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-17 15:53:20 PDT
Is there STR QA can use to verify this fixed? Alternatively, can you confirm this is fixed Nathan?
Comment 13 Nathan Froyd [:froydnj] 2012-10-18 09:11:56 PDT
I can confirm this is fixed.
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-10-18 11:21:14 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.