Turning the Session Restore prompt into an error page would fix several outstanding issues (among others bug 407117, bug 426123, bug 371031, bug 380862 and bug 381887). The new behavior would be the following: * In the non-crash case, nothing changes * In case we crash, we add an error page as the first tab to the first opened browser window (which might already contain other tabs, although the user's homepage won't be loaded) * Firefox is fully usable with this error page open. The error page gets saved by Session Restore as any other tab and would be restored in case of a crash while the error page is still open * The error page offers by default to either restore the crashed session or to load the user's homepage(s) * The user can additionally display a list of all the windows/tabs contained in the crashed session and selectively uncheck tabs/windows from being restored (as a bonus, individual tabs can be restored through middle-clicking) * When the session is restored, the tab with the error page is automatically closed and the session is restored into new windows. The window containing the error page will remain, if other tabs have already been opened - as will other windows. Alex: Please install this extension and do a UI review of the behavior. If you agree UI wise, I'll try to turn the extension into a patch. Note that the strings aren't finalized, at all. I'd like to get this behavior for Firefox 3.1. Marco: How does this extension work from an accessibility point of view? Do you get the error text read first before the focus moves to the "Restore Previous Session" button? And is displaying the tab list discoverable and usable?

I do get the page's text read when it comes up. I can also work the buttons. But for some reason, we're currently not rendering the xul:vbox containing the tree at all. I can tab to it with the virtual feature off, and work it then, but inside the virtual buffers, it is not discoverable. It is not part of the a11y tree, platform independent or otherwise. Aaron, Alex (Surkov), is this a bug in our rendering of XUL inside HTML? I do remember seeing a xul:combobox rendered on the "Subscribe to this page" HTML page which has XUL embedded.

"If you don't want to restore this session, either load your home page or close this tab". Don't want to split hairs, but without the setting "Always show the tab bar" there is no way to close the tab, at least not for mouse users. This option was never available, but now people may go looking for it on the page, at least I did.

Marco, could you say me please how can I find your vbox?

Alex, once you install the extension and get the error page (for example by killing a running Firefox through Task manager and then restarting it), simply bring up DOM Inspector. It's in there.

Also, View Source helps.

Ah, ok, I can see. It seems html accessible doesn't want to be friends with xul accessibles :)

I believe our invalidation code is broken because explicit invalidation of accessible for that div makes xul accessibles visible in the tree. Thanks Simon for the tricky catch :)

I filed bug 450089 for the accessibility issue.

For reference: Fixing this bug as planned would also fix the following bugs: Bug 355898 – Errors if you quit while Session restore dialog is showing Bug 365581 – Session restore breaks if there's no browser window Bug 371031 – Session Restore prompt blocks Firefox from opening a shortcut with URL specified (symptom: DDE timeout) Bug 380862 – Closing "Restore Previous Session" dialog loses session restore data by starting a new session rather than stopping startup and exiting Firefox Bug 381386 – New Window via AppleEvent prevents Session Restore from working Bug 381887 – Figure out the best moment to display the Session Restore prompt Bug 384983 – Session restore kicks in with -silent Bug 407117 – Make it possible to restore all but one tab Bug 426123 – Add option to choose which tabs to restore during session restore Bug 433975 – delete sessionstore.js when starting a new session

Ok, I've finally had a chance to review the functionality and touch base with both beltzner and mconnor about it. Here is what we collectively think the best user experience is: -After the first crash, when you reopen Firefox it automatically restores your session (no dialog or error tab). We expect the vast majority of users to want their session back at this point, and very few people will be able to make intelligent decisions on which tabs to prevent opening to avoid the crash. The best dialog box is no dialog box :) -In the event that the browser crashes again, we display the restore previous session tab on opening -details are already displayed (no "Show Details" button) -Have two buttons: "Restore Previous Session" and "Start new Session" (start new session loads the home page in this tab) -Remove the sentence about loading your home page -switch the icon from an error to a question, since we are recovering from an error, not producing an error -remove question mark from Restore column, and make this the first column in the list view We will still want to work on the wording, but those are the general changes to the UI we are looking for.

Comment on attachment 332147 [details] extension (mostly) implementing the described behavior Details in the previous comment

This implements a new pref browser.sessionstore.max_resumed_crashes (default 1) which controls for how many consequent crashes we don't bother the user with questions at all. All of the other points are fixed as well. Alex: Is this how you've envisaged it? Note: This extension ships a significantly updated version of the SessionStore component which is more thorough but not fully backwards compatible. Use at your own risk.

Comment on attachment 335065 [details] [diff] [review] extension updated to comment #10 From Alex Faaborg in bug 345135 comment #3: > ui-r+ with the following changes: > > One thing I think I forgot to mention was that mconnor wanted this content area > message displayed if the crash occurred more than 24 hours ago (under the > assumption that the user may want to just start a fresh session). > > Here are some suggested string changes: > > -Title case for "Your Last Minefield Session Closed Unexpectedly" > > Trying to shorten the text a little, remove the word "crash" and added a direct > apology: > > -------------------------------------------------------------------- > You can restore the tabs and windows from your previous session, or start a new > session if they are no longer needed. Our apologies for the inconvenience. > -------------------------------------------------------------------- > If Minefield closes repeatedly: > * Try disabling any recently added extensions in the Add-ons Manager. > * Try restoring your session without any Web pages you suspect might be causing > the crash: > > [list view] > > -removed "uncheck the tabs that you don't want to restore" since this should be > self explanatory, and is referenced in the second bullet

This patch includes all the changes as asked for by Alex with one slight modification: the Session Restore page is always displayed if Firefox wasn't touched for 6 hours since the last crash. We could probably go even lower (I guess that after about half an hour you usually won't remember anymore whether Firefox crashed or not and might thus get confused by what Firefox automatically loads on the next startup).

Simon, testing your extension (therefore i set browser.sessionstore.max_resumed_crashes to "0") with Minefield/3.1b1pre ID:20080902033133 i'm getting with my "daily profile" (with several extensions etc.) following error console output: Error: Security error = NS_ERROR_DOM_SECURITY_ERR Source file: file:///***path_to_my_profile_dir***/extensions/restorepage@mozilla.zeniko.ch/components/nsSessionStore.next.js Line: 1156 ---------- Warning: reference to undefined property aTabData.attributes Source file: chrome://browser/content/aboutSessionRestore.js Line: 87 ---------- Error: aTabData.attributes is undefined Source file: chrome://browser/content/aboutSessionRestore.js Line: 87 this does not happen in a fresh profile. i'm unsure how much this is relevant, so just FYI. apart of this issue, it works & now IMHO just needs some stylish love :-)

(In reply to comment #16) Thanks for the error reports. > Error: Security error = NS_ERROR_DOM_SECURITY_ERR > Source file: > file:///***path_to_my_profile_dir***/extensions/restorepage@mozilla.zeniko.ch/components/nsSessionStore.next.js > Line: 1156 Looks like an issue with DOMStorage saving as introduced in bug 339445. Can you reliably reproduce this error by visiting a specific URL? > Error: aTabData.attributes is undefined > Source file: chrome://browser/content/aboutSessionRestore.js > Line: 87 This should only happen when you crash right after the extension was installed (at the very first startup). My guess is that you have a session managing extension overwriting sessionstore.js at shutdown. Nonetheless, we should fix this by replacing >+ src: aTabData.attributes.image || null, with >+ src: aTabData.attributes && aTabData.attributes.image || null, in content/aboutSessionRestore.js.

Here is a UI mockup showing the sequence in which we present the user with various options. The overall idea is to give them the correct set of controls depending on the current situation, instead of explaining to them how they can go somewhere else in the UI to try to resolve their problem.

Comment on attachment 336134 [details] [diff] [review] version 0.9 here's a first-pass. >diff -r f4bc794256c2 -r 23e37326755b browser/app/profile/firefox.js >--- a/browser/app/profile/firefox.js Fri Aug 29 14:26:56 2008 -0700 >+++ b/browser/app/profile/firefox.js Sat Aug 30 00:48:14 2008 +0200 >@@ -689,16 +689,19 @@ pref("browser.sessionstore.interval", 10 > // maximum amount of POSTDATA to be saved in bytes per history entry (-1 = all of it) > // (NB: POSTDATA will be saved either entirely or not at all) > pref("browser.sessionstore.postdata", 0); > // on which sites to save text data, POSTDATA and cookies > // 0 = everywhere, 1 = unencrypted sites, 2 = nowhere > pref("browser.sessionstore.privacy_level", 1); > // how many tabs can be reopened (per window) > pref("browser.sessionstore.max_tabs_undo", 10); >+// number of crashes which are automatically resumed (unless more than 6 hours >+// past since the last crash); set to -1 for always resuming automatically >+pref("browser.sessionstore.max_resumed_crashes", 1); The language is not clear here. I'd prefer something like: The number of crashes that can occur before the about:sessionstore page is displayed. This pref has no effect if >6 hours have passed since the last crash. >+const Cc = Components.classes; >+const Ci = Components.interfaces; >+ >+var gStateObject; >+var gTreeData; >+ >+// Page initialization >+ >+window.onload = function() >+{ i'd prefer a bracing style that is consistent with the rest of the sessionstore code >+ >+// User actions >+ >+function restoreSession() >+{ >+ // remove all unselected tabs from the state before restoring it >+ var ix = gStateObject.windows.length - 1; >+ for (var t = gTreeData.length - 1; t >= 0; t--) { >+ if (treeView.isContainer(t)) { >+ if (gTreeData[t].checked === 0) >+ // this window will be restored partially >+ gStateObject.windows[ix].tabs = gStateObject.windows[ix].tabs.filter(function(aTabData, aIx) gTreeData[t].tabs[aIx].checked); line length >diff -r f4bc794256c2 -r 23e37326755b browser/components/sessionstore/src/Makefile.in >--- a/browser/components/sessionstore/src/Makefile.in Fri Aug 29 14:26:56 2008 -0700 >+++ b/browser/components/sessionstore/src/Makefile.in Sat Aug 30 00:48:14 2008 +0200 >@@ -37,11 +37,12 @@ srcdir = @srcdir@ > srcdir = @srcdir@ > VPATH = @srcdir@ > > include $(DEPTH)/config/autoconf.mk > > EXTRA_PP_COMPONENTS = \ > nsSessionStore.js \ > nsSessionStartup.js \ >+ aboutSessionStore.js \ > $(NULL) > > include $(topsrcdir)/config/rules.mk iirc, needs to be added to packages-static >diff -r f4bc794256c2 -r 23e37326755b browser/components/sessionstore/src/aboutSessionRestore.js >--- /dev/null Thu Jan 01 00:00:00 1970 +0000 >+++ b/browser/components/sessionstore/src/aboutSessionRestore.js Sat Aug 30 00:48:14 2008 +0200 would be clearer if the file name indicated it was the protocol handler. >diff -r f4bc794256c2 -r 23e37326755b browser/components/sessionstore/src/nsSessionStore.js >--- a/browser/components/sessionstore/src/nsSessionStore.js Fri Aug 29 14:26:56 2008 -0700 >+++ b/browser/components/sessionstore/src/nsSessionStore.js Sat Aug 30 00:48:14 2008 +0200 >@@ -118,17 +118,17 @@ SessionStoreService.prototype = { > contractID: "@mozilla.org/browser/sessionstore;1", > classID: Components.ID("{5280606b-2510-4fe0-97ef-9b5a22eafe6b}"), > QueryInterface: XPCOMUtils.generateQI([Ci.nsISessionStore, > Ci.nsIDOMEventListener, > Ci.nsIObserver, > Ci.nsISupportsWeakReference]), > > // xul:tab attributes to (re)store (extensions might want to hook in here) >- xulAttributes: [], >+ xulAttributes: ["image"], what's this change for? >@@ -222,16 +225,30 @@ SessionStoreService.prototype = { > } > > // if last session crashed, backup the session > if (this._lastSessionCrashed) { > try { > this._writeFile(this._sessionFileBackup, iniString); > } > catch (ex) { } // nothing else we can do here >+ this._recentCrashes = (this._initialState.session && >+ this._initialState.session.recentCrashes || 0) + 1; >+ >+ const SIX_HOURS = 6 * 60 * 60 * 1000; indicate the unit in the const name >+ let max_resumed_crashes = this._prefBranch.getIntPref("sessionstore.max_resumed_crashes"); >+ let sessionAge = this._initialState.session && this._initialState.session.lastUpdate && >+ (Date.now() - this._initialState.session.lastUpdate); nit: line length >@@ -1886,17 +1904,19 @@ SessionStoreService.prototype = { > */ > saveState: function sss_saveState(aUpdateAll) { > // if crash recovery is disabled, only save session resuming information > if (!this._resume_from_crash && this._loadState == STATE_RUNNING) > return; > > this._dirty = aUpdateAll; > var oState = this._getCurrentState(); >- oState.session = { state: ((this._loadState == STATE_RUNNING) ? STATE_RUNNING_STR : STATE_STOPPED_STR) }; >+ oState.session = { state: ((this._loadState == STATE_RUNNING) ? STATE_RUNNING_STR : STATE_STOPPED_STR), lastUpdate: Date.now() }; nit: line-length question: how does this handle startup w/ externally-passed-in URI?

(In reply to comment #19) > The language is not clear here. Changed. > i'd prefer a bracing style that is consistent with the rest of the > sessionstore code Changed. > line length Fixed. > iirc, needs to be added to packages-static Same old, same old. Good catch! > would be clearer if the file name indicated it was the protocol handler. Suggestions? We've already got aboutRobots.js for about:robots. > what's this change for? For making sure that we've got pretty favicons to display in the about:sessionrestore page. Added a comment. > indicate the unit in the const name Done. > nit: line-length Fixed. > question: how does this handle startup w/ externally-passed-in URI? The same we currently handle a resumed session (i.e. not after a crash) with additional URLs passed in: The additional pages load as additional tabs in the first restored window (in this case: right beside the about:sessionrestore page).

Comment on attachment 340395 [details] [diff] [review] v0.9.1 > > would be clearer if the file name indicated it was the protocol handler. > > Suggestions? We've already got aboutRobots.js for about:robots. nm, consistency++ r=me

Thanks! So let's get some user feedback on these changes...

This can't land for beta1, due to the string freeze.

Comment on attachment 340395 [details] [diff] [review] v0.9.1 you named all the files aboutSessionRestore.*, but then put aboutSessionStore.* in all the packaging and makefiles.

Yeah, the difference between SessionStore and Session Restore really is subtle when one is in need of a vacation. Anything else I've been missing?

Another minor oversight: Single tabs could only be restored with the mouse. Now Ctrl+Enter has the same effect as Ctrl+clicking or middle-clicking.

Comment on attachment 341818 [details] [diff] [review] v0.9.1.2 >+// number of crashes that can occur before the about:sessionstore page is displayed >+// (this pref has no effect if more than 6 hours have passed since the last crash) >+pref("browser.sessionstore.max_resumed_crashes", 1); about:sessionrestore >-# Localization note: It is recommended that okTitle be longer than cancelTitle >-# so that hitting the more prominent button doesn't lead to dataloss (see bug 346264). >-okTitle=Restore Previous Session >-cancelTitle=Start New Session might look odd in some locales, but maybe should just enforce a min-width? fine for a followup here. r=me

(In reply to comment #27) > might look odd in some locales, but maybe should just enforce a min-width? I'm not sure a min-width'd button would look better. Then again: This is just a recommendation...

Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/df62dc445ddc>

This patch caused a build bustage <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1223751060.1223751443.28669.gz> because it didn't remove sessionstore.properties from browser/locales/jar.mn. I fixed this as <http://hg.mozilla.org/mozilla-central/rev/98f6214e28d0>.

Thanks for the check-in and the quick follow up fix, Ehsan!

Comment on attachment 342623 [details] [diff] [review] for check-in >+#errorPageContainer { >+ background-image: url("chrome://global/skin/icons/question-48.png"); >+} >+treechildren::-moz-tree-image(container, noicon) { >+ list-style-image: url("chrome://global/skin/icons/folder-item.png"); >+ -moz-image-region: rect(0px, 32px, 16px, 16px); >+} >+treechildren::-moz-tree-image(container, open, noicon) { >+ -moz-image-region: rect(16px, 32px, 32px, 16px); Oopsie, Gnomestripe doesn't have these files! To get any images to display you'll need to use the stock icons. Replace the first one with: url("moz-icon://stock/gtk-dialog-question?size=dialog"); and the second one with url("moz-icon://stock/gtk-directory?size=menu"); (assuming its the folder that you want). Remove the open rule. I'd make a patch myself but I have other things to do at the mo.

Reopening to fix the problem in comment 32.

(In reply to comment #33) Lets fix this in a follow up bug.

Comment on attachment 342623 [details] [diff] [review] for check-in +<!-- LOCALIZATION NOTE: %S will be replaced with a number. --> +<!ENTITY restorepage.windowLabel "Window #&#37;S"> Shouldn't the &#37; entity be replaced with a real percent sign? ;-)

(In reply to comment #36) > Shouldn't the &#37; entity be replaced with a real percent sign? ;-) That's intentional: % is used for parameter entities and can't thus stand for itself. Please file a bug, though, should our L10n tools not be able to correctly handle this case.

Simon: I noticed that the session restore page picked up the fact that I had updated from yesterday's nightly to today - is that expected?

(In reply to comment #38) In what respect has it picked up that fact?

I had a session open from yesterday and then updated to the nightly. When I go to about:sessionrestore I see all of the windows and tabs I had open listed there, even though it was not an abnormal shutdown. (In reply to comment #39) > (In reply to comment #38) > In what respect has it picked up that fact?

(In reply to comment #40) That's a side-effect of how we load the session state into about:sessionrestore. In that case, the text is obviously wrong and the displayed session state is the one from the last startup (if you have Firefox always resume your session or after restarts). IOW: The behavior of about:sessionrestore is currently undefined when not automatically loaded by Firefox after a crash. Please file a new bug if you think that this should change.

I've filed bug 459950 with some minor polish changes to the interface.

I found a problem with the way this is implemented. Since Firefox waits 10 seconds at startup before writing to the sessionstore.js file, if the browser keeps crashing within those 10 seconds, the session restore prompt will never show up. Instead the browser will keep trying to restore the crashing session over and over again until the user deletes the sessionstore.js file. I filed this as bug 460112.

I have added a basic test case to cover this change - https://litmus.mozilla.org/show_test.cgi?id=7059. There will be a revision of the existing test cases as well as additional ones added when I have time.

Verified with by following the given Litmus test: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081017 Minefield/3.1b2pre ID:20081017020232 Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081017 Minefield/3.1b2pre When Session Restore is disabled and Firefox crashes we always open all the windows and tabs from the last session. There is no way to de-select a tab which caused the crash. Before the check-in users got the dialog to select new session/restore session which isn't available anymore now. Users will end in an endless loop now. :( Simon, is there already a bug about?

(In reply to comment #45) > When Session Restore is disabled and Firefox crashes we always open all the > windows and tabs from the last session. Only after the first crash. After the second crash you should get the new Session Restore page (unless the first crash happens less than 10s after startup in which case you're observing bug 451292 - to be fixed in bug 395488). > There is no way to de-select a tab which caused the crash. Should you actually get to the Session Restore page, you're running into bug 459746 which fails to show the checkboxes on OS X. > Users will end in an endless loop now. With bug 451292 fixed, this shouldn't be possible. To be on the safe side, we'll drop the automatic crash recovery for Safe Mode in bug 459593, though.

(In reply to comment #46) > (In reply to comment #45) > > When Session Restore is disabled and Firefox crashes we always open all the > > windows and tabs from the last session. > > Only after the first crash. After the second crash you should get the new > Session Restore page (unless the first crash happens less than 10s after > startup in which case you're observing bug 451292 - to be fixed in bug 395488). Probably this was the cause. I was too quick in crashing the browser. Now after waiting more than 10s I see bug 459651 each time I restart Firefox. But lets move the discussion to that bug if you need more information. > Should you actually get to the Session Restore page, you're running into bug > 459746 which fails to show the checkboxes on OS X. That's true. Thanks. > With bug 451292 fixed, this shouldn't be possible. To be on the safe side, > we'll drop the automatic crash recovery for Safe Mode in bug 459593, though. Ok, will re-check after bug 451292 is fixed.

AFAICT, this bug added aboutSessionRestore.dtd to the tree, http://hg.mozilla.org/mozilla-central/log/c4da2fa2e6c4/browser/locales/en-US/chrome/browser/aboutSessionRestore.dtd. I don't find it in the patches, though. <!-- LOCALIZATION NOTE: %S will be replaced with a number. --> <!ENTITY restorepage.windowLabel "Window #&#37;S"> looks way odd, and the localization note above it doesn't really help. Did this file land in another bug or without review?

(In reply to comment #48) > I don't find it in the patches, though. Have you had a look at the patch named "for checkin"? Edit->Find is your friend. > <!-- LOCALIZATION NOTE: %S will be replaced with a number. --> > <!ENTITY restorepage.windowLabel "Window #&#37;S"> > > looks way odd, and the localization note above it doesn't really help. See comment #37.

Then the localization note should call out that &#37; stands for %. Right now, it talks about %S, and that doesn't exist in the localizable string. And yeah, sorry, I looked at the diff view, which is apparently broken.

user-doc-complete <https://support.mozilla.com/kb/Session+Restore?bl=n>

The result of this bug is a gaping privacy concern, which even the previous comment's link on support.mozilla.com makes apparent (though I will mention more serious aspects). https://support.mozilla.com/en-US/kb/Session+Restore#Privacy_issues In the time between a person asking the browser to start immediately following a crash and the browser (and any number of tabs in its session) actually loading, any number of things can happen, including any person walking up to you and looking at your screen just in time for the browser to load, _popping up_ the session for their viewing pleasure. One can also suffer a browser crash and not immediately re-start the browser and then forget entirely what had been loaded, potentially leading to the next person to run the program to have no choice but to get an automatically restored session, which is almost definitely going to be annoying and can potentially thoroughly disrupt people's relationships. Even with the 'This is embarassing' notice, titles of sites in the session are displayed, presenting the very same privacy issues. Given 3.0's behavior, the only somewhat responsible way to implement this is to use 3.0's restore-or-not dialog on the first crash, and simultaneously inform the user that subsequent crashes will utilize a new behavior, in which sessions are automatically restored without being prompted, etc.

THe privacy concerns are not new, anyone can watch your session restore itself, anyone can restore the session if they want to snoop. If you want to avoid others discovering what you're doing, private browsing is likely your best solution.

I agree, but just to confirm, we don't restore from a crash inside of private browsing, right?

Absolutely correct.