Last Comment Bug 509602 - Crash [@ nsTreeBodyFrame::InvalidateScrollbars] with overlay, tree and DOMAttrModified
: Crash [@ nsTreeBodyFrame::InvalidateScrollbars] with overlay, tree and DOMAtt...
Status: VERIFIED FIXED
: [sg:critical?]
: crash, regression, testcase, verified1.9.0.15, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Layout
: Trunk
: All All
: P2 critical (vote)
: mozilla1.9.3a1
Assigned To: Timothy Nikkel (:tn)
: layout
: jar:https://bugzilla.mozilla.org/atta...
:
: 382444 478416
  Show dependency treegraph
 
Reported: 2009-08-10 16:25 PDT by Martijn Wargers [:mw22] (QA - IRC nick: mw22)
Modified: 2009-12-06 17:04 PST (History)
12 users (show)
roc: blocking1.9.2+
dveditz: blocking1.9.0.15+
dveditz: wanted1.9.0.x+
jruderman: in‑testsuite+
See Also:
Crash Signature:
[@ nsTreeBodyFrame::InvalidateScrollbars]
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  ---
  beta1-fixed
  .4+
  .4-fixed


Attachments
zipped up testcase (572 bytes, application/java-archive)
2009-08-10 16:25 PDT, Martijn Wargers [:mw22] (QA - IRC nick: mw22)
no flags Details
hold a reference to the scrollbar content (2.54 KB, patch)
2009-08-17 01:54 PDT, Timothy Nikkel (:tn)
no flags Details | Diff | Splinter Review
hold a reference to the scrollbar content and do a weak frame check on the columns frame (8.76 KB, patch)
2009-08-17 13:50 PDT, Timothy Nikkel (:tn)
Olli.Pettay: review+
Details | Diff | Splinter Review
above patch with nit fixed, and without test (7.26 KB, patch)
2009-08-18 14:16 PDT, Timothy Nikkel (:tn)
neil: superreview+
roc: approval1.9.2+
Details | Diff | Splinter Review
crashtest (1.57 KB, patch)
2009-08-18 14:19 PDT, Timothy Nikkel (:tn)
no flags Details | Diff | Splinter Review
1.9.1 patch (7.02 KB, patch)
2009-08-21 23:39 PDT, Timothy Nikkel (:tn)
dveditz: approval1.9.1.4+
Details | Diff | Splinter Review
1.9.0 patch (8.57 KB, patch)
2009-08-21 23:43 PDT, Timothy Nikkel (:tn)
Olli.Pettay: review+
dveditz: approval1.9.0.15+
Details | Diff | Splinter Review

Summon comment box

Description Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2009-08-10 16:25:39 PDT
Created attachment 393655 [details]
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 [:mw22] (QA - IRC nick: mw22) 2009-08-10 16:28:20 PDT
The crashing link is:
jar:https://bugzilla.mozilla.org/attachment.cgi?id=393655!/crash1.xul
Comment 2 Timothy Nikkel (:tn) 2009-08-10 23:06:54 PDT
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 2009-08-10 23:26:00 PDT
This crashes on Linux as well, stack is almost identical:

bp-9e1048a2-0d19-4744-a0a2-704a02090810
Comment 4 Timothy Nikkel (:tn) 2009-08-17 01:54:54 PDT
Created attachment 394787 [details] [review]
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] 2009-08-17 02:51:40 PDT
(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 (:tn) 2009-08-17 13:50:18 PDT
Created attachment 394884 [details] [review]
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] 2009-08-18 06:12:23 PDT
Comment on attachment 394884 [details] [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 (:tn) 2009-08-18 14:16:34 PDT
Created attachment 395146 [details] [review]
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] 2009-08-18 14:17:59 PDT
Ok, I'll push it tomorrow.
Could you still upload a separate patch which contains just the test.
Comment 10 Timothy Nikkel (:tn) 2009-08-18 14:19:59 PDT
Created attachment 395148 [details] [review]
crashtest
Comment 11 Olli Pettay [:smaug] 2009-08-19 03:45:12 PDT
Comment on attachment 395146 [details] [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] 2009-08-19 03:45:58 PDT
Comment on attachment 395146 [details] [review]
above patch with nit fixed, and without test

Roc is on vacation, so perhaps Neil could sr.
Comment 13 Olli Pettay [:smaug] 2009-08-19 04:43:00 PDT
http://hg.mozilla.org/mozilla-central/rev/945cc6a18e71
Comment 14 Timothy Nikkel (:tn) 2009-08-21 23:39:48 PDT
Created attachment 396045 [details] [review]
1.9.1 patch

Just fixed up for context.
Comment 15 Timothy Nikkel (:tn) 2009-08-21 23:43:58 PDT
Created attachment 396046 [details] [review]
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 2009-08-24 16:03:07 PDT
Comment on attachment 396046 [details] [review]
1.9.0 patch

Approved for 1.9.0.15, a=dveditz for release-drivers
Comment 17 Daniel Veditz 2009-08-24 16:03:26 PDT
Comment on attachment 396045 [details] [review]
1.9.1 patch

Approved for 1.9.1.4, a=dveditz for release-drivers
Comment 18 Timothy Nikkel (:tn) 2009-08-24 16:08:25 PDT
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] 2009-08-26 04:04:40 PDT
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] 2009-08-26 04:16:36 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/713661cc83b2
Comment 21 Timothy Nikkel (:tn) 2009-09-08 15:20:27 PDT
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] 2009-09-09 09:20:08 PDT
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/7e9fa691511a
Comment 23 Al Billings [:abillings] 2009-09-16 17:03:09 PDT
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) 2009-10-08 12:21:57 PDT
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 2009-12-06 17:04:04 PST
Crashtest checked in: http://hg.mozilla.org/mozilla-central/rev/3256d08b6705

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