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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17

People

(Reporter: froydnj, Assigned: espindola)

Details

Attachments

(1 file)

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.
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...
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
(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...
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?
(I tested that it works on linux, I have to setup my windows VM again to test it on windows).
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+
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.

Attachment

General

Created:
Updated:
Size: