358689 - Window state should be restored after a crash

Status: RESOLVED FIXED

(Camino Graveyard :: General, enhancement)

Description

[Ben Willmore]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a1) Gecko/20061027 Camino/1.2+
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a1) Gecko/20061027 Camino/1.2+

Once bug 152147 has been implemented, you should add crash protection, i.e. automatic restoration of windows/tabs after a crash.  You should also provide a GUI that allows the user to choose which windows/tabs to restore.

The attached code is the source of a plugin, CaminoSession, which does all this.  Currently, it uses its own session-saving system, but the session-saving logic and GUI could be stripped out fairly easily, and used with the new Camino session-saving code.


Reproducible: Always

Comment 1

[Ben Willmore]

Attachment: caminosession.h

Comment 2

[Ben Willmore]

Attachment: caminosession.m

Comment 3

[Ben Willmore]

Attachment: crashdialogcontroller.h

Comment 4

[Ben Willmore]

Attachment: crashdialogcontroller.m

Comment 5

[Ben Willmore]

Attachment: crashdialog.nib

Comment 6

[Ben Willmore]

Attachment: csbrowsersession.h

Comment 7

[Ben Willmore]

Attachment: csbrowsersession.m

Comment 8

[Samuel Sidler (old account; do not CC)]

Hey Ben,

This code is a great start (as I'm sure various CaminoSession users will attest to), but it'd be beneficial if you took this code and incorporated it into a patch for this specific feature (I believe CaminoSession does things other than window-state restore post-crash).

When doing so, be sure to base your patch on the code that bug 152147 is adding.

(Also, we typically just add one attachment as the actual patch and one for nibs [usually zipped].)

Comment 9

[Ben Willmore]

> When doing so, be sure to base your patch on the code that bug 152147 is
> adding.

Yup, I can do this once the other code is solidified.  My motivation for posting this here was to try and make sure the whole thing isn't needlessly reimplemented from scratch.

If you really intend to use (a variant of) the attached code, it would be worth having a look at it before finalizing the implementation for bug 152147.

There are a couple of things that come to mind immediately that smorgan's patches don't do:
* allow for parallel saved sessions (you need one for 'normal' session saves, and one for crash protection)
* have unique IDs for each tab (you need them so the user can select which ones to disable)

Comment 10

[Stuart Morgan]

Someone needs to check if the licensing terms on these files are such that we can actually make use of them as posted.

Comment 11

[Ben Willmore]

I have no interest in or understanding of licensing issues.  I'm happy to relicense the code any way you like.

Comment 12

[Chris Lawson (gone)]

Confirming, since this was something we said we wanted to do as a follow-up to session saving.

cl

Comment 13

[Stuart Morgan]

(In reply to comment #9)
> There are a couple of things that come to mind immediately that smorgan's
> patches don't do:
> * allow for parallel saved sessions (you need one for 'normal' session saves,
> and one for crash protection)

There's a UI decision which hasn't been made yet there; I hadn't envisioned any manual save/restore mechanism. If we do decide to go that route, though, I think we'd want it in another file, which would be easy to build onto that implementation later.

> * have unique IDs for each tab (you need them so the user can select which ones
> to disable)

The tuple {window number, tab number} should uniquely identify any tab.

If you disagree on either point (or notice other issues), definitely bring it up in bug 152147, so we can make sure we have the right foundation for moving forward.

Comment 14

[Chris Lawson (gone)]

Per the dependency, we'll need to do something here before 1.1b1 if this is going to make 1.1.

Comment 15

[Stuart Morgan]

Split out bug 363600 for the remaining non-UI work.

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Fx restore session dialogue

This is what Fx shows on restart after a crash.

Comment 17

[Sean Murphy]

I'll work on this, so we can get it in 1.1.

Comment 18

[Sean Murphy]

Attachment: patch

Here's my patch which will ask the user for confirmation before a session is restored following a crash.  Even though we're the browser that "gets out of your way," I think presenting a dialog to the user here is possibly the only appropriate way to handle session restoration post-crash.  This issue has been debated as it pertains to Fx in bug 337390 and bug 344852.

To determine if Camino crashed during the last run I created a new class which encapsulates the actual logic to check this.  This allows for future changes or enhancements from my somewhat unsophisticated technique (such as checking ~/Library/Logs/CrashReporter/ for new entries).  This way, we can start to use crash determination throughout the application, and only have to change one area of the code if we think of a better way to determine the answer.  I do realize that TalkBack may be relevant in concluding if the last run crashed, and I might be duplicating behavior here.  I was under the impression that TalkBack only kicked in right as the session crashes.  Does anyone know if it provides a way to detect a crash from the previous session?

Assuming my CrashDetector class stays, I currently use a technique similar to the way in which Fx's session restore determines a previously crashed session.  A preference is stored (using the standard OS X defaults system) during normal terminating indicating a safe shutdown, and is reset upon each launch.  If this preference indicates otherwise, a failure must have occurred.

Aside from ascertaining a previous crash, the SessionManager just had to be informed if the window state should be restored.  If the user chooses not to restore, any saved state is cleared.

Currently, the dialog is application-modal and is presented on first launch, before any windows are opened.  The dialog is identical to Fx's, as there was no compelling reason to alter this behavior, especially since those devs debated heavily and continually refined the wording of this message.  QA (or anyone) should feel free to give an opinion on this dialog though.

New Localizable.strings for the dialog (currently identical to Fx, so they're up for debate):

/* Restoring saved session after crash dialog */
"RestoreSessionAfterCrashTitle" = "Restore previous session?";
"RestoreSessionAfterCrashMessage" = "Your last Camino session closed unexpectedly. You can restore the tabs and windows from your previous session, or start a new session if you think the problem was related to a page you were viewing.";
"RestoreSessionAfterCrashActionButton" = "Restore Session";
"RestoreSessionAfterCrashCancelButton" = "Don't Restore";

Notes: CrashDetector is declared in new header/imp files, since I didn't want to arbitrarily cram it into one of the already-lengthy controllers.  The RestoreSessionAfterCrashMessage could be modified to use a token for the application name, but adding complexity there probably is harder than just changing the string manually.  Resolving bug 365746 is very advantageous towards this one (especially if a modal dialog is used).

Comment 19

[Stuart Morgan]

Comment on attachment 250354 [details] [diff] [review]
patch

>+// Convenience method to restore the saved session behind a previously opened window.
>+- (void)restoreWindowStateBehindExistingWindow

Since there's only one place this is really called, I'd rather see the problem of having duplicate branches in the logic solved rather than create a convenience method.

>   if ([prefManager getBooleanPref:"camino.remember_window_state" withSuccess:NULL]) {

This shouldn't enclose everything it does here; we want to offer to restore after a crash regardless of the pref value.

With those two changes, I'd suggest pseudocode along the lines of:

BOOL restore
if (crashed last time)
  restore = ask if we should restore
else
  restore = pref value

if (restore)
  do the restore
else
  cleareSavedState


>+// CrashDetector
>+//
>+// A shared object which can be queried to determine if Camino crashed
>+// the last time it was run.

Creating a new global singleton object for something we really only need in one place seems pretty heavy. I'd rather just have a few extra lines in the startup and shutdown code in MainController.

Comment 20

[Stuart Morgan]

(In reply to comment #18)
> This allows for future changes or enhancements from my somewhat
> unsophisticated technique (such as checking ~/Library/Logs/CrashReporter/
> for new entries).

And as an aside, that would actually be a very fragile way to determine unexpected shutdown; it doesn't cover force quit, power failure, kernel panics, etc, and would rely on OS internals (just because those files are there and in a certain format now doesn't mean they will continue to be). More complicated != better ;)

Comment 21

[Sean Murphy]

Attachment: patch v2

(In reply to comment #19)

Thanks Stuart.  I incorporated your suggestions into a new patch.

Gone is the CrashDetector class.  I added a -(BOOL)previousSessionCrashed method to MainController.mm which encapsulates the crash detection process well enough.  Two lines of code used for crash determination are required in -applicationWillTerminate: however.  I write a comment in the patch documenting that fact, since future changes to the crash detection technique would also warrant modifying or removing those two lines.

I used a static variable in -previousSessionCrashed.  The 'terminated normally' default is checked and immediately reset so any crash from that point on will be indicated correctly.  Without remembering that default's original value in some way, this method would only return the correct answer once and any subsequent call would not work properly.  I felt that an instance variable was not appropriate for this situation, but I can easily change that if deemed more suitable.  I also avoided just allowing this method to exhibit one-time-use only behavior. Even with documentation indicating so, it makes the code pretty fragile.

I changed the keys for the localized strings slightly.  Possible values to have in Localizable.strings when testing this:

/* Restoring saved session after crash dialog */
"RestoreAfterCrashTitle" = "Restore previous session?";
"RestoreAfterCrashMessage" = "Your last Camino session closed
unexpectedly. You can restore the tabs and windows from your previous session,
or start a new session if you think the problem was related to a page you were
viewing.";
"RestoreAfterCrashActionButton" = "Restore Session";
"RestoreAfterCrashCancelButton" = "Don't Restore";

(In reply to comment #20)

> More complicated != better ;)

Yeah, you're right.  Using mach exception ports was another possible technique, but again wouldn't work for power-outages and other similar situations in which a signal is not sent.  Plus, that's quite a bit more complicated, and this does work.

I included a fix for bug 36574 inside this patch as well.  I accidentally left that code in place before the diff, but kept it in anyway since it's beneficial to this bug.  If the issue should be handled separately, I'll remove the additional fix and do so.

Comment 22

[Sean Murphy]

The bug id I referenced above should be: bug 365746.

Comment 23

[Sean Murphy]

Attachment: patch v2 update

I slightly updated this patch to fix the issue in bug 364497.  If Camino was launched opening a URL and  the window state was restored (both normally and after a crash) the new URL would not obey the "links from other applications" (browser.reuse_window) preference.  This update addresses that problem.  I submitted a patch here since the entire restoration code had to be moved around to fix it.

Basically, the fix consists of restoring the window state inside of the NSApplication delegate method applicationWillFinishLaunching: before the remaining initialization in applicationDidFinishLaunching.  The prior method is called before any apple events are performed.  This means that Camino will ultimately perform the openNewWindowOrTabWithURL... method after the window state has been restored.  The usual application:openFile: is not used in this case (launching with a URL to open).

If links are set to open in tabs, the URL will load as the rightmost tab in the frontmost restored window.

It's worth noting that we have to be careful here and make sure doing this doesn't negatively impact the launch time perception by the user, which is an important optimization.  The dock icon will still bounce for the duration of applicationWillFinishLaunching: and will have stopped when applicationWillFinishLaunching is called.  That's the reason I just didn't move all of the initialization code over, but only what I had to.  I did test the launch times, and even with a huge collection of tabs being restored there was not any noticeable difference whatsoever.  More testing on other systems might be required to verify that, just in case.  Additionally, the dock icon will stop bouncing after the launch in cases when the modal dialog is displayed after a crash asking whether to restore.

As a plus, the following code wasn't needed anymore:

// if we've already opened a window (e.g., command line argument or apple event), we need
// to pull it to the front after restoring the window state
NSWindow* existingWindow = [self getFrontmostBrowserWindow];
[[SessionManager sharedInstance] restoreWindowState];
[existingWindow makeKeyAndOrderFront:self];

I couldn't figure out how to launch Camino with a command line argument to verify that the above check was definitely not needed.

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #23)
> As a plus, the following code wasn't needed anymore:
> 
> // if we've already opened a window (e.g., command line argument or apple
> event), we need
> // to pull it to the front after restoring the window state
> NSWindow* existingWindow = [self getFrontmostBrowserWindow];
> [[SessionManager sharedInstance] restoreWindowState];
> [existingWindow makeKeyAndOrderFront:self];
> 
> I couldn't figure out how to launch Camino with a command line argument to
> verify that the above check was definitely not needed.

Tinderbox test run via commands like this:

/builds/tinderbox/CmTrunk/Darwin_8.6.0_Depend/mozilla/../build/dist/Camino.app/Contents/MacOS/Camino -url http://www.mozilla.org/performance/test-cases/dhtml/runTests.html

so you need to make sure it's not needed.  Check the full build logs for maya to see some of the other command-line arguments that are used when running tests.

Comment 25

[Chris Lawson (gone)]

Per IRC, this is going to block b1.

Comment 26

[Sean Murphy]

(In reply to comment #24)

> so you need to make sure it's not needed.  Check the full build logs for maya
> to see some of the other command-line arguments that are used when running
> tests.
 
Supplying a url on the command line still works fine, but is processed before the window state is restored meaning it will always just open up in a new window.  A cmd line url is obtained during MainController's init, which is performed before the window state restoration code in applicationWillFinishLaunching:.

The job of the three lines of code I removed was to make sure the previous session's window state was restored behind any windows which were newly opened on launch and not part of that session (eg, argument or apple event).  The code wasn't needed anymore, since windows opened due to a command-line supplied url already open as the front/key window, and all others (such as links clicked which launch Camino) will now work properly and observe the appropriate preference in the updated patch.

I noticed that maya does measure startup time performance.  We'll still have to make sure there are no significant slowdowns when restoring previously saved sessions, since maya is most likely not using this feature.

So, to clarify a bit, command-line arguments are still working OK.  A cmd line -url will open in a new front and key window if there's also a session restored on launch.  All other links that should be opened along with a restored session will take on whatever behavior is defined by the preferences.

Comment 27

[Stuart Morgan]

Comment on attachment 251590 [details] [diff] [review]
patch v2 update

I'm not at all convinced that doing all that work in applicationWillFinishLaunching: is necessarily safe. Since there is no actual interdependence between this bug and bug 364497, and this bug is blocking our ability to make session saving generally usable (and has string changes) I would rather see this patch without anything related to bug 364497.

In general, unless bugs are more difficult to fix serially than together it's best to leave them separate as it makes both reviewing and QA easier.

Comment 28

[Sean Murphy]

Attachment: patch v2

(In reply to comment #27)
> I would rather see this patch without anything related to bug 364497.

My fault.  The attachment "patch v2" was that patch.  It was a follow-up to address everything you mentioned in comment #19 (the review from the initial patch I posted to this bug).

I'm re-submitting the attachment I mentioned above, removing a one-line fix for bug 365746 which I wrongfully included in the original.

Further explanation pertaining to the code in this patch is given in my previous comment #21.

Comment 29

[Stuart Morgan]

Comment on attachment 252335 [details] [diff] [review]
patch v2

>+- (BOOL)previousSessionCrashed
>+{
>+  // This value must be remembered since the actual preference in the defaults system is
>+  // reset to "NO" the first time it is checked.
>+  static BOOL previousSessionTerminatedNormally = NO;
>+  
>+  // Assume the application terminated normally if no key is present to otherwise 
>+  // indicate the termination status. Prevents cases where preferences from older 
>+  // versions of Camino would falsely indicate a crash.
>+  [[NSUserDefaults standardUserDefaults] registerDefaults:[NSDictionary dictionaryWithObject:@"YES" forKey:kPreviousSessionTerminatedNormallyKey]];
>+  
>+  if (previousSessionTerminatedNormally) {
>+    return NO; // Already looked at and have since reset the key in defaults.
>+  }
>+  else if ([[NSUserDefaults standardUserDefaults] boolForKey:kPreviousSessionTerminatedNormallyKey]) {
>+    // The application closed normally from the last session.
>+    // Reset the termination status preference.
>+    [[NSUserDefaults standardUserDefaults] setObject:@"NO" forKey:kPreviousSessionTerminatedNormallyKey];
>+    
>+    // Force this default to persist on disk immediately.
>+    [[NSUserDefaults standardUserDefaults] synchronize];
>+    
>+    previousSessionTerminatedNormally = YES;
>+    return NO;
>+  }
>+  else {
>+    return YES;
>+  }
>+}

Although this does work, I had to read it a couple of times to convince myself of that. I think for readability it would be worth adding another static BOOL and structuring it like:
  
  static BOOL previousSessionTerminatedNormally = NO;
  static BOOL checked = NO;
  if (!checked) {
    //register default here
    previousSessionTerminatedNormally = [... boolForKey:...];
    //set pref to no and synchronize
  }
  return previousSessionTerminatedNormally;

r=me with that change.

Comment 30

[Sean Murphy]

Attachment: patch v2 + improvements for readability

(In reply to comment #29)
> I think for readability it would be worth adding another static BOOL

Thank you Stuart - it is indeed much more understandable at first glance now.

While I was at it, I further improved readability by renaming the method to |previousSessionTerminatedNormally| instead of |previousSessionCrashed|.  The method, as it was initially named, would require the actual status variable to be inverted before returning its value:

	- (BOOL)previousSessionCrashed
	...
	if (previousSessionTerminatedNormally)
	  return NO;
	else
	  return YES;

The name of the default key could have been changed instead, but I favored this approach since the current default name seemed much more rational.

Anyway, changing the method name permits the restoration code inside of |applicationDidFinishLaunching:| to follow a more logical path.  The most common case (a normal exit last time) is handled first and restoring after a crash is now inside the else clause.

I carried down the r+, but since I made some additional changes you can feel free to double-check the patch again Stuart if you have time.

Comment 31

[Stuart Morgan]

Comment on attachment 252623 [details] [diff] [review]
patch v2 + improvements for readability

>+// -previousSessionCrashed
...
>+- (BOOL)previousSessionTerminatedNormally

The comment needs to be synced up with the new method name.

Also, when you respin please strip all the whitespace from all your blank lines.

Comment 32

[Sean Murphy]

Attachment: same as above, with comment fix

Comment 33

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 252631 [details] [diff] [review]
same as above, with comment fix

sr=pink

Comment 34

[froodian (Ian Leue)]

Landed on trunk and MOZILLA_1_8_BRANCH with the following strings (per #camino):

"RestoreAfterCrashTitle" = "Restore previous pages?";
"RestoreAfterCrashMessage" = "Your last Camino session closed unexpectedly.  Would you like to restore the web pages you were previously viewing?";
"RestoreAfterCrashActionButton" = "Restore Pages";
"RestoreAfterCrashCancelButton" = "Don’t Restore";

Comment 35

[froodian (Ian Leue)]

Backed out, since the dialog was killing TP, until we can find a better solution.

Comment 36

[Stuart Morgan]

Attachment: respecting pref

This is the same patch, but with
  else if ([prefManager getBooleanPref:"browser.sessionstore.resume_from_crash" withSuccess:NULL]) {
instead of just an |else| around tossing up the dialog, and a corresponding
  pref("browser.sessionstore.resume_from_crash", true);
in all-camino.js. This will let us land the patch once the tinderbox test scripts have been updated.

Comment 37

[Stuart Morgan]

Bug 368444 is tracking the afformentioned TB script change.

Comment 38

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 253031 [details] [diff] [review]
respecting pref

rs=pink

Comment 39

[froodian (Ian Leue)]

Pref-respecting version checked in on trunk and MOZILLA_1_8_BRANCH.

Comment 40

[Stuart Morgan]

Checked in the pref file, which slipped through yesterday, so without manually enabling the pref today's build won't restore from crashes.