311659 - Crash [@ nsSplitterFrameInner::SetPreferredSize] with this testcase when resizing using xul:splitter

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, it crashes when using the splitter.

Talkback ID: TB10384493H

nsSplitterFrameInner::SetPreferredSize 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsSplitterFrame.cpp,
line 1056]
nsSplitterFrameInner::AdjustChildren 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsSplitterFrame.cpp,
line 1026]
nsSplitterFrameInner::AdjustChildren 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsSplitterFrame.cpp,
line 980]
nsSplitterFrameInner::MouseDrag 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsSplitterFrame.cpp,
line 581]
nsSplitterFrame::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/nsSplitterFrame.cpp,
line 480]
PresShell::HandleEventInternal 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6039]
PresShell::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 5850]
nsViewManager::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/view/src/nsViewManager.cpp,
line 2545]
nsViewManager::DispatchEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/view/src/nsViewManager.cpp,
line 2237]
HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/view/src/nsView.cpp, line
174]
nsWindow::DispatchEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1060]
nsWindow::DispatchMouseEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 5793]
ChildWindow::DispatchMouseEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 6044]
nsWindow::WindowProc 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1249]
USER32.dll + 0x27b17 (0x77d37b17)
USER32.dll + 0x2cdce (0x77d3cdce)
USER32.dll + 0x4435 (0x77d14435)
USER32.dll + 0x9611 (0x77d19611)
nsAppStartup::Run 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,
line 162]
main 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/browser/app/nsBrowserApp.cpp,
line 61]
kernel32.dll + 0x1eb69 (0x77e5eb69)

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Hmm, seems like a regression, this doesn't crash Mozilla1.7.

Comment 2

[Martijn Wargers (dead)]

Ok, doesn't crash with 2004-08-09 build, but crashes with 2004-08-10 build, so
that leads me to think this could be a regression from bug 230170.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I don't really know how to fix this, offhand.  The problem is that the splitter
creates this list of frames in mChildInfosBefore when it gets a mousedown.  Then
in mousedrag it does things with them.  But in this case, the frame gets
destroyed after the mousedown (when we flush out pending restyles because
someone wants a box object) and so we get:

(gdb) fram 0
#0  0xb660157b in nsSplitterFrameInner::SetPreferredSize (this=0x840f1e8, 
    aState=@0xbfffe650, aChildBox=0x8913894, aOnePixel=14, aIsHorizontal=1, 
    aSize=0xbfffe640)
    at ../../../../../mozilla/layout/xul/base/src/nsSplitterFrame.cpp:1056
1056      aChildBox->GetMargin(margin);
(gdb) p/x *aChildBox
$4 = {<nsISupports> = {_vptr.nsISupports = 0x0}, mRect = {x = 0xdddddddd, 
    y = 0xdddddddd, width = 0xdddddddd, height = 0xdddddddd}, mContent =
0xdddddddd, 
  mStyleContext = 0xdddddddd, mParent = 0xdddddddd, mNextSibling = 0xdddddddd, 
  mState = 0xdddddddd}

Which is not happy, as you might imagine.

You could probably get the same result in a build from before bug 230170 got
fixed by doing the style change off a 0-msec timeout....

Comment 4

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

A simple band-aid approach is just to have the nsSplitterInfos store a strong
reference to the content objects for the children, and get the box from those
objects every time.

Comment 5

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

Attachment: fix as described

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 199134 [details] [diff] [review]
fix as described

Seems reasonable, but are we sure we always get the mouseup (eg if the mouseup
happens outside the window)?  I guess we're capturing?

Comment 7

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

I think it doesn't *really* matter if we don't. The only effect will be that we
keep the content alive longer than we otherwise would. We'll still destroy the
frame eventually and that will drop the reference.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK.  Sounds good.

Comment 9

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

checked into trunk. We may want to consider this for the branch on the chance
that it's exploitable.

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

Crash is verified FIXED using SeaMonkey 1.1a;Mozilla/5.0 (Windows; U; Windows NT
5.1; en-US; rv:1.9a1) Gecko/20051012 Mozilla/1.0.

Comment 12

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

Comment on attachment 199134 [details] [diff] [review]
fix as described

Seeking branch approval --- the patch applies fine and fixes the crash.

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 199134 [details] [diff] [review]
fix as described

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers

Comment 14

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

Checked into branches.

Comment 15

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5pre) Gecko/20070710 BonEcho/2.0.0.5pre and the attached "testcase". I crashed in a 2007-07-02 build (without the patch), but don't crash in today's build.

Comment 16

[juan becerra [:juanb]]

Verified fixed using Thunderbird version 1.5.0.13 (20070809) on MacIntel, by setting the start page to the testcase URL. Resizing did not crash it.