Last Comment Bug 360642 - Crash [@ nsListBoxBodyFrame::GetNextItemBox] removing child from <listboxbody>
: Crash [@ nsListBoxBodyFrame::GetNextItemBox] removing child from <listboxbody>
Status: RESOLVED FIXED
[sg:critical?]
: crash, testcase, verified1.8.0.9, verified1.8.1.1
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
Depends on: 361058
Blocks: 344486 360917
  Show dependency treegraph
 
Reported: 2006-11-13 23:07 PST by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
5 users (show)
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (280 bytes, application/vnd.mozilla.xul+xml)
2006-11-13 23:08 PST, Jesse Ruderman
no flags Details
possible patch (1.48 KB, patch)
2006-11-15 06:29 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
enndeakin: review+
roc: superreview+
dveditz: approval1.8.0.9+
dveditz: approval1.8.1.1+
Details | Diff | Splinter Review
for 1.8 with Bug 361058 (3.14 KB, patch)
2006-11-21 08:29 PST, Olli Pettay [:smaug] (vacation Aug 25-28)
no flags Details | Diff | Splinter Review

Description Jesse Ruderman 2006-11-13 23:07:59 PST
Loading the testcase makes Firefox crash [@ nsListBoxBodyFrame::GetNextItemBox].
Comment 1 Jesse Ruderman 2006-11-13 23:08:30 PST
Created attachment 245540 [details]
testcase
Comment 2 Jesse Ruderman 2006-11-13 23:30:49 PST
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] (vacation Aug 25-28) 2006-11-14 22:34:53 PST
So the problem seems to be that mTopFrame is cached.
Comment 4 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-11-15 06:29:44 PST
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.
Comment 5 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-11-15 08:20:50 PST
Is this too late for 1.8.1.1?
Comment 6 Neil Deakin 2006-11-15 09:45:00 PST
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] (vacation Aug 25-28) 2006-11-15 09:53:50 PST
(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] (vacation Aug 25-28) 2006-11-15 09:59:52 PST
mLinkupFrame is used only for comparisons, so that should be safe, I think.
Comment 9 Olli Pettay [:smaug] (vacation Aug 25-28) 2006-11-15 10:03:34 PST
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) (Exited; email my personal email if necessary) 2006-11-16 00:48:30 PST
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] 2006-11-19 23:46:49 PST
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] (vacation Aug 25-28) 2006-11-21 08:29:03 PST
Created attachment 246161 [details] [diff] [review]
for 1.8 with Bug 361058
Comment 13 Jay Patel [:jay] 2006-11-28 14:52:32 PST
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 2007-12-19 15:09:52 PST
Crashtest checked in.

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