326834 - CVE-2006-1531 [FIX]Crash with evil xul testcase, using listbox/listitem and display: table-cell

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Martijn Wargers (dead)]

See upcoming testcase, which crashes current trunk Mozilla's build when visiting the page.
Doesn't crash in 2006-01-30 build, crashes in 2006-01-31 build, so a regression from bug 282105, I think.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yeah.  I bet prior to bug 282105 we just totally failed to call into the listbox here.  Now we do, and it screws up the OnContentRemoved notification, sorta.  I bet we could produce similar effects even before the patch for bug 282105.

The problem is that the table cell is not actually a child of the listboxbody -- the anon table frame is.  So when we call into nsListBoxBodyFrame::RemoveChildFrame we don't actually remove any frames from anywhere... but we DO call Destroy() on aFrame. So after that we have a deleted frame in the frametree, and we're dead.

Now I could easily fix things to not call Destroy() if we fail to destroy aFrame.  But then we'd end up with the frame not getting removed, and ContentInserted creating a new frame and bad stuff would happen.

I guess what we should do is only notify the listbox body if the parent of our frame is a listbox body frame.  But that means that the pseudo work will need to deal with this later (that is, when we get pseudo removal working).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

I'll merge this to branches once I know it's ok in general.

Comment 4

[[e-mail]]

Comment on attachment 211519 [details] [diff] [review]
Proposed patch

I don't have time to give this a proper review as I need to get up early tomorrow morning to go on holiday (US: vacation) for the rest of the week. But it occurs to me that you could tack your extra condition onto the one just missed by your -u8 diff since aChildFrame is always null when notifying for insertions.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 211519 [details] [diff] [review]
Proposed patch

I can't, since my extra condition needs to compare to |listBoxBodyFrame|, which I don't have up there.

I think we want this in sooner than a week.  Roc, are you ok with doing r+sr on this?

Comment 6

[[e-mail]]

(In reply to comment #5)
>(From update of attachment 211519 [details] [diff] [review])
>I can't, since my extra condition needs to compare to |listBoxBodyFrame|, which
>I don't have up there.
I do need the sleep, don't I ;-) How about putting the condition on the call to RemoveChildFrame in nsListBoxBodyFrame::OnContentRemoved i.e.
if (aChildFrame && aChildFrame->GetParent() == this) {
  RemoveChildFrame(state, aChildFrame);
}

Comment 7

[Boris Zbarsky [:bzbarsky]]

> How about putting the condition on the call to RemoveChildFrame in
> nsListBoxBodyFrame::OnContentRemoved 

I need to return different booleans in the frame constructor depending on this condition; if we're not removing via OnContentRemoved we need to remove through the normal codepath.  See comment 2 paragraph 3.  And nsListBoxBodyFrame::OnContentRemoved returns void.  I could change this, I guess, and just return what I'm checking up front right now...

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should take this patch and add an assertion to OnContentRemoved that aChildFrame's parent == this if it's non-null.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8.0.x branch patch

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 version of patch

Comment 12

[Boris Zbarsky [:bzbarsky]]

This is not an issue on the 1.7 branch since we didn't do table pseudos for XUL there.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.1 branch.

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using build 2006-02-13-09 with testcase: https://bugzilla.mozilla.org/attachment.cgi?id=211511&action=view on Windows XP SeaMonkey trunk.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 211637 [details] [diff] [review]
1.8.0.x branch patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 17

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.0.2 branch.

Comment 18

[Jay Patel [:jay]]

v.fixed on 1.8.0 branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.1) Gecko/20060224 Firefox/1.5.0.1, no crash with testcase.

Comment 19

[juan becerra [:juanb]]

Verified on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060815 BonEcho/2.0b1, as well as MacOS X version.

Firefox did not crash with the testcase.

Comment 20

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/60280a93b9cd