488505 - Crash [@ nsDocAccessible::FlushPendingEvents][@ nsAccessibleTreeWalker::UpdateFrame] with menuitem, menupopup and DOMAttrmodified removing window

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Martijn Wargers (dead)]

Attachment: zipped up testcase

See zipped up testcase, the zipped up testcase uses enhanced privileges (for the accessible retrieval part).
Open the 'parentframe.htm' file and let it run for a while.
It usually crashes within 5 minutes or so.

http://crash-stats.mozilla.com/report/index/f37df384-f9e2-4f32-b70b-bda602090414?p=1
0  	xul.dll  	nsDocAccessible::FlushPendingEvents  	 accessible/src/base/nsDocAccessible.cpp:1715
1 	xul.dll 	nsDocAccessible::FlushEventsCallback 	accessible/src/base/nsDocAccessible.cpp:1755
2 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:427
3 	nspr4.dll 	_PR_MD_UNLOCK 	nsprpub/pr/src/md/windows/w95cv.c:344
4 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:519
5 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
6 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:170
7 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:192
8 	nspr4.dll 	PR_GetEnv 	
9 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:107
10 	firefox.exe 	firefox.exe@0x21a7 	
11 	kernel32.dll 	BaseProcessStart 

http://crash-stats.mozilla.com/report/index/c5022632-0c17-431a-bbdc-e51732090415
0  	xul.dll  	nsAccessibleTreeWalker::UpdateFrame  	 accessible/src/base/nsAccessibleTreeWalker.cpp:270
1 	xul.dll 	nsAccessibleTreeWalker::GetNextSibling 	accessible/src/base/nsAccessibleTreeWalker.cpp:187
2 	xul.dll 	nsAccessible::CacheChildren 	accessible/src/base/nsAccessible.cpp:757
3 	xul.dll 	nsHyperTextAccessible::CacheChildren 	accessible/src/html/nsHyperTextAccessible.cpp:217
4 	xul.dll 	nsAccessible::GetChildCount 	accessible/src/base/nsAccessible.cpp:773
5 	xul.dll 	nsAccessible::GetFirstChild 	accessible/src/base/nsAccessible.cpp:645
6 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
7 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2480

It also crashes Firefox3.0.7, but it has a different stacktrace there:
http://crash-stats.mozilla.com/report/index/4b98be3c-e2c3-4bdc-9ea9-046712090415?p=1
0  	xul.dll  	XPCJSRuntime::GCCallback  	 mozilla/js/src/xpconnect/src/xpcjsruntime.cpp:818

I don't know if this needs to be security sensitive, but I'm doing it now, just to be sure.

Comment 1

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

This looks sg:critical to me, if content can trigger the crash somehow.
mState.frame points to 0xdadadadadadadada

Comment 2

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

(In reply to comment #1)
> This looks sg:critical to me, if content can trigger the crash somehow.
> mState.frame points to 0xdadadadadadadada
That is with nsAccessibleTreeWalker::UpdateFrame stack.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: wip

This bail should fix the nsAccessibleTreeWalker::UpdateFrame stack since we have an else clause later that doesn't check mFrame.frame.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

The first stack seem to indicate general badness.

Benjamin, any ideas about the third stack? Seems to point to an access violation at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/src/xpcjsruntime.cpp&rev=1.74&mark=818#818

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Oops. Benjamin please see comment #4

Comment 6

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

nsAccessibleTreeWalker::UpdateFrame stack shows some problems in that method.
For example the flush may change event count, I think.

Comment 7

[Benjamin Smedberg]

The xpcjsruntime stack indicates that you're over-releasing an object (it's calling ->Release() on an object that has already been destroyed).

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #6)
> nsAccessibleTreeWalker::UpdateFrame stack shows some problems in that method.
> For example the flush may change event count, I think.

Olli, can you expand on that? Also, please feel free to take this one if you think you can get at the real source of the issue.

(In reply to comment #7)
> The xpcjsruntime stack indicates that you're over-releasing an object (it's
> calling ->Release() on an object that has already been destroyed).

Hmmm yeah, makes sense. Given that, it sounds like Olli might be on to something.

Comment 9

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

(In reply to comment #8)
> Olli, can you expand on that? Also, please feel free to take this one if you
> think you can get at the real source of the issue.
In that method there is PRUint32 length = mEventsToFire.Count(); and
after that layout is flushed. That may run scripts. Then there is the loop
which uses length. Not sure if that causes the problem.

There is perhaps another problem too. There is NS_RELEASE_THIS() near end of the
method. But after that 'this' is still accessed, at least when mInFlushPendingEvents is set to PR_FALSE

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Ah, okay thanks. I realize now that "that method" refers to FlushPendingEvents :)

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Attachment: wip2

(In reply to comment #9)
> In that method there is PRUint32 length = mEventsToFire.Count(); and
> after that layout is flushed. That may run scripts. Then there is the loop
> which uses length. Not sure if that causes the problem.

Yeah I think it is prudent to move the calc of length. I also tweaked ::Shutdown which clears the event queue.
 
> There is perhaps another problem too. There is NS_RELEASE_THIS() near end of
> the
> method. But after that 'this' is still accessed, at least when
> mInFlushPendingEvents is set to PR_FALSE

I think there should still be a non-zero reference count though. I'm not yet sure how we are doing too many releases.

Comment 12

[Martijn Wargers (dead)]

My guess is this will be fixed by bug 508523.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Yeah that was Olli's final suggestion in comment #9. Let's hope it is resolved. We probably should commit some or all of the patch here as well but I can do that separate to this bug -- unless it is reopened soonish.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Not blocking, but we'd take the patch from bug 508523 if it was nominated.