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

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Toolbars and Customization
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ginn Chen, Assigned: raymondlee)

Tracking

unspecified
Firefox 15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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?
(Assignee)

Comment 2

5 years ago
(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.
(Assignee)

Comment 3

5 years ago
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.
Attachment #626887 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #626887 - Flags: review? → review?(dietrich)
Attachment #626887 - Flags: review?(dietrich) → review?(ttaubert)
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.
Attachment #626887 - Flags: review?(ttaubert)
(Assignee)

Comment 5

5 years ago
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.
Assignee: nobody → raymond
Attachment #626887 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #627648 - Flags: review?(ttaubert)
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
Attachment #627648 - Flags: review?(ttaubert)
Attachment #627648 - Flags: review?(mak77)
Attachment #627648 - Flags: feedback+

Updated

5 years ago
Attachment #627648 - Flags: review?(mak77) → review+
(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?
(Assignee)

Comment 8

5 years ago
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
Attachment #627648 - Attachment is obsolete: true
Attachment #628595 - Flags: review?(mak77)
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.
Attachment #628595 - Flags: review?(mak77) → review+
http://hg.mozilla.org/mozilla-central/rev/f4981b5e1f7a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 15

Comment 11

5 years ago
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

5 years ago
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

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

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

Updated

5 years ago
Blocks: 763622
You need to log in before you can comment on or make changes to this bug.