###!!! ASSERTION: Going to destroy a frame we didn't remove. Prepare to crash: 'removed', file /Users/jruderman/central/layout/xul/base/src/nsListBoxBodyFrame.cpp, line 1497 Null-deref crash [@ nsStackLayout::Layout]

I have another testcase that triggers this assertion followed by a call to 0xdddddddd...

Latest nightly still crashes. http://hg.mozilla.org/mozilla-central/rev/1886b176f000 doesn't crash. http://hg.mozilla.org/mozilla-central/rev/fec668a58714 crashes. Most likely caused by bug 432068.

If the next content isn't a list item but has a frame then we end up returning something other than a list item. I haven't looked into this too deeply, but this seemed like the logical thing to fix it. This fixes the crash for me, but I still get WARNING: ENSURE_TRUE(listbox) failed: file /home/tim/ffapply/src/layout/xul/base/src/nsListBoxBodyFrame.cpp, line 779 WARNING: ENSURE_TRUE(listboxContent) failed: file /home/tim/ffapply/src/layout/xul/base/src/nsListBoxBodyFrame.cpp, line 1447 because of the appended listboxbody with no corresponding listbox, I would assume.

Got a better idea of what is going on here now. Before bug 432068 landed we would create two frames for any non-listitem content inside a listbox. One would be in the nsListBoxBodyFrame's mFrames nsFrameList because it was created by nsListBoxyBodyFrame. The other was created in the normal fashion and would not be in mFrames. After bug 432068 landed we would detect if a non-listitem inside a listbox had a frame and reuse that one without it being in mFrames and this caused problems. So if the content already has a frame, and the content is not a listitem just skip over it. Does the recursion here have the potential to overflow the stack?

> The other was created in the normal fashion Where, exactly? This part confuses me.... Is the check on content tag really the right one? What's the parent frame of that frame we get back from GetPrimaryFrameFor? ccing some folks who might know something about this code. I really wish we could just rip it all out already.

(In reply to comment #5) > > The other was created in the normal fashion > > Where, exactly? This part confuses me.... nsListBoxBodyFrame does lazy construction of the frames for its listitems. In nsCSSFrameConstructor::ContentInserted/Appended/Removed we have special checks (NotifyListBoxBody in ContentInserted/Removed and MaybeGetListBoxBodyFrame in ContentAppended) for child content with tag listitem and parent content with tag listbox. If we get that combination we short circuit the usual frame construction work and just call nsListBoxBodyFrame::OnContentInserted/Removed. If we have a parent with tag listbox but the child is not of tag listitem we don't follow this path and create the frame as normal. > Is the check on content tag really the right one? GetListItemContentAt, GetListItemNextSibling, GetIndexOfItem, GetItemAtIndex, ComputeIntrinsicWidth, and ComputeTotalRowCount all do a similar thing. And nsCSSFrameConstuctor first checks the new child's content tag before calling nsListBoxBodyFrame::OnContentInserted/Removed. Hmm, your next question prompted me to try checking if existingFrame's parent isn't |this|. This works too. > What's the parent frame of that frame we get back from GetPrimaryFrameFor? The parent of the existingFrame is a box frame based on listbox content, it is an ancestor of the listboxbody frame. > I really wish we could just rip it all out already. I've had that same thought.

I thought we'd already decided to rip out the listbox dynamic frame creation stuff.

> for child content with tag listitem Ah, I'd forgotten this part. And the code moved on m-c, so I didn't see it. OK, yeah. > And nsCSSFrameConstuctor first checks the new child's content tag And node type, note. We need to check both here. > The parent of the existingFrame is a box frame based on listbox content OK. I think just checking the tag + namespace should be fine here. Please move that to before the existingFrame get, though, and don't check existingFrame in that conditional: we shouldn't be returning non-listitem stuff here, right? > I thought we'd already decided to rip out the listbox dynamic frame creation We had. But I haven't done it yet, and we need to fix this bug on all the branches too. :(

Maybe Timothy could take an axe to it :-)

Made requested changes. I also added an assertion for the parent thing.

Comment on attachment 384550 [details] [diff] [review] patch Looks great. Let's get this landed on trunk ASAP (I can push tomorrow if it hasn't happened before then) and then see about branches. Timothy, if you do want to work on removing this lazy frame stuff, that would be really nice!

(In reply to comment #11) > Timothy, if you do want to work on removing this lazy frame stuff, that would > be really nice! I can add it to my list after everything else. But I don't plan on looking at it any time soon, so feel free to go ahead with it.

Added Jesse's testcase as a crashtest.

Pushed http://hg.mozilla.org/mozilla-central/rev/4eb507b977b5

Comment on attachment 384586 [details] [diff] [review] patch with test Other than needing a merge on crashtest.list, this applies to both branches. We should land this for 1.9.0.13 and 1.9.1.1...

Comment on attachment 384586 [details] [diff] [review] patch with test Approved for 1.9.0.13, a=dveditz for release-drivers

RCS file: /cvsroot/mozilla/layout/xul/base/src/crashtests/488210-1.xhtml,v done Checking in layout/xul/base/src/crashtests/488210-1.xhtml; /cvsroot/mozilla/layout/xul/base/src/crashtests/488210-1.xhtml,v <-- 488210-1.xhtml initial revision: 1.1 done Checking in layout/xul/base/src/crashtests/crashtests.list; /cvsroot/mozilla/layout/xul/base/src/crashtests/crashtests.list,v <-- crashtests.list new revision: 1.28; previous revision: 1.27 done Checking in layout/xul/base/src/nsListBoxBodyFrame.cpp; /cvsroot/mozilla/layout/xul/base/src/nsListBoxBodyFrame.cpp,v <-- nsListBoxBodyFrame.cpp new revision: 1.102; previous revision: 1.101 done

Comment on attachment 384586 [details] [diff] [review] patch with test a=beltzner, please land on mozilla-1.9.1

Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f899bbaf3487

I don't see the assertion or crash from comment 0 in 1.9.0? Is it trunk or 1.9.1 only?

I don't think I ever tried it in 1.9.0 or 1.9.1.

I was not able to reproduce a crash on any platform with the testcase in commetn #0 using 3.5. If anyone has way to verify this for 3.5.2, it would be greatly appreciated.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 I've tried the test case in comment 0 on all the above builds (Windows and Linux include for good measure...). I tried loading the test case 50 times on each platform. Not once did Firefox crash. Verified1.9.1