Last Comment Bug 448976 - turn the Session Restore prompt into an error page
: turn the Session Restore prompt into an error page
Status: VERIFIED FIXED
: user-doc-complete
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 3.1b2
Assigned To: Simon Bünzli
:
Mentors:
: 459605 (view as bug list)
Depends on: 459561 381887 450089 457195 459651
Blocks: 407117 426123 459546 459550 459567 459593 459950 486461
  Show dependency treegraph
 
Reported: 2008-08-03 16:28 PDT by Simon Bünzli
Modified: 2009-10-27 23:35 PDT (History)
32 users (show)
mconnor: wanted‑firefox3.5+
mozillamarcia.knous: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
extension (mostly) implementing the described behavior (9.50 KB, application/x-xpinstall)
2008-08-03 16:28 PDT, Simon Bünzli
faaborg: ui‑review-
Details
extension updated to comment #10 (31.53 KB, patch)
2008-08-22 10:03 PDT, Simon Bünzli
zeniko: ui‑review+
Details | Diff | Splinter Review
version 0.9 (49.01 KB, patch)
2008-08-29 16:07 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review
extension updated to comment #14 (38.49 KB, application/x-xpinstall)
2008-08-29 16:10 PDT, Simon Bünzli
no flags Details
UI Mockup (295.94 KB, image/png)
2008-09-03 21:01 PDT, Alex Faaborg [:faaborg] (Firefox UX)
no flags Details
v0.9.1 (52.66 KB, patch)
2008-09-25 12:49 PDT, Simon Bünzli
dietrich: review-
Details | Diff | Splinter Review
v0.9.1.1 (52.67 KB, patch)
2008-10-04 04:19 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review
v0.9.1.2 (52.53 KB, patch)
2008-10-05 08:38 PDT, Simon Bünzli
dietrich: review+
Details | Diff | Splinter Review
for check-in (52.54 KB, patch)
2008-10-10 13:11 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review

Description Simon Bünzli 2008-08-03 16:28:53 PDT
Created attachment 332147 [details]
extension (mostly) implementing the described behavior

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?
Comment 1 Marco Zehe (:MarcoZ) on PTO until August 15 2008-08-03 23:06:10 PDT
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.
Comment 2 Ria Klaassen (not reading all bugmail) 2008-08-04 01:20:41 PDT
"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.
Comment 3 alexander :surkov 2008-08-09 04:29:39 PDT
Marco, could you say me please how can I find your vbox?
Comment 4 Marco Zehe (:MarcoZ) on PTO until August 15 2008-08-09 04:42:42 PDT
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.
Comment 5 Marco Zehe (:MarcoZ) on PTO until August 15 2008-08-09 04:42:57 PDT
Also, View Source helps.
Comment 6 alexander :surkov 2008-08-11 04:12:07 PDT
Ah, ok, I can see. It seems html accessible doesn't want to be friends with xul accessibles :)
Comment 7 alexander :surkov 2008-08-11 04:57:51 PDT
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 :)
Comment 8 alexander :surkov 2008-08-11 05:29:55 PDT
I filed bug 450089 for the accessibility issue.
Comment 9 Simon Bünzli 2008-08-19 16:52:17 PDT
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
Comment 10 Alex Faaborg [:faaborg] (Firefox UX) 2008-08-21 17:15:39 PDT
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 11 Alex Faaborg [:faaborg] (Firefox UX) 2008-08-21 17:15:55 PDT
Comment on attachment 332147 [details]
extension (mostly) implementing the described behavior

Details in the previous comment
Comment 12 Simon Bünzli 2008-08-22 10:03:52 PDT
Created attachment 335065 [details] [diff] [review]
extension updated to comment #10

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 13 Simon Bünzli 2008-08-27 15:57:47 PDT
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
Comment 14 Simon Bünzli 2008-08-29 16:07:39 PDT
Created attachment 336134 [details] [diff] [review]
version 0.9

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).
Comment 15 Simon Bünzli 2008-08-29 16:10:14 PDT
Created attachment 336136 [details]
extension updated to comment #14
Comment 16 XtC4UaLL [:xtc4uall] 2008-09-02 21:12:11 PDT
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 :-)
Comment 17 Simon Bünzli 2008-09-03 04:13:51 PDT
(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.
Comment 18 Alex Faaborg [:faaborg] (Firefox UX) 2008-09-03 21:01:36 PDT
Created attachment 336774 [details]
UI Mockup

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 19 Dietrich Ayala (:dietrich) 2008-09-25 12:10:21 PDT
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?
Comment 20 Simon Bünzli 2008-09-25 12:49:43 PDT
Created attachment 340395 [details] [diff] [review]
v0.9.1

(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 21 Dietrich Ayala (:dietrich) 2008-09-25 16:52:51 PDT
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
Comment 22 Simon Bünzli 2008-09-25 16:59:39 PDT
Thanks! So let's get some user feedback on these changes...
Comment 23 Dão Gottwald [:dao] 2008-09-30 05:49:26 PDT
This can't land for beta1, due to the string freeze.
Comment 24 Dietrich Ayala (:dietrich) 2008-09-30 10:55:38 PDT
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.
Comment 25 Simon Bünzli 2008-10-04 04:19:03 PDT
Created attachment 341740 [details] [diff] [review]
v0.9.1.1

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?
Comment 26 Simon Bünzli 2008-10-05 08:38:14 PDT
Created attachment 341818 [details] [diff] [review]
v0.9.1.2

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 27 Dietrich Ayala (:dietrich) 2008-10-10 12:53:44 PDT
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
Comment 28 Simon Bünzli 2008-10-10 13:11:51 PDT
Created attachment 342623 [details] [diff] [review]
for check-in

(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...
Comment 29 :Ehsan Akhgari (away Aug 1-5) 2008-10-11 11:48:30 PDT
Pushed to mozilla-central: <http://hg.mozilla.org/mozilla-central/rev/df62dc445ddc>
Comment 30 :Ehsan Akhgari (away Aug 1-5) 2008-10-11 12:04:09 PDT
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>.
Comment 31 Simon Bünzli 2008-10-11 14:07:24 PDT
Thanks for the check-in and the quick follow up fix, Ehsan!
Comment 32 Michael Ventnor 2008-10-11 18:13:43 PDT
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.
Comment 33 :Ehsan Akhgari (away Aug 1-5) 2008-10-11 23:55:19 PDT
Reopening to fix the problem in comment 32.
Comment 34 Simon Bünzli 2008-10-12 00:11:11 PDT
(In reply to comment #33)
Lets fix this in a follow up bug.
Comment 35 Kevin Brosnan 2008-10-12 15:01:36 PDT
*** Bug 459605 has been marked as a duplicate of this bug. ***
Comment 36 Marek Stępień [:marcoos, inactive] 2008-10-13 09:50:58 PDT
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? ;-)
Comment 37 Simon Bünzli 2008-10-13 10:45:37 PDT
(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.
Comment 38 Marcia Knous [:marcia - use ni] 2008-10-13 12:39:35 PDT
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?
Comment 39 Simon Bünzli 2008-10-13 12:47:04 PDT
(In reply to comment #38)
In what respect has it picked up that fact?
Comment 40 Marcia Knous [:marcia - use ni] 2008-10-13 12:49:35 PDT
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?
Comment 41 Simon Bünzli 2008-10-13 13:06:55 PDT
(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.
Comment 42 Alex Faaborg [:faaborg] (Firefox UX) 2008-10-14 16:24:04 PDT
I've filed bug 459950 with some minor polish changes to the interface.
Comment 43 Michael Kraft [:morac] 2008-10-15 13:16:35 PDT
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.
Comment 44 Marcia Knous [:marcia - use ni] 2008-10-16 15:52:49 PDT
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.
Comment 45 Henrik Skupin (:whimboo) 2008-10-17 13:47:32 PDT
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?
Comment 46 Simon Bünzli 2008-10-17 13:59:43 PDT
(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.
Comment 47 Henrik Skupin (:whimboo) 2008-10-17 14:46:48 PDT
(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.
Comment 48 Axel Hecht [:Pike] 2008-10-26 04:17:48 PDT
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?
Comment 49 Simon Bünzli 2008-10-26 05:41:25 PDT
(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.
Comment 50 Axel Hecht [:Pike] 2008-10-26 09:10:20 PDT
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.
Comment 51 Chris Ilias [:cilias] 2009-05-13 21:44:34 PDT
user-doc-complete
<https://support.mozilla.com/kb/Session+Restore?bl=n>
Comment 52 tnomam 2009-10-25 06:31:10 PDT
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.
Comment 53 Mike Connor [:mconnor] 2009-10-26 09:59:25 PDT
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.
Comment 54 Alex Faaborg [:faaborg] (Firefox UX) 2009-10-27 15:22:21 PDT
I agree, but just to confirm, we don't restore from a crash inside of private browsing, right?
Comment 55 Mike Connor [:mconnor] 2009-10-27 23:35:41 PDT
Absolutely correct.

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