Last Comment Bug 326834 - CVE-2006-1531 [FIX]Crash with evil xul testcase, using listbox/listitem and display: table-cell
: CVE-2006-1531 [FIX]Crash with evil xul testcase, using listbox/listitem and d...
Status: VERIFIED FIXED
[sg:critical?][rft-dl] not in moz1.7/...
: crash, regression, testcase, verified1.8.0.2, verified1.8.1
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 282105
  Show dependency treegraph
 
Reported: 2006-02-11 13:18 PST by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
Modified: 2009-04-24 11:19 PDT (History)
10 users (show)
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.2+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (760 bytes, application/vnd.mozilla.xul+xml)
2006-02-11 13:20 PST, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
Proposed patch (3.19 KB, patch)
2006-02-11 15:02 PST, Boris Zbarsky [:bz]
roc: review+
roc: superreview+
Details | Diff | Review
Updated to comments (3.81 KB, patch)
2006-02-12 13:38 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Review
1.8.0.x branch patch (3.80 KB, patch)
2006-02-12 13:50 PST, Boris Zbarsky [:bz]
dveditz: approval1.8.0.2+
Details | Diff | Review
1.8 version of patch (3.72 KB, patch)
2006-02-12 13:56 PST, Boris Zbarsky [:bz]
roc: approval‑branch‑1.8.1+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2006-02-11 13:18:06 PST
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 [:mwargers] (gone per 2016-05-31 :-( ) 2006-02-11 13:20:22 PST
Created attachment 211511 [details]
testcase
Comment 2 Boris Zbarsky [:bz] 2006-02-11 14:37:57 PST
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 [:bz] 2006-02-11 15:02:32 PST
Created attachment 211519 [details] [diff] [review]
Proposed patch

I'll merge this to branches once I know it's ok in general.
Comment 4 neil@parkwaycc.co.uk 2006-02-11 15:18:29 PST
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 [:bz] 2006-02-11 15:23:03 PST
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 neil@parkwaycc.co.uk 2006-02-11 15:35:57 PST
(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 [:bz] 2006-02-11 15:57:34 PST
> 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) (Exited; email my personal email if necessary) 2006-02-12 13:08:00 PST
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 [:bz] 2006-02-12 13:38:22 PST
Created attachment 211635 [details] [diff] [review]
Updated to comments
Comment 10 Boris Zbarsky [:bz] 2006-02-12 13:50:55 PST
Created attachment 211637 [details] [diff] [review]
1.8.0.x branch patch
Comment 11 Boris Zbarsky [:bz] 2006-02-12 13:56:49 PST
Created attachment 211639 [details] [diff] [review]
1.8 version of patch
Comment 12 Boris Zbarsky [:bz] 2006-02-12 13:57:53 PST
This is not an issue on the 1.7 branch since we didn't do table pseudos for XUL there.
Comment 13 Boris Zbarsky [:bz] 2006-02-12 13:59:35 PST
Fixed on trunk.
Comment 14 Boris Zbarsky [:bz] 2006-02-13 11:34:13 PST
Fixed on 1.8.1 branch.
Comment 15 Stephen Donner [:stephend] - PTO; back on 5/28 2006-02-13 23:31:08 PST
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] 2006-02-16 12:52:30 PST
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 [:bz] 2006-02-16 18:10:04 PST
Fixed on 1.8.0.2 branch.
Comment 18 Jay Patel [:jay] 2006-02-24 16:49:06 PST
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] 2006-08-15 16:00:36 PDT
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:] 2009-04-24 11:19:40 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/60280a93b9cd

Note You need to log in before you can comment on or make changes to this bug.