547623 - about:support should have a button to enter safemode

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Safemode is often one of the first steps in troubleshooting a major issue. It can also be difficult/confusing for some people to find. Therefore, it'd be great to be able to enter safemode directly from the about:support page.

Comment 1

[Dave Garrett]

This would be the perfect replacement for removing the safe mode start menu item (bug 542122 & bug 598779) as you'd be adding a button right where you'd consider needing it.

(moving to Toolkit, where about:support lives)

Comment 2

[[not reading bugmail]]

I think we might want to go with consistent strings on on this (see bug 542122).

Comment 3

[shreyas]

Hi, I would like to work on this. How should I proceed?

Thanks in advance
shreyas

Comment 4

[Drew Willcoxon :adw]

Hi again shreyas, thanks for volunteering.

about:support is implemented here:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.xhtml
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/aboutSupport.js

You'll need to add the button to the XHTML and then restart in safe mode when it's clicked in the JS.

There's already a "refresh" button in the top right corner of the page.  (It's only visible when using your default Firefox profile.)  I think we should add a safe mode button to the same box.  I played around with adding it to the right of the refresh button, but there just isn't enough horizontal space.  So I think we should put it under the refresh button, and make it all look like this:

  Give Firefox a tune up  [Refresh Firefox...]
           Try safe mode  [Restart with Add-ons Disabled...]

The text and buttons should align like that, so you may need to use a <table> or fancy CSS.  The refresh part would still only appear when ResetProfile.resetSupported() is true, but the safe mode part would always be there.

You'll need to add those two new strings to the localization file.  Both should live here: http://mxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/aboutSupport.dtd  You can see how aboutSupport.xhtml references the strings in that file by their entity names.  You can copy and paste this existing "Restart" string from here: http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd?rev=6be5d432b1b6#26

Then you'll need to add code to aboutSupport.js to restart in safe mode when the button is clicked.  You can see how other buttons there are hooked up.  We already have code to restart in safe mode, when you select Help > Restart with Add-ons Disabled.  So we can use that.  But it's not easy to access from about:support, so you'll need to move it.  It's here currently: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=5dcd284d63af#7712

A tricky part is that about:support lives in toolkit, so every toolkit-based app can use it -- like Thunderbird and Seamonkey.  But safeModeRestart() in browser.js lives in browser, which is Firefox-only.

So I think we should probably broadcast some new notification when the button is clicked to let the front-end know about it, and then the front-end can handle it.  You can broadcast the notification using Services.obs.notifyObservers().  Services.obs implements this interface: http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIObserverService.idl#55  And you can search mxr to see how other code uses it: http://mxr.mozilla.org/mozilla-central/search?string=Services.obs.notifyObservers

Now in Firefox, we want to handle that notification by doing what safeModeRestart() in browser.js already does.  Somewhere we need to observe the notification and then do what safeModeRestart() does.  browser.js is loaded in every browser window, so that's not the right place to observe it.  You can use nsBrowserGlue.js instead, which is a global singleton.  It already implements the nsIObserver method observe(), so you can handle your new observer topic there: http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js?rev=a8bbf40c80b6#258  You'll need to call addObserver() first.  Search nsBrowserGlue.js to find where it calls addObserver() a bunch of times.

Then safeModeRestart() in browser.js should be updated to broadcast the new notification, just like your new safe mode button does.

For some toolkit apps, it may be the case that there are no observers of your new notification.  That will certainly be true when your patch lands, because only Firefox will handle it.  So, before your new button broadcasts the notification, it should use Services.obs.enumerateObservers() to check if anyone is listening.  If not, then you should do this part of safeModeRestart() directly in aboutSupport.js: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=5dcd284d63af#7732

Good luck, and let me know when you have questions.

Comment 5

[shreyas]

I am trying to create a table. So directly creating a table just creates it inside the box. I am trying if I could replace the box with a table like all the others on the about:Support while retaining the box features. I looked up aboutSupport.css but couldn't find the reset-box code. Can you please suggest how should I move forward?

(the style editor in firefox shows additional pieces of code in aboutSupport.css)

Comment 6

[shreyas]

I added the safeModeRestart() to aboutSupport.js but it gives "gNavigatorBundle not defined" error. How should I proceed now?

Comment 7

[Drew Willcoxon :adw]

The relevant CSS is here: http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/aboutSupport.css  There's one other aboutSupport.css in the codebase, and I know it's hard to tell which one is the one actually used in aboutSupport.xhtml.  The other one lives in the mobile/ directory, which means it's only used in Firefox for Android, so it's not the relevant file in this case.

You shouldn't be using gNavigatorBundle inside aboutSupport.js.  The only part of safeModeRestart() that you need inside aboutSupport.js is the part from line 7732 and on, that I linked to above.

But inside nsBrowserGlue.js, where I suggested that you move safeModeRestart(), you will need the object, a string bundle, that gNavigatorBundle refers to.  You can see where browser.js defines gNavigatorBundle here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js?rev=5dcd284d63af#63  It's referencing a DOM element whose ID is "bundle_browser".  You can find that element by searching mxr for it: http://mxr.mozilla.org/mozilla-central/search?string=bundle_browser

You can see it's a <stringbundle>, which is a XUL element that encapsulates this thing called a string bundle, which allows you to reference UI strings in the user's language.  There is no DOM inside nsBrowserGlue.js, so instead of using a XUL <stringbundle> to get the string bundle, you can use the string bundle JS API.

Search in nsBrowserGlue.js for "Services.strings.createBundle" to see how you can create a string bundle.  You pass a URL to createBundle(), and in this case you want to pass the same URL that the XUL <stringbundle> is using.  The thing that createBundle() returns is an object that implements this interface: http://mxr.mozilla.org/mozilla-central/source/intl/strres/nsIStringBundle.idl#29

Once you have the string bundle created by createBundle(), then you can use that in place of gNavigatorBundle.

Let me know when you have questions.

Comment 8

[shreyas]

Attachment: bug_547623.diff

It seems to be working.

Comment 9

[Drew Willcoxon :adw]

Comment on attachment 8562786 [details] [diff] [review]
bug_547623.diff

Review of attachment 8562786 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks shreyas, this is a good start.  Please see my comments below and post a new patch.

You need to replace the body of safeModeRestart() in browser.js with a broadcast of the restart-in-safe-mode notification.

::: browser/components/nsBrowserGlue.js
@@ +515,5 @@
>      os.addObserver(this, "keyword-search", false);
>  #endif
>      os.addObserver(this, "browser-search-engine-modified", false);
>      os.addObserver(this, "browser-search-service", false);
> +    os.addObserver(this, "restart-in-safe-mode", true);

The last argument should be false like all the other addObserver calls.

@@ +2765,5 @@
>    UITour.onPageEvent(aMessage, aMessage.data);
>  });
> +
> +// Prompt user to restart the browser in safe mode
> +   _safeModeRestart: function BG_safeModeRestart() {

Please use multiples-of-two-spaces indentation like all the code in this file, and please indent the method comment two spaces.

::: mobile/android/themes/core/aboutSupport.css
@@ +92,5 @@
>  }
>  
>  #reset-box {
>    display: none;
>  }

Please revert this change.

::: toolkit/content/aboutSupport.js
@@ +11,5 @@
>  Cu.import("resource://gre/modules/Troubleshoot.jsm");
>  Cu.import("resource://gre/modules/ResetProfile.jsm");
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
>                                    "resource://gre/modules/PluralForm.jsm");

Please revert this change.

@@ +689,5 @@
>  }
>  
> +// Prompt user to restart the browser in safe mode
> +function safeModeRestart()
> +{ 

Please use the same bracing style used in this file, meaning:

function safeModeRestart() {

@@ +720,5 @@
>      openProfileDirectory();
>    });
> +  $("restart-in-safe-mode-button").addEventListener("click", function (event) {
> +    Services.obs.notifyObservers(null, "restart-in-safe-mode", true);
> +    safeModeRestart();

This isn't doing what I asked.  It needs to use Services.obs.enumerateObservers() to check if anyone is listening for the restart-in-safe-mode notification.  If so, then it broadcasts that notification and that's all.  If not, then it calls safeModeRestart.

::: toolkit/content/aboutSupport.xhtml
@@ +29,5 @@
>  
>    <body dir="&locale.dir;">
>  
> +    <div id = "reset-box">
> +    <table>

On second thought, let's make this simpler and just use two divs instead of a table.  There really isn't enough horizontal room for what I first suggested.  Like this:

<div id="action-box">
  <div id="reset-box">
    (inside here is the current reset-box)
  </div>
  <div id="safe-mode-box">
    (inside here is your new safe mode <h3> and button
  </div>
</div>

You can't call the outer box "reset-box" like you're doing because reset-box is hidden when the reset profile feature is disabled.  We want to show the safe mode button even when reset profile is disabled.

::: toolkit/locales/en-US/chrome/global/aboutSupport.dtd
@@ +98,5 @@
>  
>  <!ENTITY aboutSupport.sandboxTitle "Sandbox">
> +
> +<!ENTITY aboutSupport.restart.title "Try Safe Mode">
> +<!ENTITY aboutSupport.restart.label "Restart Add-ons with disabled...">

This isn't the right wording ("Add-ons" and "with" are transposed), and the ellipsis needs to be the actual ellipsis character instead of three dot characters.  Please just copy and paste this string exactly: http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/baseMenuOverlay.dtd?rev=6be5d432b1b6#26

Comment 10

[shreyas]

Attachment: bug_547623.diff

Made the necessary changes and posting the updated patch.

Comment 11

[Drew Willcoxon :adw]

Comment on attachment 8564133 [details] [diff] [review]
bug_547623.diff

Review of attachment 8564133 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks shreyas, this looks good but there are still a few things left to fix.  Hopefully the next patch will be good to go.

We want the action-box (and its descendents) to be styled just like the reset-box is styled now.  It should float in the top right corner of the page.  So you need to update aboutSupport.css.  Note that there is this rule, however:

#reset-box {
  visibility: hidden;
}

We still want that because the reset-box should be hidden by default, and then the JS in the page unhides it if appropriate.  So your update should retain that rule.

::: browser/components/nsBrowserGlue.js
@@ +642,5 @@
>      }
>    },
>  
> +    // Prompt user to restart the browser in safe mode
> +  _onsafeModeRestart: function BG__onsafeModeRestart() {

Please capitalize the S in "safe": _onSafeModeRestart

@@ +2793,5 @@
>  let globalMM = Cc["@mozilla.org/globalmessagemanager;1"].getService(Ci.nsIMessageListenerManager);
>  globalMM.addMessageListener("UITour:onPageEvent", function(aMessage) {
>    UITour.onPageEvent(aMessage, aMessage.data);
>  });
> +

Please revert this change.

::: toolkit/content/aboutSupport.js
@@ +689,5 @@
>      $("reset-box").style.visibility = "visible";
>  }
>  
> +// Prompt user to restart the browser in safe mode
> +function safeModeRestart() { 

Please remove the trailing space from this line.

@@ +719,5 @@
>    $("profile-dir-button").addEventListener("click", function (event){
>      openProfileDirectory();
>    });
> +  $("restart-in-safe-mode-button").addEventListener("click", function (event) {
> +    if(Services.obs.enumerateObservers("restart-in-safe-mode")) {

This isn't quite right.  enumerateObservers returns an nsISimpleEnumerator, as you can see here: http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIObserverService.idl#72

An nsISimpleEnumerator is an object that contains some items and lets you enumerate them.  enumerateObservers always returns an enumerator object even if that enumerator doesn't contain any items, so your conditional here will always evaluate to true.

Take a look at the nsISimpleEnumerator interface and see if you can tell what the right conditional should be.  If you need help, let me know.  http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsISimpleEnumerator.idl

Also, please put a space between "if" and the opening parenthesis.

@@ +720,5 @@
>      openProfileDirectory();
>    });
> +  $("restart-in-safe-mode-button").addEventListener("click", function (event) {
> +    if(Services.obs.enumerateObservers("restart-in-safe-mode")) {
> +      Services.obs.notifyObservers(null, "restart-in-safe-mode", true);

The last argument to notifyObservers is a string, not a bool: http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIObserverService.idl#61

So please pass the empty string instead.

::: toolkit/content/aboutSupport.xhtml
@@ +28,5 @@
>    </head>
>  
>    <body dir="&locale.dir;">
> +    <div id="action-box">
> +      <div id = "reset-box">

Please remove the spaces around the equals sign.

::: toolkit/locales/en-US/chrome/global/aboutSupport.dtd
@@ +97,5 @@
>  <!ENTITY aboutSupport.copyRawDataToClipboard.label "Copy raw data to clipboard">
>  
>  <!ENTITY aboutSupport.sandboxTitle "Sandbox">
> +
> +<!ENTITY aboutSupport.restart.title "Try Safe Mode">

Please call this aboutSupport.safeModeTitle

@@ +98,5 @@
>  
>  <!ENTITY aboutSupport.sandboxTitle "Sandbox">
> +
> +<!ENTITY aboutSupport.restart.title "Try Safe Mode">
> +<!ENTITY helpSafeMode.label "Restart with Add-ons Disabled…">

And please call this one aboutSupport.restartInSafeMode.label

Comment 12

[shreyas]

Attachment: bug547623.diff

Updated the patch...

Comment 13

[Drew Willcoxon :adw]

Comment on attachment 8566472 [details] [diff] [review]
bug547623.diff

Review of attachment 8566472 [details] [diff] [review]:
-----------------------------------------------------------------

Almost there!

::: browser/base/content/browser.js
@@ +7637,5 @@
>  });
>  
>  // Prompt user to restart the browser in safe mode
> +function safeModeRestart() {
> +  Services.obs.notifyObservers(null, "restart-in-safe-mode", true);

The last argument here needs to be a string too.

::: browser/components/nsBrowserGlue.js
@@ +516,4 @@
>  #endif
>      os.addObserver(this, "browser-search-engine-modified", false);
>      os.addObserver(this, "browser-search-service", false);
> +    os.addObserver(this, "restart-in-safe-modem", false);

modem? :-)

Also, there's one thing I think I forgot to ask you to do, which is to call removeObserver for this notification in the _dispose method.  You can see that there are a bunch of removeObserver calls there already, one per addObserver call in _init.

I don't see why we actually need to do that since _dispose's comment says it's called on app shutdown, but we should probably just follow precedent.

::: toolkit/content/aboutSupport.js
@@ +690,3 @@
>  }
>  
> +//Prompt user to restart the browser in safe mode

Please put a space between // and Prompt:

// Prompt

@@ +719,5 @@
>    $("profile-dir-button").addEventListener("click", function (event){
>      openProfileDirectory();
>    });
> +  $("restart-in-safe-mode-button").addEventListener("click", function (event) {
> +    if (Services.obs.enumerateObservers("restart-in-safe-mode").hasMoreElements()) {

Right.

@@ +725,5 @@
> +    }
> +    else {
> +      safeModeRestart();
> +    }
> +   });

Please indent this line two spaces, not three.

::: toolkit/content/aboutSupport.xhtml
@@ +29,4 @@
>  
>    <body dir="&locale.dir;">
>  
> +    <div id="action-box"> 

Please remove the trailing space on this line.

@@ -37,2 @@
>      </div>
> -

Please don't make this change.

::: toolkit/themes/windows/global/aboutSupport.css
@@ +99,4 @@
>    float: left;
>  }
>  
> +#action-box > h3 {

This selector isn't right anymore.  > means "child", but the h3's aren't children of #action-box.  If you look at the page, you can see that the safe mode h3 has too much space above it.

The simplest way to fix this would be to write:

#action-box h3 {

@@ +103,4 @@
>    margin-top: 0;
>  }
>  
> +#action-box > button {

Again, same problem here.  The buttons aren't children of #action-box.  Instead you need:

#action-box button {

Comment 14

[shreyas]

Attachment: bug547623.diff

Updated the patch...

Comment 15

[Drew Willcoxon :adw]

Comment on attachment 8566626 [details] [diff] [review]
bug547623.diff

Review of attachment 8566626 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/nsBrowserGlue.js
@@ +644,5 @@
>    },
>  
> +  _onSafeModeRestart: function BG_onSafeModeRestart() {
> +    // prompt the user to confirm
> +    let promptTitle = gNavigatorBundle.getString("safeModeRestartPromptTitle");

This is missing the gNavigatorBundle definition that your previous patches had.  Without it this method doesn't work at all.  Did you test it?  Please put the definition back and instead of calling the variable gNavigatorBundle, please call it `strings`.  Variable names with G prefixes are reserved for global variables.

@@ +653,5 @@
> +                      (Services.prompt.BUTTON_POS_1 *
> +                       Services.prompt.BUTTON_TITLE_CANCEL) +
> +                      Services.prompt.BUTTON_POS_0_DEFAULT;
> +
> +    let rv = Services.prompt.confirmEx(window, promptTitle, promptMessage,

`window` is also undefined here.  Just pass null instead.

::: toolkit/content/aboutSupport.js
@@ +686,4 @@
>   */
>  function populateResetBox() {
>    if (ResetProfile.resetSupported())
> +    $("reset-box").style.display = "block";

There's one more thing I thought of now that I'd like to ask you to do.  If the browser is already in safe mode, then we should hide the Try Safe Mode part.  Fortunately it's easy to check:

Services.appinfo.inSafeMode

But -- if both the reset box and the safe mode box are hidden, then the entire #action-box should be hidden too.  Otherwise it appears as a tiny empty gray box.

What I think we should do is modify the CSS like this:

#action-box,
#reset-box,
#safe-mode-box {
  display: none;
};

And then in populateResetBox (please rename it populateActionBox):

function populateActionBox() {
  if (ResetProfile.resetSupported()) {
    $("reset-box").style.display = "block";
    $("action-box").style.display = "block";
  }
  if (!Services.appinfo.inSafeMode) {
    $("safe-mode-box").style.display = "block";
    $("action-box").style.display = "block";
  }
}

::: toolkit/content/aboutSupport.xhtml
@@ +41,5 @@
> +        <button id="restart-in-safe-mode-button">
> +          &aboutSupport.restartInSafeMode.label;
> +        </button>
> +      </div>
> +    </div>  

Please remove the trailing space from this line.

Comment 16

[shreyas]

Attachment: bug547623.diff

Made the changes, but the styling doesn't seem to be proper under the default profile.

Comment 17

[Drew Willcoxon :adw]

Comment on attachment 8566709 [details] [diff] [review]
bug547623.diff

Review of attachment 8566709 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/themes/windows/global/aboutSupport.css
@@ +101,4 @@
>    float: left;
>  }
>  
> +#action-box h3 {

You're right, thanks for pointing that out.  I think this is the problem.  Let's revert this change and go back to:

#reset-box > h3 {

The #safe-mode-box h3 should have the margin-top.

Is that the problem you're talking about?

Comment 18

[shreyas]

Attachment: bug547623.diff

Done...

Comment 19

[Drew Willcoxon :adw]

Comment on attachment 8566719 [details] [diff] [review]
bug547623.diff

Review of attachment 8566719 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thank you!  All done.  I'll land this when the tree reopens.

Comment 20

[Drew Willcoxon :adw]

Thanks shreyas.  I landed this on the fx-team tree.  Within a day or so someone will come along and then merge your commit to the mozilla-central tree, which Firefox Nightly is built from.  After that, you'll be able to see your change on Nightly.

https://hg.mozilla.org/integration/fx-team/rev/5595892c4c07

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5595892c4c07

Comment 22

[shreyas]

Drew, 

Thanks for mentoring.