Crash [@ nsListBoxBodyFrame::GetNextItemBox] removing child from <listboxbody>

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
13 years ago
8 years ago

People

(Reporter: jruderman, Assigned: smaug)

Tracking

(Blocks 2 bugs, 4 keywords)

Dependency tree / graph
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(3 attachments)

Loading the testcase makes Firefox crash [@ nsListBoxBodyFrame::GetNextItemBox].
Posted file testcase
Group: security
Oops.  This looked like a null deref in an opt build, but in a debug build, not so much:

EXC_BAD_ACCESS (0x0001)
KERN_INVALID_ADDRESS (0x0001) at 0xddddde01

Thread 0 Crashed:
0    nsIFrame::IsBoxFrame() const + 20 (nsIFrame.h:1546)
1    nsListBoxBodyFrame::GetNextItemBox(nsIFrame*, int, int*) + 696 (nsListBoxBodyFrame.cpp:1224)
...
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
So the problem seems to be that mTopFrame is cached.
Using nsWeakFrame is at least simple, operator overloading works in most
cases.
Other option might be to not to cache mTopFrame at all.
Assignee: nobody → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #245649 - Flags: review?(enndeakin)
Is this too late for 1.8.1.1?
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Comment on attachment 245649 [details] [diff] [review]
possible patch

This should be ok for a security update if needed. There's also mBottomFrame and mLinkupFrame as well. Maybe we should do to the same for those.

I think in the long run we want to find a way to remove the need to cache these frames.
Attachment #245649 - Flags: review?(enndeakin) → review+
(In reply to comment #6)
> (From update of attachment 245649 [details] [diff] [review] [edit])
> This should be ok for a security update if needed. There's also mBottomFrame
> and mLinkupFrame as well. Maybe we should do to the same for those.
> 
mBottomFrame is usually updated whenever mTopFrame is, but in any case yes, 
should look at whether it is possible to crash with deleted mBottomFrame or
mLinkupFrame.

Attachment #245649 - Flags: superreview?(roc)
mLinkupFrame is used only for comparisons, so that should be safe, I think.
And mBottomFrame should be safe too. It is used only when mTopFrame and mBottomFrame are both updated.
Fragile, but hopefully safe.
Whiteboard: [sg:critical?]
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
Comment on attachment 245649 [details] [diff] [review]
possible patch

We probably shouldn't be holding pointers to these frames here. Can't we reacquire them as needed? I'd like to see a real fix for this on trunk...
Attachment #245649 - Flags: superreview?(roc) → superreview+
Blocks: 360917
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #245649 - Flags: approval1.8.1.1?
Attachment #245649 - Flags: approval1.8.0.9?
Depends on: 361058
Comment on attachment 245649 [details] [diff] [review]
possible patch

approved for 1.8/1.8.0 branches, a=dveditz

Please also include port/build bustage fix bug 361058
Attachment #245649 - Flags: approval1.8.1.1?
Attachment #245649 - Flags: approval1.8.1.1+
Attachment #245649 - Flags: approval1.8.0.9?
Attachment #245649 - Flags: approval1.8.0.9+
v.fixed on 1.8.0 and 1.8.1 branches with 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.9pre) Gecko/20061128 Firefox/1.5.0.9pre 
and 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre

No crashes with testcase (was crashing FF2.0).
Group: security
Crashtest checked in.
Flags: in-testsuite+
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Crash Signature: [@ nsListBoxBodyFrame::GetNextItemBox]
You need to log in before you can comment on or make changes to this bug.