"ASSERTION: Going to destroy a frame we didn't remove. Prepare to crash" with XUL listbox

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: jruderman, Assigned: tnikkel)

Tracking

(Blocks 1 bug, 6 keywords)

Dependency tree / graph
Bug Flags:
blocking1.9.1.1 -
wanted1.9.1.x +
blocking1.9.0.14 +
wanted1.9.0.x +
in-testsuite +

Firefox Tracking Flags

(blocking1.9.1 .2+, status1.9.1 .2-fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Reporter

Description

10 years ago
###!!! 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]
Reporter

Comment 1

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

Updated

10 years ago
Flags: blocking1.9.2?
Assignee

Comment 2

10 years ago
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.
Depends on: 432068
Reporter

Updated

10 years ago
Blocks: 432068
No longer depends on: 432068
Assignee

Comment 3

10 years ago
Posted patch fix? (obsolete) — Splinter Review
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.
Assignee

Comment 4

10 years ago
Posted patch a better fix? (obsolete) — Splinter Review
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?
Attachment #384201 - Attachment is obsolete: true
Attachment #384277 - Flags: superreview?(bzbarsky)
Attachment #384277 - Flags: review?(bzbarsky)
> 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.
Flags: wanted1.9.1.x?
Flags: blocking1.9.0.13?
Assignee

Comment 6

10 years ago
(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 :-)
Assignee

Comment 10

10 years ago
Posted patch patch (obsolete) — Splinter Review
Made requested changes. I also added an assertion for the parent thing.
Assignee: nobody → tnikkel
Attachment #384277 - Attachment is obsolete: true
Attachment #384550 - Flags: superreview?(bzbarsky)
Attachment #384550 - Flags: review?(bzbarsky)
Attachment #384277 - Flags: superreview?(bzbarsky)
Attachment #384277 - Flags: review?(bzbarsky)
Attachment #384550 - Flags: superreview?(bzbarsky)
Attachment #384550 - Flags: superreview+
Attachment #384550 - Flags: review?(bzbarsky)
Attachment #384550 - Flags: review+
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!
Assignee

Comment 12

10 years ago
(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.
Assignee

Comment 13

10 years ago
Added Jesse's testcase as a crashtest.
Attachment #384550 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/4eb507b977b5
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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...
Attachment #384586 - Flags: approval1.9.1?
Attachment #384586 - Flags: approval1.9.0.13?
Flags: wanted1.9.1.x?
Flags: wanted1.9.1.x+
Flags: wanted1.9.0.x+
Flags: blocking1.9.1.1?
Flags: blocking1.9.0.13?
Flags: blocking1.9.0.13+
Comment on attachment 384586 [details] [diff] [review]
patch with test

Approved for 1.9.0.13, a=dveditz for release-drivers
Attachment #384586 - Flags: approval1.9.0.13? → approval1.9.0.13+
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
Keywords: fixed1.9.0.13
blocking1.9.1: --- → .2+
Flags: blocking1.9.1.1? → blocking1.9.1.1-
Comment on attachment 384586 [details] [diff] [review]
patch with test

a=beltzner, please land on mozilla-1.9.1
Attachment #384586 - Flags: approval1.9.1? → approval1.9.1.2+
I don't see the assertion or crash from comment 0 in 1.9.0? Is it trunk or 1.9.1 only?
Assignee

Comment 21

10 years ago
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
Keywords: verified1.9.1
Reporter

Updated

10 years ago
Depends on: 508927
You need to log in before you can comment on or make changes to this bug.