Last Comment Bug 724821 - Firefox menu bar becomes disabled after toggling toolbars while "Customize Toolbars" dialog open
: Firefox menu bar becomes disabled after toggling toolbars while "Customize To...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Toolbars and Customization (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: Firefox 15
Assigned To: Raymond Lee [:raymondlee]
:
: :Gijs Kruitbosch
Mentors:
Depends on:
Blocks: 763622
  Show dependency treegraph
 
Reported: 2012-02-07 03:26 PST by Ginn Chen
Modified: 2012-06-11 12:28 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (833 bytes, patch)
2012-05-24 11:07 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v2 (2.31 KB, patch)
2012-05-28 03:18 PDT, Raymond Lee [:raymondlee]
mak77: review+
ttaubert: feedback+
Details | Diff | Splinter Review
v3 (4.34 KB, patch)
2012-05-30 21:23 PDT, Raymond Lee [:raymondlee]
mak77: review+
Details | Diff | Splinter Review

Description Ginn Chen 2012-02-07 03:26:42 PST
Steps:
1) Click View ->Toolbars -> Customize to open "Customize Toolbar"
2) Right-click menu bar, in the context menu, toggle a toolbar, e.g. add-ons toolbar.
3) Close "Customize Toolbar" dialog
Got
Timestamp: 02/ 7/12 07:19:10 PM
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/search/search.xml ::  :: line 91"  data: no]

4) Click View ->Toolbars -> Customize to open "Customize Toolbar"
Nothing happens.
Got
Timestamp: 02/ 7/12 07:19:15 PM
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIControllers.removeController]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/places/browserPlacesViews.js :: PVB_uninit :: line 669"  data: no]

5) Firefox menubar is grey now.

Firefox 6.0 works fine with above steps.
Firefox 7.0-13.0 all have this problem.
Comment 1 Raymond Lee [:raymondlee] 2012-05-24 01:52:55 PDT
Even though I see the error message mentioned in point 3) but I can reproduce your issue on Firefox 13 Mac and Windows.

Can you create a new profile and try to see whether you can reproduce it?
Comment 2 Raymond Lee [:raymondlee] 2012-05-24 01:54:33 PDT
(In reply to Raymond Lee [:raymondlee] from comment #1)
> Even though I see the error message mentioned in point 3) but I can
> reproduce your issue on Firefox 13 Mac and Windows.
> 
> Can you create a new profile and try to see whether you can reproduce it?

OK, I can reproduce now. I missed some steps.
Comment 3 Raymond Lee [:raymondlee] 2012-05-24 11:07:11 PDT
Created attachment 626887 [details] [diff] [review]
v1

After those steps, controllers.removeController(this._controller) throws an exception which causes the issue. Adding a try and catch block solves that.
Comment 4 Tim Taubert [:ttaubert] 2012-05-25 01:37:55 PDT
Comment on attachment 626887 [details] [diff] [review]
v1

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

How does this fix the error message of point (3)?

I don't like putting a try/catch around this, that would just hide the actual problem. If .removeController() gets called repeatedly we should fix that and maybe introduce a flag that lets us not do it twice.
Comment 5 Raymond Lee [:raymondlee] 2012-05-28 03:18:51 PDT
Created attachment 627648 [details] [diff] [review]
v2

(In reply to Tim Taubert [:ttaubert] from comment #4)
> Comment on attachment 626887 [details] [diff] [review]
> v1
> 
> Review of attachment 626887 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> How does this fix the error message of point (3)?

The error is not related to this bug at all, however, this patch should fix that too.

> 
> I don't like putting a try/catch around this, that would just hide the
> actual problem. If .removeController() gets called repeatedly we should fix
> that and maybe introduce a flag that lets us not do it twice.

Fixed.
Comment 6 Tim Taubert [:ttaubert] 2012-05-29 03:03:35 PDT
Comment on attachment 627648 [details] [diff] [review]
v2

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

(In reply to Raymond Lee [:raymondlee] from comment #5)
> (In reply to Tim Taubert [:ttaubert] from comment #4)
> > How does this fix the error message of point (3)?
> 
> The error is not related to this bug at all, however, this patch should fix
> that too.

Oh, I see, thanks for including it. I'd normally tend to file a separate bug for this but it's a very small fix so I'd be ok with letting it as is.

The patch looks fine to me but I'm not at all familiar with the places toolbar. Asking Marco for additional review.

::: browser/base/content/browser-places.js
@@ +1109,5 @@
>        return;
>  
>      // If the bookmarks toolbar item is hidden because the parent toolbar is
> +    // collapsed or hidden (i.e. in a popup), spare the initialization.  Also,
> +    // there is no needed to initialize the toolbar if customizing because

Nit: there is no need to

@@ +1110,5 @@
>  
>      // If the bookmarks toolbar item is hidden because the parent toolbar is
> +    // collapsed or hidden (i.e. in a popup), spare the initialization.  Also,
> +    // there is no needed to initialize the toolbar if customizing because
> +    // init() would be called when the customization is done.

Nit: will be called
Comment 7 Dão Gottwald [:dao] 2012-05-30 08:58:43 PDT
(In reply to Ginn Chen from comment #0)
> Steps:
> 1) Click View ->Toolbars -> Customize to open "Customize Toolbar"
> 2) Right-click menu bar, in the context menu, toggle a toolbar, e.g. add-ons
> toolbar.
> 3) Close "Customize Toolbar" dialog
> Got
> Timestamp: 02/ 7/12 07:19:10 PM
> Error: uncaught exception: [Exception... "Component returned failure code:
> 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]" 
> nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame ::
> chrome://browser/content/search/search.xml ::  :: line 91"  data: no]

Various toolbar customization tests call ignoreAllUncaughtExceptions. Can they stop doing this?
Comment 8 Raymond Lee [:raymondlee] 2012-05-30 21:23:39 PDT
Created attachment 628595 [details] [diff] [review]
v3

I have removed ignoreAllUncaughtExceptions() in various toolbar customization tests.  Those tests passed in my machine.  Pushed to try and waiting for results https://tbpl.mozilla.org/?tree=Try&rev=5da19af5673f
Comment 9 Marco Bonardo [::mak] 2012-05-31 05:25:33 PDT
Comment on attachment 628595 [details] [diff] [review]
v3

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

I suggest handling removal in other tests in another bug, cause would be quite unrelated.
Very good we can stop ignoring exceptions here.
Comment 10 Dão Gottwald [:dao] 2012-05-31 05:48:28 PDT
http://hg.mozilla.org/mozilla-central/rev/f4981b5e1f7a
Comment 11 Gary [:streetwolf] 2012-05-31 14:42:44 PDT
Seems that this bug fix is causing me to crash as soon as I start to type in the Location Bar.  Probably other ways to crash FX.

Regression checking led me to cset http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-win32/1338468385/ which appears to only contains 724821.

Here's a crash dump:  https://crash-stats.mozilla.com/report/index/bp-4dfdfd7d-ed92-4146-9cb4-5f7812120531
Comment 12 Gary [:streetwolf] 2012-05-31 14:55:58 PDT
More info for you:

A new profile doesn't cause a crash.  Starting with add-ons disabled still causes the crash.  Could it be a Pref that is causing the crash?
Comment 13 Gary [:streetwolf] 2012-05-31 17:08:23 PDT
Seems that the following pref along with this fix is causing my crashes:

user_pref("layers.offmainthreadcomposition.enabled", true);

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