394404 - Crash [@ nsQueryInterface::operator] with q, input and changing display

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase. You need to download it to your computer because of the use of enhanced privileges.

It doesn't seem to crash on branch, so it looks like a regression. I can look for the regression range, if wanted.

(the testcase might be even more simplified a bit, but then it doesn't crash that easily anymore)

http://crash-stats.mozilla.com/report/index/30f36b85-576e-11dc-823c-001a4bd43e5c
0  	@0x83  	
1 	nsQueryInterface::operator()(nsID const&, void**) 	nsCOMPtr.cpp:47
2 	xul.dll@0x58fdef 	
3 	XPCWrappedNative::GetNewOrUsed(XPCCallContext&, nsISupports*, XPCWrappedNativeScope*, XPCNativeInterface*, int, XPCWrappedNative**) 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:289
4 	XPCConvert::NativeInterface2JSObject(XPCCallContext&, nsIXPConnectJSObjectHolder**, nsISupports*, nsID const*, JSObject*, int, int, unsigned int*) 	mozilla/js/src/xpconnect/src/xpcconvert.cpp:1105
5 	XPCConvert::NativeData2JS(XPCCallContext&, long*, void const*, nsXPTType const&, nsID const*, JSObject*, unsigned int*) 	mozilla/js/src/xpconnect/src/xpcconvert.cpp:481
6 	XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) 	mozilla/js/src/xpconnect/src/xpcwrappednative.cpp:2375
7 	XPC_WN_GetterSetter(JSContext*, JSObject*, unsigned int, long*, long*) 	mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp:1499
8 	js_Invoke 	mozilla/js/src/jsinterp.c:1378
9 	js_InternalInvoke 	mozilla/js/src/jsinterp.c:1474
10 	js_InternalGetOrSet 	mozilla/js/src/jsinterp.c:1546
11 	js_NativeGet 	mozilla/js/src/jsobj.c:3441
12 	js_GetProperty 	mozilla/js/src/jsobj.c:3584
13 	js_Interpret 	mozilla/js/src/jsinterp.c:3851
14 	js_Invoke 	mozilla/js/src/jsinterp.c:1399
15 	js_InternalInvoke 	mozilla/js/src/jsinterp.c:1474
16 	JS_CallFunctionValue 	mozilla/js/src/jsapi.c:4940
17 	nsJSContext::CallEventHandler(nsISupports*, void*, void*, nsIArray*, nsIVariant**) 	mozilla/dom/src/base/nsJSEnvironment.cpp:1842
18 	nsGlobalWindow::RunTimeout(nsTimeout*) 	mozilla/dom/src/base/nsGlobalWindow.cpp:7098
19 	nsGlobalWindow::TimerCallback(nsITimer*, void*) 	mozilla/dom/src/base/nsGlobalWindow.cpp:7429
20 	nsTimerImpl::Fire() 	mozilla/xpcom/threads/nsTimerImpl.cpp:384
21 	nsTimerEvent::Run() 	mozilla/xpcom/threads/nsTimerImpl.cpp:457
22 	nsThread::ProcessNextEvent(int, int*) 	mozilla/xpcom/threads/nsThread.cpp:490
23 	NS_ProcessNextEvent_P(nsIThread*, int) 	nsThreadUtils.cpp:227
24 	nsBaseAppShell::Run() 	mozilla/widget/src/xpwidgets/nsBaseAppShell.cpp:154
25 	nsAppStartup::Run() 	mozilla/toolkit/components/startup/src/nsAppStartup.cpp:170
26 	XRE_main 	mozilla/toolkit/xre/nsAppRunner.cpp:3069
27 	main 	mozilla/browser/app/nsBrowserApp.cpp:153
28 	WinMain 	mozilla/browser/app/nsBrowserApp.cpp:166
29 	__tmainCRTStartup 	crtexe.c:589
30 	BaseProcessStart

Comment 1

[Aaron Leventhal]

Martijn, this does not relate to accessibility support. It crashes even if I rename accessibility.dll where all the support is, so that the accessibility code doesn't get loaded or used.

Comment 2

[Martijn Wargers (dead)]

Attachment: Smaller testcase

Comment 3

[Martijn Wargers (dead)]

With that smaller testcase, I still need this line:
  var acc = Components.classes['@mozilla.org/accessibleRetrieval;1']
                         .getService(Components.interfaces.nsIAccessibleRetrieval);
to get the crash.

I get a regression range between 2007-08-28 14:00 and 2007-8-28 15:30 :
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-08-28+14&maxdate=2007-08-28+1530&cvsroot=%2Fcvsroot
I suspect this is a regression from bug 391846, somehow.

I can now build with accessibility (hooray!), and I see this assertions with the testcase:
###!!! ASSERTION: Stealing child!: '!parent || parent == this', file c:/mozilla-build/mozilla/accessible/src/base/nsAccessible.cpp, line 459
###!!! ASSERTION: Hypertext is reporting a different accessible for this node: 'changeAccessible == aAccessibleForChangeNode', file c:/mozilla-build/mozilla/accessible/src/base/nsDocAccessible.cpp, line 1302
###!!! ASSERTION: Creating accessible for node with no document: 'content->GetDocument()', file c:/mozilla-build/mozilla/accessible/src/base/nsAccessibilityService.cpp, line 1259

Comment 4

[Martijn Wargers (dead)]

Backing out the patch from bug 391846 makes the crash go away in my debug build.

Comment 5

[Aaron Leventhal]

Thanks Martijn, I'll look.

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase3

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> Created an attachment (id=282330) [details]
> testcase3
> 

this one doesn't crash with patch of bug 397112. smaller testcase is still reproduced.

Comment 8

[Aaron Leventhal]

Attachment: Even smaller testcase -- assertion "Stealing child", crashes upon reload

Comment 9

[Aaron Leventhal]

Attachment: One way to fix the problem -- don't try to create a parent in the middle of a chainging tree, we really don't need text change events for asynch changes anyway, their usecase is for editing

Comment 10

[Aaron Leventhal]

Attachment: Alternative approach -- allow a new parent to adopt children. We may decide to use both approaches, because 1st approach will improve perf by not firing events we dont' really need

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> we really don't need text change events for asynch changes
> anyway
> 

Probably it's better to have own patch for this? It would be easier to review.

Comment 12

[Aaron Leventhal]

Attachment: Better patch

Comment 13

[Aaron Leventhal]

Attachment: Better patch

Comment 14

[alexander :surkov (:asurkov)]

Aaron, why did you remove debug code?

Comment 15

[Aaron Leventhal]

Comment on attachment 287142 [details] [diff] [review]
Better patch

Debugging code is being removed because that is a valid case now. But instead of saying we're "stealing" the child we're adopting it now. It's handled in the CacheChildren() code path. The SetParent() is always called right after SetFirstChild/SetNextSibling.

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 287142 [details] [diff] [review]
Better patch

looks fine with me. I would have a XXX section here to mention we should remove adoption if we will freeze the tree and you leaved comment in SetFirstChild, why?

Comment 17

[Aaron Leventhal]

I can add a comment that we may remove adoption if we go with tree freezing, but I don't want to decide that for sure right now.

Comment 18

[Damon Sicore (:damons)]

Comment on attachment 287142 [details] [diff] [review]
Better patch

a=release drivers.

Comment 19

[Martijn Wargers (dead)]

Thanks for fixing this, Aaron!
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007111404 Minefield/3.0b2pre
testing all attached testcases.