Last Comment Bug 666707 - Add a telemetry probe to record whether we started following a successful shutdown
: Add a telemetry probe to record whether we started following a successful shu...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: Firefox 7
Assigned To: (dormant account)
:
Mentors:
Depends on: 666309
Blocks: 659396
  Show dependency treegraph
 
Reported: 2011-06-23 12:18 PDT by (dormant account)
Modified: 2011-06-30 14:48 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
record shutdown success (1.38 KB, patch)
2011-06-24 15:39 PDT, (dormant account)
paul: review+
Details | Diff | Review

Description (dormant account) 2011-06-23 12:18:11 PDT
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 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-23 14:57:45 PDT
(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?
Comment 2 (dormant account) 2011-06-24 11:17:50 PDT
(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 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-24 11:49:51 PDT
(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.
Comment 4 (dormant account) 2011-06-24 15:39:16 PDT
Created attachment 541838 [details] [diff] [review]
record shutdown success

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.
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-24 15:49:40 PDT
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
Comment 6 mmc 2011-06-28 06:27:08 PDT
Note also the typo "succeful".
Comment 8 Marco Bonardo [::mak] 2011-06-29 02:23:41 PDT
http://hg.mozilla.org/mozilla-central/rev/41f776cc42d5

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