Closed
Bug 777913
Opened 12 years ago
Closed 12 years ago
shutdown timings for multiple sessions are not recorded correctly
Categories
(Toolkit :: Startup and Profile System, defect)
Toolkit
Startup and Profile System
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: froydnj, Assigned: espindola)
Details
Attachments
(1 file)
406 bytes,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
We have this: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/startup/nsAppStartup.cpp#336 except that PR_Rename is deeply concerned about possible dataloss issues: http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/unix/unix.c#198 We should fix this or we are going to have very...consistent shutdown times.
Assignee | ||
Comment 1•12 years ago
|
||
Should we just delete the file once telemetry reads it? We should probably do that anyway to avoid reporting the same time twice in the case of a crash...
Assignee | ||
Comment 2•12 years ago
|
||
Thinking a bit more about: We should probably delete the file once telemetry sends that data, but we should also make sure this rename works so that file always has the time of the *last* shutdown. I will take a look.
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Reporter | ||
Comment 3•12 years ago
|
||
(In reply to Rafael Ávila de Espíndola (:espindola) from comment #1) > Should we just delete the file once telemetry reads it? We should probably > do that anyway to avoid reporting the same time twice in the case of a > crash... We could probably just delete it in nsAppStartup.cpp after reading from it, when we cache the value. I think we still have to handle overwriting it, though: 0. User enables telemetry. 1. User shuts down, file gets written. 2. User starts up. 3. User shuts down prior to sending telemetry, file still exists. Though at step 3, telemetry might have already gotten the value for saving the not-yet-sent ping. I don't know that I'd want to depend on that ordering, though...
Assignee | ||
Comment 4•12 years ago
|
||
Yes, my first though was "telemetry will get *a* value", but it is cleaner if it always gets the last one and the previous run is dropped. I will try just deleting the file before the rename. That should work on posix. Do you know if there are other restrictions on windows that PR_Rename is not emulating?
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #646366 -
Flags: review?(nfroyd)
Assignee | ||
Comment 6•12 years ago
|
||
(I tested that it works on linux, I have to setup my windows VM again to test it on windows).
Reporter | ||
Comment 7•12 years ago
|
||
Comment on attachment 646366 [details] [diff] [review] delete it first I'm willing to say this is correct without testing on a Windows machine. I will build on Windows today and check, but I'd say go ahead and commit. (The same idiom is used elsewhere in the tree, FWIW.)
Attachment #646366 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 8•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/436fce553cd0
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/436fce553cd0
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in
before you can comment on or make changes to this bug.
Description
•