346757 - Toolbars not shown in toolbar context menu on first open, assertion 'getElementById(""), fix caller?'

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Adam Guthrie]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060731 Minefield/3.0a1

This only happens the first time you open the context menu, AFAIK.

Steps to reproduce:
1. Right click on the Go button.

Results:
Context menu only has "Customize..." option and is missing the options to toggle on/off the Navigation and Bookmark toolbars.

Expected results:
All items are in the context menu.

Comment 1

[Adam Guthrie]

Regressed between 2006-07-24-04 and 2006-07-25-04.

Checkins: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-07-24+03&maxdate=2006-07-25+05&cvsroot=%2Fcvsroot

Looks like either bug 345560 or bug 241015 (but I suspect the latter one more).

Comment 2

[Boris Zbarsky [:bzbarsky]]

This is in fact a regression from bug 345560, though I'm not sure why.  You can in fact reproduce this behavior in a build from before that change.  Just do:

  javascript:window.open("", "", "toolbar=yes");

then follow the steps for this bug in that window.  We get into updateToolbarStates(), which sees that we have a chromehidden attribute set and starts doing weird stuff.

If I just make updateToolbarStates() return and do nothing, I get the right behavior.

I can change the code in bug 345560 to not set chromehidden to the empty string, but that won't really help in general... Someone who understands what this code is doing should look into why it's being confused by the presence of a chromehidden attr.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Of interest is:

JavaScript error: chrome://browser/content/browser.js, line 1952: document.getElementById(toolbarMenuElt.childNodes[i].getAttribute("observes")) has no properties

where line 1952 over here is:

document.getElementById(toolbarMenuElt.childNodes[i].getAttribute("observes")).removeAttribute("checked");

which matches the following code in browser.js in updateToolbarStates():

2482                             var i;
2483                             for (i = 0; i < toolbarMenuElt.childNodes.length; ++i)
2484                               document.getElementById(toolbarMenuElt.childNodes[i].getAttribute("observes")).removeAttribute("checked");

Oh, and before that there's a nice assert:

###!!! ASSERTION: getElementById(""), fix caller?: '!aId.IsEmpty()', file ../../../../../mozilla/content/xul/document/src/nsXULDocument.cpp, line 1598

So I posit that either someone forgot to put an "observes" attribute on a toolbar, or this code needs to deal with the lack of such an attribute.

Comment 4

[[e-mail]]

(In reply to comment #2)
>We get into updateToolbarStates(), which sees that we have
>a chromehidden attribute set and starts doing weird stuff.
Ah yes, I remember that code; it was blake's answer to bug 75742...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Ah.  So it's just trying to show all the toolbars.... but failing miserably at it?

Comment 6

[[e-mail]]

I think the idea was that it switches the toolbars from "chromehidden"-type hiding to "View>Show/Hide"-type hiding so that that submenu works.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  So what should it do when there is no observes or no element pointed to by it?

Comment 8

[Brian Polidoro]

Windows too
-> ALL

Comment 9

[Jeremy Baron]

Build: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060805 Firefox/3.0a1

I was able to reproduce this on a fresh profile exactly.  Before trying a fresh profile I noticed this behavior, but it required more than one right click for the full list to appear.  I have not been able to reproduce this variation either with the profile where I observed it or a fresh profile.

Comment 10

[Peter van der Woude [:Peter6]]

EC - error on first rightclick on the toolbar

Error: document.getElementById(toolbarMenuElt.childNodes[i].getAttribute("observes")) has no properties
Source file: chrome://browser/content/browser.js
Line: 1911

Comment 11

[Uri Bernstein]

Reproduced on Mac => All/All.

Comment 12

[Jeremy Baron]

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

Comment 13

[Ryan Flint [:rflint] (ping via IRC for reviews)]

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

Comment 14

[Nickolay_Ponomarev]

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

Comment 15

[Nickolay_Ponomarev]

bz, while that code needs fixing, I'm not sure what's the utility of putting an empty chromehidden attribute on the doc element either.

Comment 16

[Nickolay_Ponomarev]

So this code is left-over from a fix for navigator. I don't see how the original fix even worked (it manually hides all toolbars, independent on what chromehidden's value is). bug 75158 has a more sensible patch, but comment 20 there indicates a more low-level fix is needed.

I suggest removing this clearly not working code altogether and use bug 75158 to track the correct fix.

Comment 17

[Boris Zbarsky [:bzbarsky]]

There is not much utility other than having simpler code (not having to check whether the new value is empty, if it is (common case) not having to do a GetAttribute to figure out whether we need to unset the existing attribute (rare case), that sort of thing).

I guess there's the secondary utility that having the code as-is makes it more likely someone will actually fix this bug, which shows up even without the empty chromehidden attribute.

Comment 18

[Nickolay_Ponomarev]

Attachment: patch

Comment 19

[Mats Palmgren (inactive)]

Nickolay, could you please fix SeaMonkey "View -> Show/Hide" also?
(reported in duped bug 349983)

Comment 20

[Nickolay_Ponomarev]

I don't build seamonkey, but can attach a similar (but untested) patch removing the !gHaveUpdatedToolbarState branch in updateToolbarStates. I don't know what the rest of the code there does, so I'd rather not touch it.

Comment 21

[Mats Palmgren (inactive)]

Attachment: SeaMonkey patch

Ok, I did what you suggested, tested it and the assertion is gone...

Comment 22

[Asaf Romano (gone)]

Comment on attachment 241613 [details] [diff] [review]
patch

r=mano.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 241627 [details] [diff] [review]
SeaMonkey patch

I can't review SeaMonkey patches, because I'm not a peer and don't know the code. I'd try Neil.

Comment 24

[[e-mail]]

Comment on attachment 241627 [details] [diff] [review]
SeaMonkey patch

r+sr=me

Comment 25

[Asaf Romano (gone)]

mozilla/browser/base/content/browser-menubar.inc 1.103
mozilla/browser/base/content/browser.js 1.714
mozilla/browser/base/content/browser.xul 1.324
mozilla/suite/browser/navigator.js 1.599