347680 - Add Safe Mode option to Session Restore dialogue

Status: NEW

(Firefox :: Session Restore, enhancement)

Description

[Ian Pottinger]

It would be useful to add a checkbox to the Session Restore dialogue to start in Safe Mode since that is one of the first steps in diagnosing the cause of a crash.

Comment 1

[Dave Townsend [:mossop]]

*** Bug 349745 has been marked as a duplicate of this bug. ***

Comment 2

[Jackie Liu]

When restoring from a crash, Firefox should make some mention of "safe mode" in
the dialog about restoring the session. Something like this:

If you are continually experiencing problems, Firefox "safe mode" may help you
diagnose the problem.  If you don't experience the problem in "safe mode", then
you may have a corrupted profile or a faulty add-on (extensions/themes).

Rather than a checkbox, though, I think that a "Restart Firefox in Safe Mode" button among "Restore Session" and "Start New Session" would be good. :)

Comment 4

[(mostly gone) XtC4UaLL [:xtc4uall]]

i prefer an entry in the help menu over in Bug 417272 ;-)

Comment 5

[Simon Bünzli]

Alex: Do we also want to mention Safe Mode on the new Session Restore page?

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

We should try to help the user with as little UI overhead as possible.  What if on the third crash in a row we automatically start in safe mode, I'll attach a quick mockup.

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: UI mockup of about:safemode

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 8

[Michael Kohler [:mkohler]]

Taking this, but please don't expect a result in the next few days. Is the mockup still up-to-date?

Comment 9

[Michael Kohler [:mkohler]]

Attachment: UI of about:safemode

Comment 10

[Michael Kohler [:mkohler]]

Comment on attachment 379963 [details] [diff] [review]
UI of about:safemode

* TODO: insert icon where now is [ICON]
* TODO: find another possibility to color the warning text

Comment 11

[Simon Bünzli]

Comment on attachment 379963 [details] [diff] [review]
UI of about:safemode

Please implement the back-end first.

Comment 12

[Alex Faaborg [:faaborg] (Firefox UX)]

>Is the mockup still up-to-date?

yep

Comment 13

[Dave Townsend [:mossop]]

(In reply to comment #7)
> Created an attachment (id=336775) [details]
> UI mockup of about:safemode
> 
> 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.

What happens if say I crash while trying to restore from about:sessionrestore and I choose to not restart. Instead I quit and go on holiday for a week and come back. Should Firefox still start straight up in safe mode?

Comment 14

[Dave Townsend [:mossop]]

Assuming all you care about is detecting crashes caused by restoring the session then the approach would be something like:

1. When the user clicks "Restore previous session" in about:sessionrestore create a flag file in the profile folder.
2. When the session is restored (measuring this is problematic) remove the flag file in the profile folder.
3. In the code that checks for the -safe-mode command line argument also check whether the flag file exists, if it does remove it and force safe mode. This is probably a small Ts hit.

Comment 15

[Dave Townsend [:mossop]]

(In reply to comment #13)
> (In reply to comment #7)
> > Created an attachment (id=336775) [details] [details]
> > UI mockup of about:safemode
> > 
> > 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.
> 
> What happens if say I crash while trying to restore from about:sessionrestore
> and I choose to not restart. Instead I quit and go on holiday for a week and
> come back. Should Firefox still start straight up in safe mode?

Actually the UI mockup is a bit ambiguous. When it says "Firefox crashes without restoring previous session" does this actually mean "Firefox crashes before displaying the session restore page" as the mockup suggests, or does it actually mean "Firefox crashes after clicking restore previous session on the session restore page"? Similarly for the safe mode choice

Comment 16

[Michael Kohler [:mkohler]]

(In reply to comment #15)
> (In reply to comment #13)
> > What happens if say I crash while trying to restore from about:sessionrestore
> > and I choose to not restart. Instead I quit and go on holiday for a week and
> > come back. Should Firefox still start straight up in safe mode?
> 
> Actually the UI mockup is a bit ambiguous. When it says "Firefox crashes
> without restoring previous session" does this actually mean "Firefox crashes
> before displaying the session restore page" as the mockup suggests, or does it
> actually mean "Firefox crashes after clicking restore previous session on the
> session restore page"? Similarly for the safe mode choice

IMO we shouldn't start in Safe Mode when "Firefox crashes after clicking restore previous session on the session restore page" because the user can avoid this with unchecking the malfunctioning tab.

We should display the about:safemode (and start in Safe Mode) only when Firefox crashes before or while displaying about:sessionrestore.

Comment 17

[Michael Kohler [:mkohler]]

(In reply to comment #13) 
> What happens if say I crash while trying to restore from about:sessionrestore
> and I choose to not restart. Instead I quit and go on holiday for a week and
> come back. Should Firefox still start straight up in safe mode?

AFAICT this would bring up about:sessionrestore again. Certainly not about:safemode since extensions are not the problem here. (see comment 16 too)

Comment 18

[Dave Townsend [:mossop]]

(In reply to comment #17)
> (In reply to comment #13) 
> > What happens if say I crash while trying to restore from about:sessionrestore
> > and I choose to not restart. Instead I quit and go on holiday for a week and
> > come back. Should Firefox still start straight up in safe mode?

The same question goes for whether to start in safe mode or not when I last crashed 2 weeks ago.
 
> AFAICT this would bring up about:sessionrestore again. Certainly not
> about:safemode since extensions are not the problem here. (see comment 16 too)

You don't know that.

(In reply to comment #16)
> IMO we shouldn't start in Safe Mode when "Firefox crashes after clicking
> restore previous session on the session restore page" because the user can
> avoid this with unchecking the malfunctioning tab.
> 
> We should display the about:safemode (and start in Safe Mode) only when Firefox
> crashes before or while displaying about:sessionrestore.

So basically then all you are asking is whether the application crashed before displaying anything last time round. This isn't really related to session restore. You will probably want to do something like this:

1. Early in application startup create a flag file in the profile folder.
2. Once the application has started up (measuring this is problematic) remove the flag file.
3. When the startup code checks for the -safe-mode argument have it check for the flag file too.

However you now get into the fun of trying to deal with this for all toolkit based applications, assuming you want good coverage of startup crashers. You would either need to implement a way for toolkit apps to opt-in to this behaviour and have them deal with when to remove the flag file, or you need to remove the flag file from a toolkit component, but there is no good place where you can assert that the application finished loading.

Comment 19

[Michael Kohler [:mkohler]]

This would mean that we show about:safemode when Firefox fails to restore some tabs and crashes again, right? I'd be fine with that, the other issue (crash at startup or while displaying about:sessionrestore) can be fixed in another bug.

Comment 20

[Dave Townsend [:mossop]]

(In reply to comment #19)
> This would mean that we show about:safemode when Firefox fails to restore some
> tabs and crashes again, right? I'd be fine with that, the other issue (crash at
> startup or while displaying about:sessionrestore) can be fixed in another bug.

That is what the process in comment 14 would mean yes, or at least a minor variation of.

Comment 21

[Simon Bünzli]

We already count the number of crashes. Why not just slightly extend about:sessionrestore to offer a few further options after 1 more crashes than .max_resumed_crashes, offering to (1) disable all add-ons, (2) reset user-preferences and (3) start in Safe Mode (and ensuring that the user restarts after doing any of these, but still letting him/her continue if s/he chooses to do so)? This extended about:sessionrestore version would still look quite similar to Alex' about:safemode mock up.

A different bug would then be to automatically start Firefox in Safe Mode if it doesn't even get to a yet to be specified startup point (functionality which might be useful to all Toolkit apps).

Comment 22

[Michael Kohler [:mkohler]]

Thanks, I'll try to do it like you suggested. (but I won't finish that within the next 2 weeks b/c of holidays)

Comment 23

[Michael Kohler [:mkohler]]

(In reply to comment #21)
> A different bug would then be to automatically start Firefox in Safe Mode if it
> doesn't even get to a yet to be specified startup point (functionality which
> might be useful to all Toolkit apps).

Filed bug 502958 for this.

Comment 24

[Michael Kohler [:mkohler]]

After the user for example disabled all addons, it should be enabled to revert that if restoring the session failed again, right? The same for resetting the preferences.

Comment 25

[Michael Kohler [:mkohler]]

See <https://wiki.mozilla.org/User:MichaelKohler/SafeModeOption> for more information.

Comment 26

[Nochum Sossonko [:Natch]]

/me thinks this needs some updating since bug 499123 landed.

Correct me if I'm wrong!

uiwanted: see url for mock-up and feature plan.

Comment 27

[Tyler Downer [He/Him]]

should reset bookmarks be changed to delete to reflect bug 503457?

Comment 28

[Michael Kohler [:mkohler]]

(In reply to comment #26)
> /me thinks this needs some updating since bug 499123 landed.

Yes. I don't plan to use an own about: page.


(In reply to comment #27)
> should reset bookmarks be changed to delete to reflect bug 503457?

See comment 21 where Simon doesn't talk about resetting bookmarks as option

Comment 29

[Alex Faaborg [:faaborg] (Firefox UX)]

Your suggestions sound right on, particularly this part:

>I think that a normal user would be very confused about this, since he/she >might not even know Safe Mode.

Since the user has just launched into safe mode, should we check "disable all add ons" by default? (to represent the current status, and to reduce the number of steps)?

Comment 30

[Alex Faaborg [:faaborg] (Firefox UX)]

>uiwanted

Perhaps we want to morph this bug a little, away from adding a Safe Mode checkbox, and towards automatically launching into Safe Mode when Firefox detects that it isn't able to even open about:sessionrestore.

The overall problem with adding a Safe Mode checkbox (outside of the user potentially never getting a chance to even see it) is that it requires the user to know what Safe Mode is, why they would want to use it, etc.  This means that they have to proactively go to support forms, ask people questions, learn about how Firefox works, etc.

The overall theme of this mockup: https://bugzilla.mozilla.org/attachment.cgi?id=336775

is that Firefox would be able to detect what was going on, and take the logical action itself.

Comment 31

[Jennifer Morrow [:Boriss] (UX)]

    > Since the user has just launched into safe mode, should we check "disable all
    > add ons" by default? (to represent the current status, and to reduce the number
    > of steps)?

I'm a bit worried that we're giving the user options they can't really weigh. 
    They know they've crashed and something's wrong, but asking them to choose
    between disabling addons and reseting bookmarks, for instance, seems to be
    making the user make a decision they don't know or care about.  Is there any
    way to take an intelligent guess on what will help based on the nature of the
    crash, or to direct a user down a path that's most likely to work?

Comment 32

[Jennifer Morrow [:Boriss] (UX)]

(In reply to comment #30)

Heh.. mid-air comment collision, but we seem to be saying about the same thing.  Perhaps Firefox should be taking the best guess, letting the user know it did, and then allowing for more information or override in what would become the "advanced" menu.

Comment 33

[Alex Faaborg [:faaborg] (Firefox UX)]

I agree, also is there any case where resetting bookmarks would actually help them out?  We might want to escalate the set of things we are suggesting they do based off of what was previously attempted.

As for not checking disabled addons by default, I'm worried that a particular extension might be the reason that Firefox is not starting up, and that some users will crash three times, see this dialog, click continue (without permanently disabling addons, or even reading the message), and follow that pattern every time they launch.  By checking it by default we will at least give them a clean launch next time (hopefully).

Comment 34

[Alex Faaborg [:faaborg] (Firefox UX)]

(Also note that when I say crash three times, this step is probably transparent to the user)

Comment 35

[Michael Kohler [:mkohler]]

Thanks for the feedback. I won't be able to do the patch within the next 2 weeks I guess, because I have a lot to do for school, but I will work on this as often as I can. :)

Comment 36

[Alex Faaborg [:faaborg] (Firefox UX)]

>I have now done some work on this bug. Now I stand before a big decision. The >current situation is:

>Firefox has crashed once more than specified in >browser.sessionstore.max_resumed_crashes and the about:sessionrestore page is >shown. Now the browser crashes again and should start in Safe Mode. Do we either >want to:

>- restart Firefox, check if it needs to load in Safe Mode when >about:sessionrestore is loading and if it needs to, restart again
>- or already test if it needs to load in Safe Mode (means recentCrashes > >max_resumed_crashes + 1) when it's startup up (this would mean in nsAppRunner [1])

>The first solution would be way easier to do. But the second one is probably >better because it needs one startup less. Unfortunately I'd not have any clue >how to test the condition written above..

I'm thinking probably the second option.  Viewing the session restore page already means 2 quickly sequential crashes, followed by loading about:session restore.

If Firefox has to crash 4 times in sequence (instead of 3) before we load in safe mode, the user probably wont be getting too much visual feedback, since we are looking at roughly startup time * 4 before they see something.

That said, if implementing this on 4 crashes is so much easier than 3 that it is the difference between getting the feature landed or having the bug remain open, then let's go for 4 and bring it down to 3 in a follow up :)

Comment 37

[Michael Kohler [:mkohler]]

Okay I will do the version with 4 startups and then file a followup bug since I am not that good in programming C++.  This is just a short version because I am writing on my mobile phone and have lost the sent mail. Please see the m.d.a.f. post for further information (nothing important)

Comment 38

[Michael Kohler [:mkohler]]

Attachment: SR: patch v0.1

This is the part of my patch which edits the sessionstore and other browser/ parts.

What this patch does:
* restart Firefox in Safe Mode after [max_resumed_crashes + 2] crashes
* advanced about:sessionrestore UI with the checkboxes to make permanent changes
* avoids the <dialog> of safeMode.xul at SM start

I will file follow up bugs for different improvement which would be overkill for this bug. For further information on these follow up bugs and the general discussion about this bug: http://groups.google.com/group/mozilla.dev.apps.firefox/browse_thread/thread/9528ec70c6ae3fdc

Requesting r?
* Simon Bünzli for the SR parts
* Dão Gottwald for the browser/* parts

Requesting ui-r?
* Alex Faaborg for an official ui-r+ on this

Comment 39

[Michael Kohler [:mkohler]]

Attachment: startup: patch v0.1 [trivial]

Comment 40

[Simon Bünzli]

Comment on attachment 413957 [details] [diff] [review]
SR: patch v0.1

A first bunch of comments:

>+  var smFile = Cc["@mozilla.org/file/directory_service;1"].
>+               getService(Ci.nsIProperties).
>+               get("ProfD", Ci.nsIFile);
>+  smFile.append("startSM.tmp");
>+  if (smFile.exists())
>+    smFile.remove(false);

Spreading the handling of this file all over the at least four files will make things quite fragile. about:sessionrestore shouldn't really need to know about it - just display the additional Safe Mode UI whenever we're in Safe Mode (or else pass in a flag as we already pass in the data for the session to be potentially restored).

>+function restartFirefox() {
>+  var disableAddons = document.getElementsByName("disableAddons")[0];
>+  var resetToolbar = document.getElementsByName("resetToolbar")[0];
>+  var resetPrefs = document.getElementsByName("resetPrefs")[0];

These options deserve their own IDs instead of just names (as everything else).

>+   const nsIDirectoryServiceContractID = "@mozilla.org/file/directory_service;1";
>+   const nsIProperties = Components.interfaces.nsIProperties;
>+   var directoryService =  Components.classes[nsIDirectoryServiceContractID]
>+                                     .getService(nsIProperties);

Just inline the constants here, and consistently use Cc and Ci for keeping things short.

>+   if (localstoreFile.exists())
>+     localstoreFile.remove(false);

try-catch the removal, in case the file is write-locked. Not sure whether to just ignore that case and continue, though.

>+    document.getElementById("errorShortDesc").style.display = "none";
>+    document.getElementById("safemodeShortDesc").style.display = "inline";

I'd prefer doing the showing/hiding of the advanced UI through CSS rules on an attribute set on the document's root element (so that themes can more easily further adjust the appearance of this page).

Then again, why can't you use errorShortDesc for Safe Mode as well?

>+    document.getElementById("errorLongDesc").innerHTML = 
>+      "<p>" + permanentDesc + "</p>" +
>+      "<div id=\"fixBulletProblem\">" +
>+        "<ul>" +
>+          "<li><input type=\"checkbox\" name=\"disableAddons\" onchange=\"displayRestartMsg()\" checked=\"checked\" />" + disableAddons + "</li>" +
>+          "<li><input type=\"checkbox\" name=\"resetToolbar\" onchange=\"displayRestartMsg()\" />" + resetToolbar + "</li>" +
>+          "<li><input type=\"checkbox\" name=\"resetPrefs\" onchange=\"displayRestartMsg()\" />" + resetPrefs + "</li>" +
>+        "</ul>" +
>+      "</div>";

Why not just include this (hidden) in the xhtml file itself?

>+  if (disableAddons.checked || resetToolbar.checked || resetPrefs.checked)
>+    document.getElementById("restartNeededDesc").style.display = "inline";
>+  else
>+    document.getElementById("restartNeededDesc").style.display = "none";

Same as above.

>+  // automatically restart Firefox in normal mode when the user has unchecked
>+  // the "Disable all add-ons" checkbox and no other checkbox is checked
>+  if (!resetToolbar.checked && !resetPrefs.checked && !disableAddons.checked)
>+    restartFirefox();

This seems wrong: touching a checkbox could lead to an application restart.

>+errorTitle=Your Last Minefield Session Closed Unexpectedly

What's a "Minefield"? ;-)

>+restartNeeded=Firefox will need to restart to apply your permanent changes.
>+restartButton=Restart Firefox
>+restartButtonAccess=R

These three values are only needed in the DTD, aren't they?

>+    locale/browser/aboutSessionRestore.properties  (%chrome/browser/aboutSessionRestore.properties)

Actually, the DTD alone should suffice, if you move things around as mentioned above.

>+#restartNeededDesc {
>+  color: #990000;

Why not just put this in bold and add the proper icon besides it? Changing colors will be tricky.

Comment 41

[Michael Kohler [:mkohler]]

(In reply to comment #40)
> (From update of attachment 413957 [details] [diff] [review])

Thanks for reviewing. This sounds way better!

> >+errorTitle=Your Last Minefield Session Closed Unexpectedly
> 
> What's a "Minefield"? ;-)

oops :)

> >+#restartNeededDesc {
> >+  color: #990000;
> 
> Why not just put this in bold and add the proper icon besides it? Changing
> colors will be tricky.

Which would be the appropriate icon?

Comment 42

[Simon Bünzli]

(In reply to comment #41)
> Which would be the appropriate icon?

What about the same Info icon the Add-on manager uses for the same message (just the 32px version instead of the 16px one)?

Comment 43

[Michael Kohler [:mkohler]]

(In reply to comment #40)
> (From update of attachment 413957 [details] [diff] [review])
> A first bunch of comments:
> 
> >+  var smFile = Cc["@mozilla.org/file/directory_service;1"].
> >+               getService(Ci.nsIProperties).
> >+               get("ProfD", Ci.nsIFile);
> >+  smFile.append("startSM.tmp");
> >+  if (smFile.exists())
> >+    smFile.remove(false);
> 
> Spreading the handling of this file all over the at least four files will make
> things quite fragile. about:sessionrestore shouldn't really need to know about
> it - just display the additional Safe Mode UI whenever we're in Safe Mode (or
> else pass in a flag as we already pass in the data for the session to be
> potentially restored).

And where should we remove the file?

Comment 44

[Simon Bünzli]

(In reply to comment #43)
> And where should we remove the file?

Ideally in the same place you create it (for which IMO either nsBrowserGlue.js or nsSessionStartup.js would be a reasonable place). Then again, is it really that difficult to instead pass a command line argument along when restarting?

Comment 45

[Michael Kohler [:mkohler]]

(In reply to comment #44)
> (In reply to comment #43)
> > And where should we remove the file?
> 
> Ideally in the same place you create it (for which IMO either nsBrowserGlue.js
> or nsSessionStartup.js would be a reasonable place). Then again, is it really
> that difficult to instead pass a command line argument along when restarting?

AFAIK it is, because nsIAppStartup can't handle and pass command line arguments. And I don't see any other possibility to do that (at least not one that doesn't need an additional file). Of course we could set a environment variable and check for that at startup, but what about builds that don't have the crash reporter?

Comment 46

[Michael Kohler [:mkohler]]

How can I display the restart message with CSS rules? When I change an attribute on the root element when (un)selecting a checkbox the css rule doesn't work because the page is not reloaded..

Comment 47

[Michael Kohler [:mkohler]]

Attachment: SR: patch v0.2

this patch addresses the comments from Simon's first review.

Comment 48

[Simon Bünzli]

Comment on attachment 414747 [details] [diff] [review]
SR: patch v0.2

Another round of comments:

>+  if (XRE.inSafeMode) {
>+    var container = document.getElementById("errorPageContainer");
>+    container.setAttribute("sm", "");

What's wrong with "safemode"? That'll also make the CSS below more readable.

>+     try {
>+       localstoreFile.remove(false);
>+     } catch (ex) {

IIRC, we always don't add code after a closing brace in new code - so please move the catch to a new line.

>+function updateRestartMsg() {
>+  var disableAddons = document.getElementById("disableAddons");
>+  var resetToolbar = document.getElementById("resetToolbar");
>+  var resetPrefs = document.getElementById("resetPrefs");
>+
>+  var container = document.getElementById("errorPageContainer");
>+  if (!container.hasAttribute("restartneeded"))
>+    container.setAttribute("restartneeded", "");

This seems unnecessarily fragile to me. Why not just set the attribute when any of the checkboxes is checked and remove it when none of them is?

Also, instead of fetching the three checkboxes and checking them individually, why not get fancy and just do

>+  if (document.querySelector("#safemodeLongDescText :checked"))

>         <h1 id="errorTitleText">&restorepage.errorTitle;</h1>
>+        <h1 id="errorTitleSMText">&aboutsafemode.errorTitle;</h1>

Please spell out SafeMode.

>+              <li><input type="checkbox" id="disableAddons" onchange="updateRestartMsg()" checked="checked" />&aboutsafemode.disableAddons;</li>

For a11y, please put a <label for="disableAddons"></label> around the text (and add a space between checkbox and label).

>+        <div id="restartNeededDesc">
>+          <p><img src="moz-icon://stock/gtk-dialog-info?size=32" /> &aboutsafemode.restartNeeded;</p>

I doubt that stock gtk icons are available on Windows and OS X. You'll likely have to set the image through CSS according to what the theme ships (as already done for #errorPageContainer's background image).

>+#errorPageContainer[sm] #errorTitleSMText,
>+#errorPageContainer[sm] #safemodeShortDescText,
>+#errorPageContainer[sm] #safemodeLongDescText {
>+  display: block;
>+}

This shouldn't be necessary, if you only hide these elements for #errorPageContainer:not([safemode]).

>+#errorPageContainer[restartneeded] #restartNeededDesc {
>+  display: inline;
>+}

And this rule shouldn't be needed at all...

>+#errorPageContainer:not([restartneeded]) #restartNeededDesc {
>+  display: none;
>+}

... while this one can be included above.

Comment 49

[Michael Kohler [:mkohler]]

Attachment: SR: patch v0.3

(In reply to comment #48)
> (From update of attachment 414747 [details] [diff] [review])
> ....

>>+#errorPageContainer[sm] #errorTitleSMText,
>>+#errorPageContainer[sm] #safemodeShortDescText,
>>+#errorPageContainer[sm] #safemodeLongDescText {
>>+  display: block;
>>+}

> This shouldn't be necessary, if you only hide these elements for
> #errorPageContainer:not([safemode]).

It is necessary. If I change that, the only thing displayed is the restart message. Not exactly what we want ;)

Comment 50

[Simon Bünzli]

Comment on attachment 414784 [details] [diff] [review]
SR: patch v0.3

Starting to look good. A few more details, though:

>+              <li><input type="checkbox" id="disableAddons" onchange="updateRestartMsg()"
>+                         checked="checked" />&nbsp;<label for="disableAddons">&aboutsafemode.disableAddons;</label></li>

Any reason this checkbox defaults to checked?

>+              <li><input type="checkbox" id="resetToolbar" onchange="updateRestartMsg()" />&nbsp;
>+                  <label for="resetToolbar">&aboutsafemode.resetToolbar;</label></li>

A non-breaking space followed by breaking space? Either one should be fine (i.e. we should be able to do without &nbsp;).

>+#restartNeededDesc > p {
>+  background: url("chrome://global/skin/console/bullet-question.png") no-repeat;
>+  padding-left: 2em;
>+  height: 16px;
>+}

Why the 2em padding? Shouldn't that be 16px+some? Else this will look odd for really large/small fonts. Also, shouldn't this be min-height instead of height (then you'd have to make sure to top-left-align the background). Also, does a bigger icon look bad or don't we have a 32x32px version of it?

>+#restartFirefox > button {
>+  font-weight: normal;

That shouldn't be needed, if you only make the <p> containing the text bold.

(In reply to comment #49)
> > This shouldn't be necessary, if you only hide these elements for
> > #errorPageContainer:not([safemode]).
> 
> It is necessary. If I change that, the only thing displayed is the restart
> message. Not exactly what we want ;)

I fail to see where everything else would be hidden. Since you only hide parts of the page depending on [safemode] resp. :not([safemode]), everything not specifically hidden should be visible.

Comment 51

[Michael Kohler [:mkohler]]

Attachment: SR: patch v0.4

(In reply to comment #50)
> (From update of attachment 414784 [details] [diff] [review])
> Starting to look good. A few more details, though:
> 
> >+              <li><input type="checkbox" id="disableAddons" onchange="updateRestartMsg()"
> >+                         checked="checked" />&nbsp;<label for="disableAddons">&aboutsafemode.disableAddons;</label></li>
> 
> Any reason this checkbox defaults to checked?

See http://groups.google.com/group/mozilla.dev.apps.firefox/msg/12ac0875b3447ed0?dmode=source . I can change it if this is not wanted.

> >+#restartNeededDesc > p {
> >+  background: url("chrome://global/skin/console/bullet-question.png") no-repeat;
> >+  padding-left: 2em;
> >+  height: 16px;
> >+}
> 
> Why the 2em padding? Shouldn't that be 16px+some? Else this will look odd for
> really large/small fonts. Also, shouldn't this be min-height instead of height
> (then you'd have to make sure to top-left-align the background). Also, does a
> bigger icon look bad or don't we have a 32x32px version of it?

Right. And no, AFAICS there is no 32x32px version of it.

Comment 52

[Simon Bünzli]

Comment on attachment 415021 [details] [diff] [review]
SR: patch v0.4

The about:sessionrestore related bits look alright, code-wise.

(In reply to comment #51)
> . I can change it if this is not wanted.

I'll leave that for Alex to decide.

> Right. And no, AFAICS there is no 32x32px version of it.

For Strata, that'd be chrome://global/skin/icons/information-32.png and I'm sure there's an equivalent for the other themes.

Comment 53

[Michael Kohler [:mkohler]]

Attachment: difference of the icons (16px vs 32px)

> (In reply to comment #51)
> > Right. And no, AFAICS there is no 32x32px version of it.
> 
> For Strata, that'd be chrome://global/skin/icons/information-32.png and I'm
> sure there's an equivalent for the other themes.

The 32x32px version doesn't look good, does it? I'd prefer the small version.

Comment 54

[Alex Faaborg [:faaborg] (Firefox UX)]

>The 32x32px version doesn't look good, does it? I'd prefer the small version.

Yep, let's go with chrome://global/skin/icons/information-16.png

Comment 55

[Alex Faaborg [:faaborg] (Firefox UX)]

> Any reason this checkbox defaults to checked?

This default state is based on the assumption that an add-on is most likely causing all of the instability (and probably not a toolbar customization, or a user preference, since we theoretically test against all of those combinations).  If that doesn't seem right we can also check one or both of the other options.

Comment 56

[Alex Faaborg [:faaborg] (Firefox UX)]

A few small changes to the text:

icon: chrome://global/skin/icons/error-48.png
(use 64 on OS X)

title: Firefox had to Perform a Temporary Factory Reset 

subtitle: Firefox is closing unexpectedly.  As a precaution your custom settings, themes and extensions have been temporarily disabled.

reasons for the changes:
-the yellow warning icon is meant to indicate that something you do could result in causing an error in the future, the red error icon indicates that something bad is happening right now

-put "temporary" in the title since disabling the user's add-ons is serious business :)

-switched to present tense, since after this many crashes there seems to be an increased likelyhood of the user seeing the screen again until they permanently disable whatever is causing all of the crashes.

-the term "safe mode" seems to be unnecessary Firefox specific jargon, so it is better to draw an analogy to what it basically is, a "factory reset" (although I'm not sure how well that will localize).

-the apology we used to have on about:session restore wasn't working, it turns out that if you say you are sincere, people think you aren't.  Also the notion of apology tends to be really culturally dependent.

Comment 57

[Alex Faaborg [:faaborg] (Firefox UX)]

>* Alex Faaborg for an official ui-r+ on this

I need to run the changes by a few people first, usually we can't ui-review the changes that we proposed ourselves, so you might end up getting a ui-r from beltzner or mconnor, or I'll grant it after they have confirmed.

Comment 58

[Michael Kohler [:mkohler]]

Attachment: SR: patch v0.5

* changed the strings and the icon

r+=zeniko (without the recent string changes)
r?=dao, please review these string and icon changes too

Comment 59

[Benjamin Smedberg]

Where do you remove startSM.tmp? It seems like you should remove it as soon as you've checked for it.

Comment 60

[Michael Kohler [:mkohler]]

(In reply to comment #59)
> Where do you remove startSM.tmp? It seems like you should remove it as soon as
> you've checked for it.

I remove it in nsBrowserGlue.js since I still need the file there to avoid the dialog that shows up when we start in Safe Mode.

>   _onProfileStartup: function() 
>   {
>     this._sanitizer.onStartup();
>     // check if we're in safe mode
>     var app = Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULAppInfo).
>               QueryInterface(Ci.nsIXULRuntime);
>     if (app.inSafeMode) {
>-      var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>-               getService(Ci.nsIWindowWatcher);
>-      ww.openWindow(null, "chrome://browser/content/safeMode.xul", 
>-                    "_blank", "chrome,centerscreen,modal,resizable=no", null);
>+      // don't display the safe mode dialog if Firefox has automatically
>+      // restarted in safe mode
>+      var smFile = Cc["@mozilla.org/file/directory_service;1"].
>+                   getService(Ci.nsIProperties).
>+                   get("ProfD", Ci.nsIFile);
>+      smFile.append("startSM.tmp");
>+      if (!smFile.exists()) {
>+        var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>+                 getService(Ci.nsIWindowWatcher);
>+        ww.openWindow(null, "chrome://browser/content/safeMode.xul", 
>+                      "_blank", "chrome,centerscreen,modal,resizable=no", null);
>+      }
>+      else
>+        smFile.remove(false);

Comment 61

[Dão Gottwald [:dao]]

Comment on attachment 415716 [details] [diff] [review]
SR: patch v0.5

>+<!ENTITY aboutsafemode.errorTitle     "&brandShortName; had to Perform a Temporary Factory Reset ">

trailing space

>+<!ENTITY aboutsafemode.problemDesc    "Firefox is closing unexpectedly.  As a precaution your custom ssettings, themes and extensions have been temporarily disabled.">

And JIT, which I think is a mistake. See bug 478846.
Besides, you probably want to use &brandShortName; instead of Firefox and "settings" instead of "ssettings".

>+<!ENTITY aboutsafemode.resetToolbar   "Reset toolbars and controls">

I don't understand how this could be related to Firefox having closed unexpectedly.

Comment 62

[Dão Gottwald [:dao]]

(In reply to comment #56)
> title: Firefox had to Perform a Temporary Factory Reset

Should "Perform" be "perform", i.e. start lowercase?

Comment 63

[Dão Gottwald [:dao]]

Comment on attachment 415716 [details] [diff] [review]
SR: patch v0.5

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js

>+  // start in Safe Mode when there were two crashes more than specified in
>+  // browser.sessionstore.max_resumed_crashes (that means that the session restore page
>+  // was already displayed once!)

I don't think this belongs in browser.js. Maybe nsSessionStartup.js or nsBrowserGlue.js?

>+#errorPageContainer[safemode] #errorTitleText,
>+#errorPageContainer[safemode] #errorLongDescText,
>+#errorPageContainer[safemode] #errorShortDescText,
>+#errorPageContainer:not([safemode]) #errorTitleSafeModeText,
>+#errorPageContainer:not([safemode]) #safemodeShortDescText,
>+#errorPageContainer:not([safemode]) #safemodeLongDescText,
>+#errorPageContainer:not([restartneeded]) #restartNeededDesc {
>+  display: none;
>+}

This belongs somewhere in browser/components/sessionstore/content/, not browser/themes/.

Comment 64

[Reed Loden [:reed]]

(In reply to comment #62)
> (In reply to comment #56)
> > title: Firefox had to Perform a Temporary Factory Reset
> 
> Should "Perform" be "perform", i.e. start lowercase?

Indeed.

Comment 65

[Reed Loden [:reed]]

Comment on attachment 415716 [details] [diff] [review]
SR: patch v0.5

>+function restartFirefox() {
...
>+          <hbox xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" id="restartFirefox">
>+            <button id="restartButton" label="&aboutsafemode.restartButton;"
>+                    accesskey="&aboutsafemode.restartButton.access;"
>+                    oncommand="restartFirefox();"/>
>+          </hbox>

Please don't use "Firefox" in the function name. Use something generic like restartBrowser().

>+<!ENTITY aboutsafemode.problemDesc    "Firefox is closing unexpectedly.  As a precaution your custom ssettings, themes and extensions have been temporarily disabled.">

Besides correcting "ssettings", please also add a comma after "themes". The double space is probably not needed, either.

>+<!ENTITY aboutsafemode.restartNeeded  "Firefox will need to restart to apply your permanent changes.">
>+<!ENTITY aboutsafemode.restartButton  "Restart Firefox">

All three of the above strings need to use &brandShortName; (dao mentioned it for the first one, but I'm just pointing out the other two).

Comment 66

[Benjamin Smedberg]

It really feels like users could end up permanently stuck in safe mode if a startSM.tmp file gets stuck in their profile and we (for instance) change the browser code in a future release, or if the file gets restored from backup as read-only. I'd be more comfortable with a system where the safe-mode file was deleted immediately and we set a flag indicating that.

Comment 67

[Alex Faaborg [:faaborg] (Firefox UX)]

[Moving a discussion over here from dev.apps.firefox, we were talking about the ideal threshold time.]

I guess I'm not entirely clear on where the 6 hour threshold came from
to begin with. The original logic was:

1) Always automatically restore (asking the user "hey, did you want that
thing you were currently using" is a kind of a silly question)
2) Except: in the event that Firefox crashes a second time as we are
automatically restoring, and it looks like we are heading towards an
infinite loop of attempting to reopen, display about:sessionrestore instead
3) And if things really go wrong: in the event that Firefox still
crashes a third or fourth time on launch and we aren't even restoring
the session (and it still looks like we are still heading towards in
infinite loop of attempting to reopen) try launching in safe mode so we
at least are able to open.

Between all of these events, I think we should be looking at a threshold
of ~5 minutes instead of 6 hours. The threshold should be about making
sure we had enough time to open successfully.

Comment 68

[Alex Faaborg [:faaborg] (Firefox UX)]

Michael Kohler in dev.apps.firefox:

>What would happen is the following:
>Firefox crashes once -> the user clicks on "Restart Firefox" in the Crash
>Reporter -> we automatically restore the session -> it crashes again -> user is
>annoyed and clicks "Quit" and waits 2 hours and then reopens firefox again

>If the threshold was ~5 minutes, Firefox would now try to automatically restore
>the session again and we would be at 1) once again. This is IMHO not at all what
>we want since we want as less steps the user has to take as possible. Alex, do
>you agree?

Comment 69

[Alex Faaborg [:faaborg] (Firefox UX)]

To solve the problem that Michael mentions in comment #68, could we modify the threshold to not count time when Firefox is not actively running.  So something along these lines of recording when Firefox has been operational for more than 5 minutes, in addition to counting the number of crashes.

Comment 70

[Michael Kohler [:mkohler]]

(In reply to comment #66)
> It really feels like users could end up permanently stuck in safe mode if a
> startSM.tmp file gets stuck in their profile and we (for instance) change the
> browser code in a future release, or if the file gets restored from backup as
> read-only. I'd be more comfortable with a system where the safe-mode file was
> deleted immediately and we set a flag indicating that.

How can I do that? (sorry, I've never done this before and I don't know C++ that good)

(In reply to comment #69)
> To solve the problem that Michael mentions in comment #68, could we modify the
> threshold to not count time when Firefox is not actively running.  So something
> along these lines of recording when Firefox has been operational for more than
> 5 minutes, in addition to counting the number of crashes.

Sounds good for a follow up.

Comment 71

[chris hofmann]

there is some interesting discussion related to this bug and an addon that is trying out some of these ideas over in Bug 545904