Closed Bug 509602 Opened 11 years ago Closed 11 years ago

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


(Core :: Layout, defect, P2)




Tracking Status
status1.9.2 --- beta1-fixed
blocking1.9.1 --- .4+
status1.9.1 --- .4-fixed


(Reporter: martijn.martijn, Assigned: tnikkel)




(6 keywords, Whiteboard: [sg:critical?])

Crash Data


(5 files, 2 obsolete files)

Attached file zipped up testcase
See zipped up testcase, which crashes in current trunk build. This regressed between 2009-07-29 and 2009-07-30:
My guess a regression from bug 486065.
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/
3 	xul.dll 	google_breakpad::ExceptionHandler::HandlePureVirtualCall 	toolkit/crashreporter/google-breakpad/src/client/windows/handler/
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
Attachment #393655 - Attachment mime type: application/octet-stream → application/java-archive
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 (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, 
    at /src/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:987
#2  0xb4f6945a in nsTreeBodyFrame::FullScrollbarsUpdate (this=0xaf8519d0, 
    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, 
    at /src/layout/base/nsPresShell.cpp:4880
#11 0xb4beb84e in nsDocument::FlushPendingNotifications (this=0xadd36400, 
    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.
No longer blocks: 486065
This crashes on Linux as well, stack is almost identical:

OS: Windows XP → All
Hardware: x86 → All
Group: core-security
Flags: blocking1.9.2?
The regression range is due to bug 478416. Here 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="" 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.
Attachment #394787 - Flags: review?(Olli.Pettay)
Blocks: 478416, 382444
(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?
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.
Assignee: nobody → tnikkel
Attachment #394787 - Attachment is obsolete: true
Attachment #394884 - Flags: review?(Olli.Pettay)
Attachment #394787 - Flags: review?(Olli.Pettay)
Attachment #394884 - Flags: review?(Olli.Pettay) → review+
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.
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.
Attachment #394884 - Attachment is obsolete: true
Flags: in-testsuite?
Ok, I'll push it tomorrow.
Could you still upload a separate patch which contains just the test.
Attached patch crashtestSplinter Review
Attachment #395146 - Flags: superreview?(roc)
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.
Attachment #395146 - Flags: superreview?(roc) → superreview?(neil)
Comment on attachment 395146 [details] [diff] [review]
above patch with nit fixed, and without test

Roc is on vacation, so perhaps Neil could sr.
Attachment #395146 - Flags: superreview?(neil) → superreview+
blocking1.9.1: --- → ?
status1.9.1: --- → ?
Flags: blocking1.9.0.15?
Closed: 11 years ago
Resolution: --- → FIXED
Summary: Crash [@ nsTreeBodyFrame::InvalidateScrollbars] with overlyay, tree and DOMAttrModified → Crash [@ nsTreeBodyFrame::InvalidateScrollbars] with overlay, tree and DOMAttrModified
Attachment #395146 - Flags: approval1.9.2?
blocking1.9.1: ? → .4+
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.15?
Flags: blocking1.9.0.15+
Whiteboard: [sg:critical?]
Attached patch 1.9.1 patchSplinter Review
Just fixed up for context.
Attachment #396045 - Flags: approval1.9.1.4?
Attached patch 1.9.0 patchSplinter Review
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.
Attachment #396046 - Flags: review?(Olli.Pettay)
Attachment #396046 - Flags: approval1.9.0.15?
Attachment #396046 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 396046 [details] [diff] [review]
1.9.0 patch

Approved for, a=dveditz for release-drivers
Attachment #396046 - Flags: approval1.9.0.15? → approval1.9.0.15+
Comment on attachment 396045 [details] [diff] [review]
1.9.1 patch

Approved for, a=dveditz for release-drivers
Attachment #396045 - Flags: approval1.9.1.4? → approval1.9.1.4+
Olli, would you be so kind as to land these on 1.9.0 and 1.9.1 for me?
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
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
Keywords: fixed1.9.0.15
Attachment #395146 - Flags: approval1.9.2? → approval1.9.2+
Olli, and finally can I ask you to land this on 1.9.2 when it's convenient for you?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Verified fixed in with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2009091606 GranParadiso/3.0.15pre (.NET CLR 3.5.30729) using crashing testcase (and verified crash in
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: Gecko/20091007 Firefox/3.5.4 (.NET CLR 3.5.30729) ID:20091007001339
Target Milestone: --- → mozilla1.9.3a1
Group: core-security
Crashtest checked in:
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsTreeBodyFrame::InvalidateScrollbars]
You need to log in before you can comment on or make changes to this bug.