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

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

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.

This crashes on Linux as well, stack is almost identical: bp-9e1048a2-0d19-4744-a0a2-704a02090810

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.

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

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.

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

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 on attachment 395146 [details] [diff] [review] above patch with nit fixed, and without test Roc is on vacation, so perhaps Neil could sr.

Just fixed up for context.

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 on attachment 396046 [details] [diff] [review] 1.9.0 patch Approved for 1.9.0.15, a=dveditz for release-drivers

Comment on attachment 396045 [details] [diff] [review] 1.9.1 patch Approved for 1.9.1.4, a=dveditz for release-drivers

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

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

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

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

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