509602 - Crash [@ nsTreeBodyFrame::InvalidateScrollbars] with overlay, tree and DOMAttrModified

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: zipped up testcase

See zipped up testcase, which crashes in current trunk build. This regressed between 2009-07-29 and 2009-07-30:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-07-29+04%3A00%3A00&enddate=2009-07-30+07%3A00%3A00
My guess a regression from bug 486065.

http://crash-stats.mozilla.com/report/index/48bbded9-d50e-4793-a9d4-fb0472090810?p=1
0  	ntdll.dll  	ntdll.dll@0xe514  	
1 	kernel32.dll 	kernel32.dll@0x2541 	
2 	xul.dll 	google_breakpad::ExceptionHandler::WriteMinidumpOnHandlerThread 	toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc:562
3 	xul.dll 	google_breakpad::ExceptionHandler::HandlePureVirtualCall 	toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc:506
4 	mozcrt19.dll 	_purecall 	obj-firefox/memory/jemalloc/crtsrc/purevirt.c:47
5 	xul.dll 	nsCOMPtr<nsIProtocolProxyCallback>::nsCOMPtr<nsIProtocolProxyCallback> 	obj-firefox/dist/include/nsCOMPtr.h:554
6 	xul.dll 	nsTreeBodyFrame::InvalidateScrollbars 	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:987
7 	xul.dll 	nsTreeBodyFrame::FullScrollbarsUpdate 	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:4717
8 	xul.dll 	nsTreeBodyFrame::SetView 	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:546
9 	xul.dll 	nsTreeBoxObject::GetView 	layout/xul/base/src/tree/src/nsTreeBoxObject.cpp:169
10 	xul.dll 	nsTreeBodyFrame::EnsureView 	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:393
11 	xul.dll 	nsTreeBodyFrame::ReflowFinished 	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:439
12 	xul.dll 	PresShell::HandlePostedReflowCallbacks 	layout/base/nsPresShell.cpp:4769

Comment 1

[Martijn Wargers (dead)]

The crashing link is:
jar:https://bugzilla.mozilla.org/attachment.cgi?id=393655!/crash1.xul

Comment 2

[Timothy Nikkel (:tnikkel)]

It wasn't caused by bug 486065, as with that backed out it still crashes. I don't know what in that regression range causes it though.

BTW Martijn, you can specify slightly better links to regression ranges using the format http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=55fd6820c596&tochange=3222b99f441c (about:buildconfig for the changeset id's). I don't know if there is a form where you can input the changeset id's to get the link though.

I get this stack.

#0  0xb499fd34 in nsCOMPtr (this=0xbfde14b8, aRawPtr=0xafb2adc0)
    at ../../../dist/include/nsCOMPtr.h:554
#1  0xb4f680cc in nsTreeBodyFrame::InvalidateScrollbars (this=0xaf8519d0, 
    aParts=@0xbfde14ec)
    at /src/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:987
#2  0xb4f6945a in nsTreeBodyFrame::FullScrollbarsUpdate (this=0xaf8519d0, 
    aNeedsFullInvalidation=0)
    at /src/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:4717
#3  0xb4f6a899 in nsTreeBodyFrame::SetView (this=0xaf8519d0, aView=0xaf7e1c40)
    at /src/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:546
#4  0xb4f6dce7 in nsTreeBoxObject::GetView (this=0xaf8117a0, aView=0xbfde1700)
    at /src/layout/xul/base/src/tree/src/nsTreeBoxObject.cpp:169
#5  0xb4f6ab1d in nsTreeBodyFrame::EnsureView (this=0xaf8519d0)
    at /src/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:393
#6  0xb4f6af84 in nsTreeBodyFrame::ReflowFinished (this=0xaf8519d0)
    at /src/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:439
#7  0xb49ffc76 in PresShell::HandlePostedReflowCallbacks (this=0xadd37c00, 
    aInterruptible=0) at /src/layout/base/nsPresShell.cpp:4769
#8  0xb4a039d9 in PresShell::DidDoReflow (this=0xadd37c00, aInterruptible=0)
    at /src/layout/base/nsPresShell.cpp:7065
#9  0xb4a0ff8f in PresShell::ProcessReflowCommands (this=0xadd37c00, 
    aInterruptible=0) at /src/layout/base/nsPresShell.cpp:7319
#10 0xb4a100c0 in PresShell::FlushPendingNotifications (this=0xadd37c00, 
    aType=Flush_Layout)
    at /src/layout/base/nsPresShell.cpp:4880
#11 0xb4beb84e in nsDocument::FlushPendingNotifications (this=0xadd36400, 
    aType=Flush_Layout)
    at /src/content/base/src/nsDocument.cpp:6272
#12 0xb56bc313 in nsDocLoader::DocLoaderIsEmpty (this=0xb01159a0, 
    aFlushLayout=1) at /src/uriloader/base/nsDocLoader.cpp:756
#13 0xb56bc714 in nsDocLoader::OnStopRequest (this=0xb01159a0, 
    aRequest=0xaf7e0130, aCtxt=0x0, aStatus=0)
    at /src/uriloader/base/nsDocLoader.cpp:697
#14 0xb5bbd653 in nsLoadGroup::RemoveRequest (this=0xaff57510, 
    request=0xaf7e0130, ctxt=0x0, aStatus=0)
    at /src/netwerk/base/src/nsLoadGroup.cpp:680
#15 0xb4beb514 in nsDocument::DoUnblockOnload (this=0xadd36400)
    at /src/content/base/src/nsDocument.cpp:7025
#16 0xb4da2edc in nsBindingManager::DoProcessAttachedQueue (this=0xafbbe900)
    at /src/content/xbl/src/nsBindingManager.cpp:1000
#17 0xb4da57a0 in nsRunnableMethod<nsBindingManager, void>::Run (
    this=0xaf7f24c0) at ../../../dist/include/nsThreadUtils.h:264
#18 0xb7d54149 in nsThread::ProcessNextEvent (this=0xb6a7ace0, mayWait=1, 
    result=0xbfde1bc0) at /src/xpcom/threads/nsThread.cpp:527
#19 0xb7cfc14e in NS_ProcessNextEvent_P (thread=0xaf8519d0, mayWait=1)
    at nsThreadUtils.cpp:230
#20 0xb594dc30 in nsBaseAppShell::Run (this=0xb25ddec0)
    at /src/widget/src/xpwidgets/nsBaseAppShell.cpp:170
#21 0xb5acb1c4 in nsAppStartup::Run (this=0xb2307550)
    at /src/toolkit/components/startup/src/nsAppStartup.cpp:193
#22 0xb7f6a418 in XRE_main (argc=1, argv=0xbfde2184, aAppData=0xb6a06540)
    at /src/toolkit/xre/nsAppRunner.cpp:3392
#23 0x0804996c in main (argc=1, argv=0xbfde2184)
    at /src/browser/app/nsBrowserApp.cpp:156

We crash trying to access parts.mVScrollbarContent.

So it looks like in nsTreeBodyFrame::FullScrollbarsUpdate that parts.mVScrollbarContent is fine after GetScrollParts(). But when we call UpdateScrollbars() we call SetAttr on parts.mVScrollbarContent with notify=true. This triggers the onDOMAttrModified in overlay3.xul and we kill the parts.mVScrollbarContent and next time we access it we crash.

Comment 3

[Elmar Ludwig]

This crashes on Linux as well, stack is almost identical:

bp-9e1048a2-0d19-4744-a0a2-704a02090810

Comment 4

[Timothy Nikkel (:tnikkel)]

Attachment: hold a reference to the scrollbar content

The regression range is due to bug 478416. Here http://hg.mozilla.org/mozilla-central/diff/d78664d9530c/toolkit/content/widgets/tree.xml it removed a setAttribute call in the tree constructor. I haven't verified this in a debugger, but adding a dummy setAttribute to the tree constructor causes it to not crash. So I think that in this testcase the setAttribute call kills the tree due to the onDOMAttrModified="event.target.parentNode.removeChild(event.target)" and we don't get any further.

In FullScrollbarsUpdate (which was added in bug 382444) we need to hold a reference to the scrollbar content in case the SetAttr calls in UpdateScrollbars or InvalidateScrollbars kill the scrollbar content.

I think we might have problems with the frames in the ScrollParts struct as well. For example in InvalidateScrollbars, the first SetAttr calls may kill the aParts.mColumnsFrame which we call GetRect on.

Comment 5

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

(In reply to comment #4)
> I think we might have problems with the frames in the ScrollParts struct as
> well. 
Could you add some weakframe checks for those frames?

Could ScrollParts have nsCOMPtr for mVScrollbarContent and mHScrollbarContent?

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: hold a reference to the scrollbar content and do a weak frame check on the columns frame

Used nsCOMPtr in ScrollParts for scrollbar content to hold the reference and get rid of other unneeded nsCOMPtr's for the same.

Do a weak frame check on mColumnsFrame where a SetAttr call might kill it.

Comment 7

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

Comment on attachment 394884 [details] [diff] [review]
hold a reference to the scrollbar content and do a weak frame check on the columns frame


>   // Update the maxpos of the scrollbar.
>-  void InvalidateScrollbars(const ScrollParts& aParts);
>+  void InvalidateScrollbars(const ScrollParts& aParts, nsWeakFrame* aWeakColumnsFrame);
Nit, I'd have
void InvalidateScrollbars(const ScrollParts& aParts, nsWeakFrame& aWeakColumnsFrame);

I think the patch should be checked in first (without tests), and only after
making sure that this doesn't need to be fixed in other branches or after fixing
this everywhere, commit the tests.

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: above patch with nit fixed, and without test

Hmm, testcase crashes 1.9.0, but not 1.9.1.

Olli, can I ask you to push this? I don't have hg access and I don't think checkin-needed is very effective for security bugs.

Comment 9

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

Ok, I'll push it tomorrow.
Could you still upload a separate patch which contains just the test.

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: crashtest

Comment 11

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

Comment on attachment 395146 [details] [diff] [review]
above patch with nit fixed, and without test

Oops, almost forgot, nowadays security bugs need separate r and sr.

Comment 12

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

Comment on attachment 395146 [details] [diff] [review]
above patch with nit fixed, and without test

Roc is on vacation, so perhaps Neil could sr.

Comment 13

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

http://hg.mozilla.org/mozilla-central/rev/945cc6a18e71

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: 1.9.1 patch

Just fixed up for context.

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: 1.9.0 patch

CheckOverflow is called directly instead of off a script runner, and uses the same ScrollParts, so the weakframe check needs to be extended to it. Otherwise the same.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 396046 [details] [diff] [review]
1.9.0 patch

Approved for 1.9.0.15, a=dveditz for release-drivers

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 396045 [details] [diff] [review]
1.9.1 patch

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 18

[Timothy Nikkel (:tnikkel)]

Olli, would you be so kind as to land these on 1.9.0 and 1.9.1 for me?

Comment 19

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

Checking in layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v  <--  nsTreeBodyFrame.cpp
new revision: 1.357; previous revision: 1.356
done
Checking in layout/xul/base/src/tree/src/nsTreeBodyFrame.h;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.h,v  <--  nsTreeBodyFrame.h
new revision: 1.126; previous revision: 1.125
done

Comment 20

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/713661cc83b2

Comment 21

[Timothy Nikkel (:tnikkel)]

Olli, and finally can I ask you to land this on 1.9.2 when it's convenient for you?

Comment 22

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

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7e9fa691511a

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Verified fixed in 1.9.0.15 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.15pre) Gecko/2009091606 GranParadiso/3.0.15pre (.NET CLR 3.5.30729) using crashing testcase (and verified crash in 1.9.0.14).

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed trunk, 1.9.2, and 1.9.1 by using the crash test too:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20091006 Minefield/3.7a1pre (.NET CLR 3.5.30729) ID:20091006044117

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b1pre) Gecko/20091007 Namoroka/3.6b1pre (.NET CLR 3.5.30729) ID:20091007045631

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4) Gecko/20091007 Firefox/3.5.4 (.NET CLR 3.5.30729) ID:20091007001339

Comment 25

[Jesse Ruderman]

Crashtest checked in: http://hg.mozilla.org/mozilla-central/rev/3256d08b6705