Closed Bug 233137 Opened 20 years ago Closed 20 years ago

Menu Bar disappears after restoring default toolbar set

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox0.9

People

(Reporter: charles.fenwick, Assigned: jst)

References

Details

(Keywords: regression)

Attachments

(4 files)

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.
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.) 
Are there any errors in the JS console?  Like say NS_ERROR_DOM_NOT_FOUND_ERR?
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
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....
Boris,

Am working on it now.  As you expected, reverting to previous version of
nsXULElement.cpp made the problem go way.
After a veritable round-robin with the functions, it does seem that InsertBefore
is the problem
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...
Depends on: 233191
Patches in bug 233191 should fix this....
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.

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. 
Attached image screenshot
Situation normal
Attached image screenshot2
A customization is made
Attached image screenshot3
restore default
#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).
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;
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.
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.
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.
(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
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).
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...
Attachment #142174 - Flags: superreview?(peterv)
Attachment #142174 - Flags: review?(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?
Attachment #142174 - Flags: review?(bzbarsky) → review+
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.
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...
OS says linux, but I see this on windows xp too. So somebody might want to
change OS to all.
Assignee: hyatt → jst
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → Firefox0.9
Comment on attachment 142174 [details] [diff] [review]
Don't reparent XUL element wrappers

dbaron says sr=dbaron
Attachment #142174 - Flags: superreview?(peterv) → superreview+
Fix checked in.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
QA Contact: bugzilla → toolbars
You need to log in before you can comment on or make changes to this bug.