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

RESOLVED FIXED

Status

()

Core
XUL
--
critical
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: smaug)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
crash, testcase, verified1.8.0.9, verified1.8.1.1
Points:
---
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)

(Reporter)

Description

11 years ago
Loading the testcase makes Firefox crash [@ nsListBoxBodyFrame::GetNextItemBox].
(Reporter)

Comment 1

11 years ago
Created attachment 245540 [details]
testcase
(Reporter)

Updated

11 years ago
Group: security
(Reporter)

Comment 2

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

Updated

11 years ago
OS: Mac OS X 10.4 → All
Hardware: Macintosh → All
(Assignee)

Comment 3

11 years ago
So the problem seems to be that mTopFrame is cached.
(Assignee)

Comment 4

11 years ago
Created attachment 245649 [details] [diff] [review]
possible patch

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)
(Assignee)

Comment 5

11 years ago
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+
(Assignee)

Comment 7

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

(Assignee)

Updated

11 years ago
Attachment #245649 - Flags: superreview?(roc)
(Assignee)

Comment 8

11 years ago
mLinkupFrame is used only for comparisons, so that should be safe, I think.
(Assignee)

Comment 9

11 years ago
And mBottomFrame should be safe too. It is used only when mTopFrame and mBottomFrame are both updated.
Fragile, but hopefully safe.
(Reporter)

Updated

11 years ago
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+
(Assignee)

Updated

11 years ago
Blocks: 360917
(Assignee)

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Attachment #245649 - Flags: approval1.8.1.1?
Attachment #245649 - Flags: approval1.8.0.9?

Updated

11 years ago
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+
(Assignee)

Comment 12

11 years ago
Created attachment 246161 [details] [diff] [review]
for 1.8 with Bug 361058
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.0.9, fixed1.8.1.1

Comment 13

11 years ago
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).
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
Group: security
(Reporter)

Comment 14

10 years ago
Crashtest checked in.
Flags: in-testsuite+

Updated

9 years ago
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.