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

VERIFIED FIXED

Status

()

Core
Disability Access APIs
--
critical
VERIFIED FIXED
11 years ago
7 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Aaron Leventhal)

Tracking

({crash, regression, testcase})

Trunk
x86
Windows XP
crash, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 279050 [details]
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
(Assignee)

Comment 1

11 years ago
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
(Reporter)

Comment 2

11 years ago
Created attachment 279313 [details]
Smaller testcase
(Reporter)

Comment 3

11 years ago
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
(Reporter)

Comment 4

11 years ago
Backing out the patch from bug 391846 makes the crash go away in my debug build.
Blocks: 391846
(Assignee)

Comment 5

11 years ago
Thanks Martijn, I'll look.
Assignee: nobody → aaronleventhal
Component: Layout → Disability Access APIs
QA Contact: layout → accessibility-apis
(Assignee)

Updated

11 years ago
Blocks: 396346
(Reporter)

Comment 6

11 years ago
Created attachment 282330 [details]
testcase3

Comment 7

11 years ago
(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.
(Assignee)

Comment 8

10 years ago
Created attachment 286985 [details]
Even smaller testcase -- assertion "Stealing child", crashes upon reload
(Assignee)

Comment 9

10 years ago
Created attachment 287001 [details] [diff] [review]
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
(Assignee)

Comment 10

10 years ago
Created attachment 287002 [details] [diff] [review]
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
Attachment #287002 - Flags: review?(surkov.alexander)
(Assignee)

Updated

10 years ago
Attachment #287001 - Flags: review?(surkov.alexander)

Comment 11

10 years ago
(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.
(Assignee)

Comment 12

10 years ago
Created attachment 287120 [details] [diff] [review]
Better patch
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)
(Assignee)

Comment 13

10 years ago
Created attachment 287142 [details] [diff] [review]
Better patch
Attachment #287120 - Attachment is obsolete: true
Attachment #287142 - Flags: review?(surkov.alexander)
Attachment #287120 - Flags: review?(surkov.alexander)

Comment 14

10 years ago
Aaron, why did you remove debug code?
(Assignee)

Comment 15

10 years ago
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

10 years ago
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+
(Assignee)

Comment 17

10 years ago
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.
(Assignee)

Updated

10 years ago
Attachment #287142 - Flags: approval1.9?
Comment on attachment 287142 [details] [diff] [review]
Better patch

a=release drivers.
Attachment #287142 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
No longer depends on: 403724
Depends on: 403724
(Reporter)

Comment 19

10 years ago
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.