724821 - Firefox menu bar becomes disabled after toggling toolbars while "Customize Toolbars" dialog open

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Ginn Chen]

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]]

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]]

(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]]

Attachment: 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] (inactive)]

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]]

Attachment: 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] (inactive)]

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]]

(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]]

Attachment: 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]]

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]]

http://hg.mozilla.org/mozilla-central/rev/f4981b5e1f7a

Comment 11

[Gary [:streetwolf52]]

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 [:streetwolf52]]

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 [:streetwolf52]]

Seems that the following pref along with this fix is causing my crashes:

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