558309 - Crash [@ nsAccessibilityService::CreateAccessibleForDeckChild] with tree

Status: RESOLVED WORKSFORME

(Core :: Disability Access APIs, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase (uses enhanced privileges)

See testcase, which crashes current trunk buil after 100ms.
It doesn't crash Firefox3.6.

http://crash-stats.mozilla.com/report/index/bf5cd9f7-6cda-431e-b052-837302100409
0  	xul.dll  	nsAccessibilityService::CreateAccessibleForDeckChild  	 accessible/src/base/nsAccessibilityService.cpp:2093
1 	xul.dll 	nsAccessibilityService::GetAccessible 	accessible/src/base/nsAccessibilityService.cpp:1627
2 	xul.dll 	nsAccTreeWalker::GetNextChildInternal 	accessible/src/base/nsAccTreeWalker.cpp:115
3 	xul.dll 	nsAccTreeWalker::GetNextChild 	accessible/src/base/nsAccTreeWalker.h:65
4 	xul.dll 	nsAccUtils::HasAccessibleChildren 	accessible/src/base/nsAccUtils.cpp:364
5 	xul.dll 	nsDocAccessible::InvalidateCacheSubtree 	accessible/src/base/nsDocAccessible.cpp:2087
6 	xul.dll 	nsAccessibilityService::InvalidateSubtreeFor 	accessible/src/base/nsAccessibilityService.cpp:2053
7 	xul.dll 	nsGenericElement::doRemoveChildAt 	
8 	xul.dll 	nsGenericElement::RemoveChildAt 	content/base/src/nsGenericElement.cpp:3367
9 	xul.dll 	nsXULElement::RemoveChildAt 	content/xul/content/src/nsXULElement.cpp:979
10 	xul.dll 	nsINode::ReplaceOrInsertBefore 	
11 	xul.dll 	nsINode::ReplaceOrInsertBefore 	content/base/src/nsGenericElement.cpp:583
12 	xul.dll 	nsGenericElement::InsertBefore 	content/base/src/nsGenericElement.h:499
13 	xul.dll 	nsGenericElement::AppendChild 	content/base/src/nsGenericElement.h:512
14 	xul.dll 	nsXMLElement::AppendChild 	content/xml/content/src/nsXMLElement.h:58
15 	xul.dll 	nsIDOMNode_AppendChild 	obj-firefox/js/src/xpconnect/src/dom_quickstubs.cpp:4879
16 	mozjs.dll 	js_Interpret 	js/src/jsops.cpp:2243
17 	mozjs.dll 	js_Invoke 	js/src/jsinterp.cpp:1372
18 	mozjs.dll 	js_InternalInvoke 	js/src/jsinterp.cpp:1429
19 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:4957
20 	xul.dll 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:2161
21 	xul.dll 	nsGlobalWindow::RunTimeout 	dom/base/nsGlobalWindow.cpp:8403
22 	xul.dll 	nsGlobalWindow::TimerCallback 	dom/base/nsGlobalWindow.cpp:8747
23 	xul.dll 	nsTimerImpl::Fire 	xpcom/threads/nsTimerImpl.cpp:427
24 	xul.dll 	nsTimerEvent::Run 	xpcom/threads/nsTimerImpl.cpp:519
25 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:527
26 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:118
27 	xul.dll 	xul.dll@0x976ff3 	
28 	xul.dll 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:216
29 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:199
30 	xul.dll 	xul.dll@0x2e4e03 	
31 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:173
32 	nspr4.dll 	nspr4.dll@0x187ff 	
33 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:174

Comment 1

[Marco Zehe (:MarcoZ)]

This looks like a regression from the TreeWalker changes done in Q1.

Comment 2

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

Hmm okay we might want to use a weakframe.

Comment 3

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

Oh we pass a weakFrame->GetFrame() into this last method but we don't null check it. Should be a straight fix.

Comment 4

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

Attachment: WIP

More guarding against bad frames. Not sure I'm happy with it yet.

Comment 5

[Marco Zehe (:MarcoZ)]

Comment on attachment 438087 [details] [diff] [review]
WIP

> NS_IMETHODIMP
> nsAccessibilityService::CreateHTMLImageAccessible(nsIFrame *aFrame,
>                                                   nsIAccessible **aAccessible)
> {
>+  NS_ENSURE_ARG_POINTER(aFrame);

I don't think that's the right thing to do here, since aFrame is not an OUT param. The others look right, but this one definitely doesn't.

Also another thought: Would it make sense to bail out before actually trying to create the accessible? I mean doesn't the calling method also know if aFrame is NULL?

Comment 6

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

(In reply to comment #5)
> (From update of attachment 438087 [details] [diff] [review])
> > NS_IMETHODIMP
> > nsAccessibilityService::CreateHTMLImageAccessible(nsIFrame *aFrame,
> >                                                   nsIAccessible **aAccessible)
> > {
> >+  NS_ENSURE_ARG_POINTER(aFrame);
> 
> I don't think that's the right thing to do here, since aFrame is not an OUT
> param. The others look right, but this one definitely doesn't.

Ah ok, I'm still getting the hang of these macros.

> Also another thought: Would it make sense to bail out before actually trying to
> create the accessible? I mean doesn't the calling method also know if aFrame is
> NULL?

It does make sense, but not sure which is better.

Thanks for the feedback.

Comment 7

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

Attachment: WIP

Comment 8

[alexander :surkov (:asurkov)]

Comment on attachment 438133 [details] [diff] [review]
WIP

All Create methods assumes the given frame is not null, these methods are called either from frame itself or from GetAccessible() method what have null check for the given frame. So the patch might fix the bug but real problem.

I'd like to know a reason why this happen instead an additional null checks, so feedback- until we know this is a best what we can do.

Comment 9

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

I can't recreate anymore. I filed related bug 566417 for something that caught my eye: our check for a live frame doesn't cover enough cases.

Please reopen if you can recreate this bug.

Comment 10

[Martijn Wargers (dead)]

It seems to crash again in current trunk build.

Comment 11

[alexander :surkov (:asurkov)]

Request blocking since it's accessible tree creation problem and we can face it in real life without enhanced privileges requirement and without test case. That must be investigated and fixed before Firefox 4 ship.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> I can't recreate anymore. I filed related bug 566417 for something that caught
> my eye: our check for a live frame doesn't cover enough cases.

At the first glance we deal with content, DOM and XBL only nsAccessibilityService::GetOrCreateAccessible in this case. Nevertheless at some point frame dies there. We need to learn where and perhaps cc Roc to get a help to understand why frame is destroyed.

Comment 13

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

Yay! I have it crashing.

Comment 14

[alexander :surkov (:asurkov)]

worksforme on 22/10/2010 (a local build).