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

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jesse Ruderman]

Loading the testcase makes Firefox crash [@ nsListBoxBodyFrame::GetNextItemBox].

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

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)
...

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

So the problem seems to be that mTopFrame is cached.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Is this too late for 1.8.1.1?

Comment 6

[Neil Deakin]

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.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

mLinkupFrame is used only for comparisons, so that should be safe, I think.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

And mBottomFrame should be safe too. It is used only when mTopFrame and mBottomFrame are both updated.
Fragile, but hopefully safe.

Comment 10

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

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...

Comment 11

[Daniel Veditz [:dveditz]]

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

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.8 with Bug 361058

Comment 13

[Jay Patel [:jay]]

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).

Comment 14

[Jesse Ruderman]

Crashtest checked in.