233137 - Menu Bar disappears after restoring default toolbar set

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Charles Fenwick]

Current build from CVS on Suse 9.0 with gtk2+xft

Regression from bug 197427

Steps to reproduce

1. From View>>Toolbars>>Customize, add something to the toolbar.
2. Decide that you don't like the addition and click 'Restore Default Set' 
3. Say 'Dude, where's my menu bar?'
4. Close out of Firebird.
5. Run Firebird
6. Note that the menu bar is now aligned right, with the throbber to its left.
7. Go to Customize Toolbar menu and 'Restore Default Set', which restores
everything to normal.

Note: Last step varies... sometimes that causes the menu bar to disappear again,
other times nothing happens, in which case adding something to the task bar and
restoring the default returns everything to normal.

Backing out the changes from bug 197427 made this problem disappear.

Comment 1

[Alex Vincent [:WeirdAl]]

The fix for bug 197427 was for broken XUL DOM code.  It's fairly likely that the Firebird 
front-end was relying on that broken code. 
 
Can you give us a URL in LXR, perhaps, for the affected chrome?  (I've never hacked 
Firebird, so this is unfamiliar turf for me.) 

Comment 2

[Boris Zbarsky [:bzbarsky]]

Are there any errors in the JS console?  Like say NS_ERROR_DOM_NOT_FOUND_ERR?

Comment 3

[Charles Fenwick]

First off, I apologize for the tone of my report.  I didn't intend for it to
come off the way that it does.  For example, I only mentioned the fact that
backing out the update made the problem disappear to emphasize that this was not
a pre-existing condition; not to say "Your checkin broke this, pull it out."  I
had a sense of what WeirdAl states in his comment, but it just didn't come
through at 5 in the morning...

...anyhow, on to the questions...
1.  The relevant JS is at
http://lxr.mozilla.org/mozilla/source/toolkit/content/customizeToolbar.js#550

2.  Bz, the JS console doesn't show anything

Comment 4

[Boris Zbarsky [:bzbarsky]]

Charles, could you possibly test which of the three functions involved causes
the problem?  That is, revert the bug 197427 changes for nsXULElement.cpp (that
should make this problem go away).  Then apply those changes to one function at
a time (just need to build in content/xul/content/src and layout/build)?  I'm
guessing the problem is with InsertBefore, but I don't have an FB build to test
in....

Comment 5

[Charles Fenwick]

Boris,

Am working on it now.  As you expected, reverting to previous version of
nsXULElement.cpp made the problem go way.

Comment 6

[Charles Fenwick]

After a veritable round-robin with the functions, it does seem that InsertBefore
is the problem

Comment 7

[Boris Zbarsky [:bzbarsky]]

So I see a few differences in InsertBefore:

1)  The XUL version used to screw up if both nodes were kids of the same node
2)  The XUL version didn't reparent content wrappers
3)  The XUL version didn't handle document fragments

#3 should be a non-issue here.  I dunno what the impact of #2 is (probably none
in this case).  #1 worries me -- could some of this code have actually relied on
the XUL InsertBefore silently failing?

Someone needs to actually sit down with DOM inspector and look at how the DOM
created differs in the failure and non-failure cases...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Patches in bug 233191 should fix this....

Comment 9

[Charles Fenwick]

Pulled and built to incoroporate JST's patch in bug 233191.  Unlike BC (comment
9), I still saw this bug.

Per bz's request over IRC, built with his patch to nsBindingManager that's
attached to that bug.  Still no joy.

Comment 10

[Alex Vincent [:WeirdAl]]

I believe DOM Inspector does work on Firebird; I don't know if JS Debugger does.  Either of 
them would be invaluable tools to figure out exactly what's going on (the current DOM at 
each step and line executed, that is). 
 
I mention this because I don't see an insertBefore call in that function, though maybe another 
function which restoreDefaultSet() calls it. 
 
wrapToolbarItems() does (line 201).  wrapPaletteItem does (line 268).  Some funky toolbar 
observer does (line 790).  If you have Venkman, breakpointing these lines would probably 
help. 

Comment 11

[Charles Fenwick]

Attachment: screenshot

Situation normal

Comment 12

[Charles Fenwick]

Attachment: screenshot2

A customization is made

Comment 13

[Charles Fenwick]

Attachment: screenshot3

restore default

Comment 14

[Charles Fenwick]

#mozilla

<bz> clfenwi: could you try changing nothing but appendChild (1-liner)
<bz> clfenwi: and seeing what that does?

When I bring up the customize window, I get the situation displayed in the
second screenshot (except that no customization has been made).  If I go ahead
and make a customization and restore default (without clicking done)
everything's fine.  If/when I click done, I lose the bookmark toolbar.  I hadn't
noticed that I had been losing the bookmark toolbar until now ($%#!%@
inattention to detail).

Comment 15

[Bob Clary [:bc] (inactive)]

I did a distclean and pulled with the patch from bug 233191 that was checked in
and can confirm it did not fix the bug. Both the menu and tool bar are messed.
But the patch in bug 233191
<http://bugzilla.mozilla.org/attachment.cgi?id=140701&action=view> is not the
one I used. 

I used

Index: content/base/src/nsGenericElement.cpp
===================================================================
RCS file: /cvsroot/mozilla/content/base/src/nsGenericElement.cpp,v
retrieving revision 3.311
diff -u -p -d -u -r3.311 nsGenericElement.cpp
--- content/base/src/nsGenericElement.cpp	5 Feb 2004 06:48:29 -0000	3.311
+++ content/base/src/nsGenericElement.cpp	6 Feb 2004 00:22:49 -0000
@@ -2782,7 +2798,11 @@ nsGenericElement::doInsertBefore(nsICont
     nsContentUtils::ReparentContentWrapper(newContent, aElement,
                                            aElement->GetDocument(), old_doc);
 
-    res = aElement->InsertChildAt(newContent, refPos, PR_TRUE, PR_TRUE);
+    if (refPos == aElement->GetChildCount() && 0) {
+      res = aElement->AppendChildTo(newContent, PR_TRUE, PR_TRUE);
+    } else {
+      res = aElement->InsertChildAt(newContent, refPos, PR_TRUE, PR_TRUE);
+    }
 
     if (NS_FAILED(res)) {
       return res;

Comment 16

[Boris Zbarsky [:bzbarsky]]

Hmm.. bc, is that what I mailed you?  That patch is a no-op, no?  The "else"
branch is always taken.....  Does doing that actually fix this bug?  If so,
something _really_ wacky is up.

Comment 17

[Bob Clary [:bc] (inactive)]

sure looks like an noop but i didn't read it before applying. iirc the menu
sliding over to the right issue was gone but i am not sure that i noticed about
the toolbar.

Comment 18

[Boris Zbarsky [:bzbarsky]]

OK, to get some traction on this, here's what will likely need to happen:

1)  Someone will take a firebird build, rip out the relevant XUL/js, and create a
    testcase that runs standalone (via mozilla -chrome).
2)  Someone (not necessarily the same someone, and this could even be me once
    step 1 is done) will reduce the testcase to a manageable size.

That will give us a better idea of what we're looking at.

Comment 19

[Kastus]

(In reply to comment #18)
> OK, to get some traction on this, here's what will likely need to happen:
> 
> 1)  Someone will take a firebird build, rip out the relevant XUL/js, and create a
>     testcase that runs standalone (via mozilla -chrome).
> 2)  Someone (not necessarily the same someone, and this could even be me once
>     step 1 is done) will reduce the testcase to a manageable size.
> 
> That will give us a better idea of what we're looking at.

Maybe not exactly the testcase you want, but here is how I reproduce it:

Checkout from CVS
Build with the following configuration on SuSE 9.0:
      --enable-crypto \
      --disable-tests \
      --disable-debug \
      --disable-mailnews \
      --disable-composer \
      --enable-optimize="$CFLAGS" \
      --disable-ldap \
     
--enable-extensions=cookie,xml-rpc,xmlextras,pref,transformiix,universalchardet,typeaheadfind,webservices,inspector
\
      --disable-profilesharing \
      --enable-plaintext-editor-only \
      --with-system-jpeg=/usr \
      --with-system-zlib=/usr \
      --with-system-png=/usr \
      --enable-default-toolkit=gtk2 \
      --disable-xinerama \
      --disable-xft \
      --enable-freetype2 \
      --enable-strip \
      --enable-reorder \
      --disable-installer \
      --disable-shared \
      --enable-static

Menu bar dissapears after any customization

If I make any changes Tools>>Options then toolbar stops functioning.
Deleting .phoenix directory restores everything to normality until next attempt
to customize anything

Comment 20

[Boris Zbarsky [:bzbarsky]]

Don't you think that if I could build it I _would_ have by now?  ;)

The other point is that with a standalone testcase reduction is feasible.  With
the full firefox it's not (things will quickly break to the point where you
won't be able to even get to this dialog, and any little change will require
re-exporting chrome, etc).

Comment 21

[Johnny Stenback (:jst)]

Attachment: Don't reparent XUL element wrappers

I don't know quite what the deal is here, but something non-obvious breaks when
we try to re-parent a XUL element (i.e. when it's being moved from one document
to another, or one scope to another, really). This may be related to the fact
that the DOM object wrapping code in nsDOMClassInfo relies on the nodes
nodeinfo pointing to the nodes document (or no document) in all cases, and
that's not true for XUL elements, their nodeinfo sometimes comes from the
prototype, sometimes from the document where they were created (which may be a
different document than the one they're in right now).

Or this could be something totally different, I don't know.

But for now, this gets us back to having functional toolbar customization in
FireFox, and the real fix for this is not necessarily anywhere near, so I'd say
we land this while we're working on the real issue here...

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 142174 [details] [diff] [review]
Don't reparent XUL element wrappers

r=bzbarsky as a total bandaid.	But I have to wonder... Does porting the code
at
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsGenericElement.cpp#1
735 to nsXULElement allow us to usefully reparent the content wrapper?

Comment 23

[Johnny Stenback (:jst)]

I tried that, but it didn't work, and there's more to this with nsXULElement's
nodeinfo sometimes coming from its prototype n' all that. This doesn't seem easy
to fix for real.

Comment 24

[Boris Zbarsky [:bzbarsky]]

OK.  It's really not cool to have the nodeinfo pointing to a bogus document, by
the way... :(  We want to try to fix this somehow.  Perhaps not store the
nodeinfo on the prototype?  A lot of places in the code assume the nodeinfo is
correct, and I'm surprised this hasn't ever broken anything before...

Comment 25

[Hussam Al-Tayeb]

OS says linux, but I see this on windows xp too. So somebody might want to
change OS to all.

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 142174 [details] [diff] [review]
Don't reparent XUL element wrappers

dbaron says sr=dbaron

Comment 27

[Johnny Stenback (:jst)]

Fix checked in.