Closed
Bug 666707
Opened 14 years ago
Closed 13 years ago
Add a telemetry probe to record whether we started following a successful shutdown
Categories
(Firefox :: Session Restore, defect)
Tracking
()
RESOLVED
FIXED
Firefox 7
People
(Reporter: taras.mozilla, Assigned: taras.mozilla)
References
Details
Attachments
(1 file)
1.38 KB,
patch
|
zpao
:
review+
|
Details | Diff | Splinter Review |
While it would be nice to reliably measure how long a shutdown took, it's quite hard to get right.
So instead we could measure whether session-restore data was from a checkpoint or whether firefox actually shutdown correctly. To do this I would make firefox write a dummy file right before exit(0)(or as close as we can get) and compare the timestamp of that to the sessionrestore timestamp.
Comment 1•14 years ago
|
||
(In reply to comment #0)
> While it would be nice to reliably measure how long a shutdown took, it's
> quite hard to get right.
> So instead we could measure whether session-restore data was from a
> checkpoint or whether firefox actually shutdown correctly. To do this I
> would make firefox write a dummy file right before exit(0)(or as close as we
> can get) and compare the timestamp of that to the sessionrestore timestamp.
With nsISessionStartup we have this - you'd want to get that early though (we reset that info post bug 588506). sessionType would be RESUME_SESSION or DEFER_SESSION following a proper shutdown (or at least a proper session write).
Does that work for you? Or do you need something more?
Assignee | ||
Comment 2•14 years ago
|
||
(In reply to comment #1)
> (In reply to comment #0)
> > While it would be nice to reliably measure how long a shutdown took, it's
> > quite hard to get right.
> > So instead we could measure whether session-restore data was from a
> > checkpoint or whether firefox actually shutdown correctly. To do this I
> > would make firefox write a dummy file right before exit(0)(or as close as we
> > can get) and compare the timestamp of that to the sessionrestore timestamp.
>
> With nsISessionStartup we have this - you'd want to get that early though
> (we reset that info post bug 588506). sessionType would be RESUME_SESSION or
> DEFER_SESSION following a proper shutdown (or at least a proper session
> write).
"proper session write" sounds like session restore can get written , the the browser could get killed and we'd mark that as a successful shutdown. That sounds wrong.
Comment 3•14 years ago
|
||
(In reply to comment #2)
> "proper session write" sounds like session restore can get written , the the
> browser could get killed and we'd mark that as a successful shutdown. That
> sounds wrong.
We write the session one last time in quit-application(-granted). At that point we save the state with "stopped". Every other time we save it, it's with state "running". So sure, it's possible that we don't quit successfully after that.
So outside of that and the fact that you can get the state (specifically session.lastUpdate) at startup (or look at the timestamp before we write to it), I'm not clear what you want from session restore here.
Assignee | ||
Comment 4•14 years ago
|
||
Measuring shutdown via session-restore isn't great, but it's a start.
Shortcoming here are being-killed-by-OS-shutdown-logic(which I'm not sure if we handle), shutdown freezing after session restore was written, etc. We can address these in more complicated followups.
Assignee: nobody → tglek
Attachment #541838 -
Flags: review?(paul)
Updated•14 years ago
|
Attachment #541838 -
Flags: review?(paul) → review+
Comment 5•14 years ago
|
||
Comment on attachment 541838 [details] [diff] [review]
record shutdown success
Bah, hit submit just a bit early…
Killed by OS shutdown is special. I don't think we handle that super well as a product. There are a few open bugs in sessionstore where that's treated as a crashed case. OS X seems to be OK, but Windows/Linux don't go so well. Anecdotal evidence is saying that the majority of people will quit before shutting down but it's still an annoying case.
Nit: Please add a comment here - perhaps just something about this not being a perfect measure of did crash, but it'll work for now.
>
>+ let Telemetry = Cc["@mozilla.org/base/telemetry;1"].getService(Ci.nsITelemetry);
>+ Telemetry.getHistogramById("SHUTDOWN_OK").add(!lastSessionCrashed);
Nit: add a blank line here.
> // set the startup type
Assignee | ||
Comment 7•13 years ago
|
||
Whiteboard: [inbound]
Target Milestone: --- → Firefox 7
Comment 8•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•13 years ago
|
Whiteboard: [inbound]
You need to log in
before you can comment on or make changes to this bug.
Description
•