Closed Bug 394404 Opened 13 years ago Closed 13 years ago

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

Categories

(Core :: Disability Access APIs, defect)

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: aaronlev)

References

Details

(Keywords: crash, regression, testcase)

Crash Data

Attachments

(5 files, 3 obsolete files)

Attached file 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
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.
Assignee: aaronleventhal → nobody
Component: Disability Access APIs → Layout
QA Contact: accessibility-apis → layout
Attached file Smaller testcase
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
Backing out the patch from bug 391846 makes the crash go away in my debug build.
Blocks: 391846
Thanks Martijn, I'll look.
Assignee: nobody → aaronleventhal
Component: Layout → Disability Access APIs
QA Contact: layout → accessibility-apis
Blocks: fox3access
Attached file testcase3
(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.
Attachment #287001 - Flags: review?(surkov.alexander)
(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.
Attached patch Better patch (obsolete) — Splinter Review
Attachment #287001 - Attachment is obsolete: true
Attachment #287002 - Attachment is obsolete: true
Attachment #287120 - Flags: review?(surkov.alexander)
Attachment #287001 - Flags: review?(surkov.alexander)
Attachment #287002 - Flags: review?(surkov.alexander)
Attached patch Better patchSplinter Review
Attachment #287120 - Attachment is obsolete: true
Attachment #287142 - Flags: review?(surkov.alexander)
Attachment #287120 - Flags: review?(surkov.alexander)
Aaron, why did you remove debug code?
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 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?
Attachment #287142 - Flags: review?(surkov.alexander) → review+
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.
Attachment #287142 - Flags: approval1.9?
Comment on attachment 287142 [details] [diff] [review]
Better patch

a=release drivers.
Attachment #287142 - Flags: approval1.9? → approval1.9+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
No longer depends on: 403724
Depends on: 403724
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.
Status: RESOLVED → VERIFIED
Crash Signature: [@ nsQueryInterface::operator]
You need to log in before you can comment on or make changes to this bug.