(dormant account) Assignee
	Description • 13 years ago

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.

	Paul O'Shannessy [:zpao] (not bugmail, email directly)
	Comment 1 • 13 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?

	(dormant account) Assignee
	Comment 2 • 13 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.

	Paul O'Shannessy [:zpao] (not bugmail, email directly)
	Comment 3 • 13 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.

	(dormant account) Assignee
	Comment 4 • 13 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.

	Paul O'Shannessy [:zpao] (not bugmail, email directly)
	Comment 5 • 13 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

	mmc
	Comment 6 • 13 years ago

Note also the typo "succeful".

	(dormant account) Assignee
	Comment 7 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/41f776cc42d5

	Marco Bonardo [:mak]
	Comment 8 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/41f776cc42d5