Open
Bug 344852
Opened 18 years ago
Updated 2 years ago
Session restore's "This is embarrassing" page shouldn't be displayed unless we detect that a crash happens soon after startup
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
NEW
People
(Reporter: asqueella, Unassigned)
References
Details
(Whiteboard: [notacrash])
Attachments
(1 file)
(deleted),
patch
|
dietrich
:
review-
|
Details | Diff | Splinter Review |
Reporter | ||
Comment 1•18 years ago
|
||
(accidentally pressed enter, wait a bit for the real report)
Reporter | ||
Comment 2•18 years ago
|
||
Right now Session Store displays the "Would you like to restore your last session" dialog if it thinks Firefox has crashed. The dialog looks rather scary ("Bon Echo experienced a serious problem") and asks the user to choose whether to restore his session.
There are a few problems with this approach:
1) The dialog may be displayed when Firefox didn't in fact "experience a serious problem", e.g. if you shut down Windows without first exiting Firefox (see bug 333907) or if you just have to reboot the computer. (There may be other reasons - see bug 342885)
2) (I claim that) the problem the dialog is trying to solve is not *that* common. The amount of reproducible crashers on common sites is fairly low (judging by my personal experience and triaging bugzilla bugs). The probability of rebooting the computer or unreproducible crash is much higher.
3) I often click the wrong button in this dialog, which leads to losing my session.
The proposed solution is to go ahead and try to restore the session without asking the user. If it crashes again (in the first X seconds after all pages finished loading), display the dialog on next startup.
Summary: Session restore confirmation dialog o startup shouldn't be displayed unless → Session restore confirmation dialog startup shouldn't be displayed unless we detect a crash happening soon after startup
Reporter | ||
Updated•18 years ago
|
Summary: Session restore confirmation dialog startup shouldn't be displayed unless we detect a crash happening soon after startup → Session restore confirmation startup dialog shouldn't be displayed unless we detect that a crash happens soon after startup
Reporter | ||
Comment 3•18 years ago
|
||
4) The user has no way to know whether restoring a session will crash or not until he tries. So if he cares about what he was doing even a little he's going to try to restore the session. Why not do this for him?
I believe this may be an annoyance for the users (mainly because of bug 333907, other similar bugs, and forced computer restarts), so asking for blocking-firefox2.
Flags: blocking-firefox2?
Comment 4•18 years ago
|
||
Instead of saying 'Firefox Experienced a Serious Problem', why not simply say 'Firefox did not close properly the last time. Would you like to restore your last session'. Maybe not exactly this but something on these lines. More realistic and less scary.
Comment 5•18 years ago
|
||
The simplest way of fixing this:
* after a crash set the "state" field from "running" to "crashed"
* that field won't be updated for about 10 seconds after the first browser window was loaded which should give whatever caused the crash enough time to cause it again
* if there's no crash, the state switches to "running" and we're green - otherwise the state is "crashed" at the next startup and we'll have to prompt the user (prompt changes aren't included in the patch - yet).
Attachment #229753 -
Flags: review?(dietrich)
Updated•18 years ago
|
Flags: blocking-firefox2? → blocking-firefox2+
Whiteboard: [has patch][needs review dietrich]
Comment 6•18 years ago
|
||
Comment on attachment 229753 [details] [diff] [review]
only prompt on the crash after the crash
> // prompt and check prefs
>- this._doRestore = this._lastSessionCrashed ? this._doRecoverSession() : this._doResumeSession();
>+ if (this._lastSessionState == STATE_RUNNING_STR) {
>+ this._doRestore = true;
>+ try {
>+ // make sure to ask the user the next time should we crash again
>+ this._initialState.session.state = STATE_CRASHED_STR;
>+ this._writeFile(this._sessionFile, this._initialState.toSource());
>+ }
>+ catch (ex) { }
The patch didn't work because _writeFile() method needs to be added to nsSessionStartup. Also, should add a dump() in the catch block so that failure here is discoverable.
This bug should be used for the behavior change. If the dialog text is still scary/confusing, a new bug should be opened, or re-open bug 337390.
Attachment #229753 -
Flags: review?(dietrich) → review-
Comment 7•18 years ago
|
||
-> dietrich
We want something more like 60 seconds, since 10 seconds won'y cover slow pageloads or large tabsets
Assignee: nobody → dietrich
Comment 8•18 years ago
|
||
The problem is that we don't only activate session restore on crashes. On Windows, if a user restarts their OS or just shuts down their system without first exiting Firefox, session restore will be activated.
While I agree that this means we should probably remove the "serious problem" part of the confirmation dialog, I think it's important to ask users about restoring the session instead of assuming its what they want to do.
Reporter | ||
Comment 9•18 years ago
|
||
(In reply to comment #8)
> While I agree that this means we should probably remove the "serious problem"
> part of the confirmation dialog, I think it's important to ask users about
> restoring the session instead of assuming its what they want to do.
>
Yeah, that makes sense. I guess my restore-on-shutdown arguments are mainly applicable to the case of "always restore session" option set to true.
It would be nice if we could auto-restore after crash without asking, though.
zeniko: I haven't read the patch, is the 10-seconds timeout fired before or after all pages are fully loaded? I think it should fire after that.
Comment 10•18 years ago
|
||
(In reply to comment #6)
> The patch didn't work because _writeFile() method needs to be added to
> nsSessionStartup.
Sorry for that omission. I'll wait with posting another patch before the issues below are discussed though.
(In reply to comment #7)
> We want something more like 60 seconds, since 10 seconds won'y cover slow
> pageloads or large tabsets
While 10 seconds might be too little, 60 seconds seems to be too much already. In that time we could've run into a different crash already. ;) I'd rather go with 30 seconds. OTOH there needn't be a limit at all. Why not rather show the dialog at startup whenever the previous two sessions crashed? This wouldn't ask after the first crash, but always ask for subsequent crashes.
(In reply to comment #8)
> The problem is that we don't only activate session restore on crashes.
If we don't, then the user will be puzzled one way or the other. As a compromise what about showing the dialog after the very first crash with an option to not display it again except for subsequent crashes (as suggested above)? OTOH, this could also be achieved by an extension.
(In reply to comment #9)
> It would be nice if we could auto-restore after crash without asking, though.
This seems reasonable and would be easy to do with the only-ask-on-subsequent-crashes pref as suggested above.
> zeniko: I haven't read the patch, is the 10-seconds timeout fired before or
> after all pages are fully loaded? I think it should fire after that.
The timeout starts before loading the pages for the simple reason that it's much simpler than to try to guess when all tabs in all pages have finished loading (without being closed or canceled).
Whiteboard: [has patch][needs review dietrich]
Comment 11•18 years ago
|
||
The right path forward isn't clear here, minusing and moving to Fx3
Flags: blocking-firefox2+ → blocking-firefox2-
Target Milestone: --- → Firefox 3 alpha1
Reporter | ||
Comment 12•18 years ago
|
||
I think the right path is to do what I suggested initially if the "Always restore session" pref is set, and also do it in other cases when bug 333907 is fixed or when we can figure out if Firefox really crashed or was it bug 333907.
Actually bug 333907 comment 17 gave me the idea that we should be able to shut down session store upon receiving WM_ENDSESSION.
Beltzner, does it sound ok UE-wise? (I mean auto-restoring when we're almost sure there was a crash, not restoring and not asking after if hit bug 333907.)
I'm also not sure if this is safe enough for fx2 at this point. The additional code to handle WM_ENDSESSION gracefully doesn't seem particularly risky, as all we need to do is change the state in sessionstore.js.
Updated•18 years ago
|
Component: Tabbed Browser → Session Restore
Updated•18 years ago
|
QA Contact: tabbed.browser → session.restore
Comment 13•17 years ago
|
||
Also on Linux. I usually do a "poweroff" to finish my jobs. So, it's a bit annoying to have this warning every time firefox it's started!
Updated•16 years ago
|
Assignee: dietrich → nobody
Updated•16 years ago
|
Comment 14•16 years ago
|
||
In bug 448976 it was decided to implement the following instead:
* When we start up after a crash, we just try to restore the crashed session without any user interaction at all - unless Firefox hasn't been touched for a longer period of time (6 or 24 hours were suggested).
* Should a recovered session crash again (no matter how long it's already been running), we display an error page, allowing the user to selectively restore the session or defer the restoration to a later moment.
(In reply to comment #2)
> 1) The dialog may be displayed when Firefox didn't in fact "experience a
> serious problem"
These issues have since been fixed.
> 2) (I claim that) the problem the dialog is trying to solve is not *that*
> common. The amount of reproducible crashers on common sites is fairly low
Hence no error page unless we get subsequent crashes.
> 3) I often click the wrong button in this dialog
The error page will remain in the browser's history and can be reopened when accidentally closed.
Keywords: uiwanted
Comment 15•16 years ago
|
||
Alex: The only thing left is the question, whether to consider sessions of a certain age (e.g. 10 minutes) as stable, even when we'd originally recovered it (i.e. no Session Restore page if Firefox crashes again more than 10 minutes after a recovered crash).
Comment 16•16 years ago
|
||
>Alex: The only thing left is the question, whether to consider sessions of a
>certain age (e.g. 10 minutes) as stable, even when we'd originally recovered it
>(i.e. no Session Restore page if Firefox crashes again more than 10 minutes
>after a recovered crash).
I can't really provide a suggestion without knowing how common a crash 10 minutes later is. Let's go with whatever time limit you think makes the most sense.
Reporter | ||
Updated•15 years ago
|
Summary: Session restore confirmation startup dialog shouldn't be displayed unless we detect that a crash happens soon after startup → Session restore's "This is embarrassing" page shouldn't be displayed unless we detect that a crash happens soon after startup
Updated•14 years ago
|
Whiteboard: [notacrash]
Comment 18•12 years ago
|
||
When I open Firefox, even though I've closed all previous pages, I get this comment:
"Well, this is embarrassing.
Firefox is having trouble recovering your windows and tabs. This is usually caused by a recently opened web page.
You can try:
Removing one or more tabs that you think may be causing the problem
Starting an entirely new browsing session"
It's annoying when all I'm interested in getting is my normal home page. I know another person who's having the same problem. Any suggestions?
Comment 19•12 years ago
|
||
@lrouse1 I would suspect some extension, plugin, or maybe session bug is crashing Firefox while it's closing. Try enabling the Crash Reporter in Options, Advanced, Data Choices.
Anyway, this bug is not about the problem you're experiencing. See comment 2. Please don't reply here.
Comment 20•12 years ago
|
||
(In reply to lrouse1 from comment #18)
> It's annoying when all I'm interested in getting is my normal home page. I
> know another person who's having the same problem. Any suggestions?
You're probably seeing bug 845681, which we're going to fix in Firefox 21 (to be released on May 5th).
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•