Mike Connor [:mconnor] Reporter
	Description • 17 years ago

https://bugzilla.mozilla.org/attachment.cgi?id=270625 is needed, apply the patch, then toggle the toolbar mode using the context menu a few times (anywhere from 3-7 is the usual case) to crash.  Usually seems to happen when I context-click the toolbar, the menu doesn't seem to appear at all.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 1 • 17 years ago

So I can definitely reproduce (on Mac, at least; haven't tried elsewhere).  I'm crashing in nsCSSFrameConstructor::FindFrameWithContent (called from GetPrimaryFrameFor, called from a style reresolve) because there are dead frames in the frametree.

In particular, aParentFrame in this case is a nsMenuFrame.  aParentContent is a <toolbarbutton>.  We're looking for the primary frame for a <label> which is a child of this <toolbarbutton>.  The <toolbarbutton> is the first child of an <hbox>.

We're looking through the kids of the nsMenuFrame, and in particular, aHint is null and listName is nsGkAtoms::popupList.  The first child of the nsMenuFrame in this list is an nsMenuPopupFrame whose content is a <menupopup> (as expected; this part is fine).  This particular popup does NOT have NS_FRAME_OUT_OF_FLOW set.

The GetNextSibling() of the nsMenuPopupFrame is an nsImageBoxFrame.  The content of the nsImageBoxFrame is an <image> that's a kid of our <toolbarbutton>.

The GetNextSibling() of the nsImageBoxFrame is a destroyed frame, and we crash when we get to this part.

So the question is how we got here... I see some DOM rearrangement happening in onViewToolbarsPopupShowing.  But we've been doing that all along, so that should work.  Neil, any ideas what might be up here?

	Neil Deakin
	Comment 2 • 17 years ago

Looks like it's a frame inside the bookmarks overflow button which is deleted. If there enough items on the toolbar, I can see this button shuffle position or even hide/collapse itself when changing the toolbar button mode. This suggests that whatever is adjusting it, possibly scroll overflow/underflow, is the cause.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 17 years ago

What worries me is that we're ending up with a dead frame still in the frametree.  We need to figure out why that's happening.

I hate asking for a minimal testcase at times like this, but that would help a ton.  Trying to debug the entire UI at once is pretty hellish.  :(

	Neil Deakin
	Comment 4 • 17 years ago

What's happening here is that the structure is a follows (the label might be an image instead depending on whether you are switching to icons only or labels only):

menuframe
  menupopupframe
  label

The menupopupframe has the label as its next sibling, yet the menupopupframe is in the menuframe's popupList so should actually be its first child.

Debugging suggests that the xbl insertion code in nsCSSFrameConstructor::FindPreviousSibling isn't skipping over the frames that are in different lists, so the label gets added as a sibling rather than a child.

	Neil Deakin
	Comment 5 • 17 years ago

(In reply to comment #4)

> The menupopupframe has the label as its next sibling, yet the menupopupframe is
> in the menuframe's popupList so should actually be its first child.

The label should be the first child that is.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 17 years ago

This is an in-flow popup, eh?  <sigh>...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 17 years ago

I don't actually have a way to test this right now; does someone else have a working tree showing this bug?

In any case, I think the patch should fix things for in-flow popups.  Out-of-flow ones are already handled by the placeholder stuff in FindPreviousSibling.

	Neil Deakin
	Comment 8 • 17 years ago

No, it still crashes. It looks like its actually FindPreviousAnonymousSibling that's being called here not FindPreviousSibling. Perhaps a similar change is needed there.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 9 • 17 years ago

Oh, FindPreviousAnonymousSibling.  That doesn't call IsValidSibling() at all, does it?  I suppose we could add IsValidSibling() calls there and hope that not too much breaks.

We really need a better frame construction setup....

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 17 years ago

Maybe we should do a consolidation patch similar to what I have in bug 275076 but pushing IsValidSibling in there too?  I hate the code duplication we have now...

Lemme try that.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 11 • 17 years ago

I meant bug 387227...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 17 years ago

I'd like to land bug 390425 before working on this, since it removes some FindNextSibling/FindPreviousSibling callers which are kinda bogus...

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 17 years ago

This conflicts with bug 390425, but it's a starting point....  Does it fix things?

Note that now we're only doing the valid sibling check on popups if we're inside a menuframe, since out-of-flow popups (which is what you get elsewhere), or rather their placeholders, are in fact valid siblings for in-flow kids.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 17 years ago

We should probably think about fixing this on branches too... and having a minimal testcase, for that matter.  Someone who understands XUL want to create one?

	Martijn Wargers (dead)
	Comment 15 • 17 years ago

This is a testcase that calls the methods in Mike's patch that cause the crash. You need to download it to your computer because fo the use of enhanced privileges.
It crashes for me after 2 times collapsing/uncollapsing the bookmarks toolbar (it's overflowing).
This seems to work just fine on branch without crashing.

	Martijn Wargers (dead)
	Comment 16 • 17 years ago

With the "testcase that calls chrome", I get a regression range between 2007-07-04 and 2007-07-05:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-07-04+03&maxdate=2007-07-05+07&cvsroot=%2Fcvsroot
I guess a regression from bug 279703, somehow.

	Neil Deakin
	Comment 17 • 17 years ago

bz, my fairly limited testing shows that the bug is fixed.

Martijn, this is likely a crash that could also occur on the branch, but bug 279703 exposed a means to do that. 

	Martijn Wargers (dead)
	Comment 18 • 17 years ago

My testcase needs to have the bookmarks on the second toolbar (which is also where the Back/Forward/Reload/Stop buttons are) to see the crash, btw.
I noticed that the crash happens when you have folders in the bookmarks toolbar. 1 empty folder in the bookmarks toolbar is enough to trigger the crash.

	Martijn Wargers (dead)
	Comment 19 • 17 years ago

This is a more or less minimized testcase that triggers the crash.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 20 • 17 years ago

Comment on attachment 276797 [details] [diff] [review]
Patch to test

Let's get this in on trunk, bake it some, then see about branches....

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 23 • 17 years ago

Comment on attachment 283304 [details] [diff] [review]
Merged to tip again

Trying roc for the review

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 24 • 17 years ago

Comment on attachment 283304 [details] [diff] [review]
Merged to tip again

bit of a rubber-stamp, I'm afraid, but you're sharing a bunch of duplicate code which is always good, and the logic makes at least superficial sense.

	Martijn Wargers (dead)
	Comment 25 • 17 years ago

Boris, this bug is blocking1.9+, so if I understand the rules correctly, you can check the patch already in, safely.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 26 • 17 years ago

Ah, indeed.  Once the tree opens, if that ever happens.  ;)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 27 • 17 years ago

And no, this does need approval...

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 28 • 17 years ago

Comment on attachment 283304 [details] [diff] [review]
Merged to tip again

Approved for landing once tree opens for M10 (post-M9 freeze, target milestone is set correctly)

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 29 • 17 years ago

Checked in.  Need tests...

	Martijn Wargers (dead)
	Comment 30 • 17 years ago

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007110805 Minefield/3.0b2pre

	Daniel Veditz [:dveditz]
	Comment 31 • 17 years ago

If we don't have a testcase that shows a problem on the 1.8 branch can we skip this patch then?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 32 • 17 years ago

It's hard to tell.  For one thing, I'm not sure how much of this bug's existence actually depends on the popup changes that landed on trunk.  Neil might know.

	Mats Palmgren (inactive)
	Comment 33 • 10 years ago

Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c52f1476944a

	Carsten Book [:Tomcat]
	Comment 34 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/c52f1476944a