458058 - Crash [@ CallQueryInterface<nsINode, nsIDOMElement>(nsINode*, nsIDOMElement**) ]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

I ran into this at least twice a day over the past 5 days or so. Reports are:
bp-4d928dd9-8f99-11dd-a004-001a4bd43ed6
bp-0179e554-8eb8-11dd-9bfd-001cc4e2bf68

They both point at a line in nsAccUtils::GetDomElementFor. And they both come from nsAccessNode::GetComputedStyleDeclaration.

Observation: I always used JAWS 10 public beta 2 (build 10.0.396) when these crashes occurred.

Alex, since these both point at a line near to where crashes after fix for bug 454997 also pointed to, these might be related.

Comment 1

[alexander :surkov (:asurkov)]

fixed in bug 454997

Comment 2

[Marco Zehe (:MarcoZ)]

Still seeing crashes in this area like
bp-d57a883d-97b5-11dd-b5bc-0013211cbf8a

in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081011 Minefield/3.1b2pre

Comment 3

[Marco Zehe (:MarcoZ)]

And this report, too:
bp-1d6cbd40-9827-11dd-998d-001a4bd43e5c

Comment 4

[Marco Zehe (:MarcoZ)]

Most reliable way to reproduce:
1. Have JAWS 10 running.
2. Go to twitter.com. I have it set to automatically log in, so I immediately get to the page where I can enter my update.
3. Often, when the page comes up, Minefield will crash with stack traces like the two quoted in the previous comments.

Comment 5

[alexander :surkov (:asurkov)]

What version of Jaws build do you use? I can't reproduce the crash.

Comment 6

[alexander :surkov (:asurkov)]

One idea I have is text node has been removed but we didn't invalidated the cache. Possibly related with nsAccessNode caching if text node is hidden.

Comment 7

[Marco Zehe (:MarcoZ)]

New crashes with 20081029 build:
bp-d6d08997-a5c7-11dd-8d28-001a4bd43ef6 and similar. All these were captured from doing this:

1. With JAWS 10 public beta 3 or later running, go to http://www.nvda-project.org/ticket/215.
2. Log in (or create an account if you don't have one, then log in).
3. Once the page is reloaded with the additional possibilities of a logged-in user, Firefox crashes.
Any ticket such as http://www.nvda-project.org/ticket/138 will also cause these crashes if you checked the box to automatically log in.

Comment 8

[timeless]

Signature	CallQueryInterface<nsIDOMHTMLElement, nsIDOMElement>(nsIDOMHTMLElement*, nsIDOMElement**)
UUID	d6d08997-a5c7-11dd-8d28-001a4bd43ef6
Time	2008-10-29 07:41:56-07
Uptime	119
Product	Firefox
Version	3.1b2pre
Build ID	20081029033642
OS	Windows NT
OS Version	5.1.2600 Service Pack 3
CPU	x86
CPU Info	GenuineIntel family 6 model 15 stepping 11
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x0
Comments	
Crashing Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	CallQueryInterface<nsIDOMHTMLElement,nsIDOMElement> 	obj-firefox/dist/include/xpcom/nsISupportsUtils.h:203
1 	xul.dll 	nsCoreUtils::GetDOMElementFor 	accessible/src/base/nsCoreUtils.cpp:171
2 	xul.dll 	nsCoreUtils::GetComputedStyleDeclaration 	accessible/src/base/nsCoreUtils.cpp:724
3 	xul.dll 	nsAccessNodeWrap::get_computedStyleForProperties 	accessible/src/msaa/nsAccessNodeWrap.cpp:356

the crash address of 0x0 means that:

   171    CallQueryInterface(node->GetNodeParent(), &element);

crashed because node->GetNodeParent() returned null.

Comment 9

[Marco Zehe (:MarcoZ)]

Timeless, I know. The question is how do we fix it. Do we simply wall-paper fix it by doing a NULL check, or do we find out the root cause for this and make sure every text node has a parent? Because that's what this says: A text node has no parent, which is close to impossible unless the cache is not invalidated correctly. I can put together a wallpaper fix in 2 minutes, but this doesn't cure the root cause.

Comment 10

[Marco Zehe (:MarcoZ)]

Attachment: Add null check

Check if the parent is valid before doing CallQueryInterface. Fixes the crashes.

Comment 11

[Aaron Leventhal]

Comment on attachment 345339 [details] [diff] [review]
Add null check

The } needs to be moved back 2 spaces.

Comment 12

[alexander :surkov (:asurkov)]

Marco, please add assertion.

Comment 13

[Marco Zehe (:MarcoZ)]

Pushed with Aaron's nit and Surkov's request in changeset:
http://hg.mozilla.org/mozilla-central/rev/4ea96420ec0f

Comment 14

[Steve England [:stevee]]

A guy on IRC is complaining of a crash on paypal.

The stack is http://crash-stats.mozilla.com/report/index/169c0eb3-0331-46e9-a7a2-d9ab82090214?p=1

Frame  	Module  	Signature [Expand]  	Source
0 	xul.dll 	nsAccUtils::GetDOMElementFor 	mozilla/accessible/src/base/nsAccessibilityUtils.cpp:1115
1 	xul.dll 	nsAccessNode::GetComputedStyleDeclaration 	mozilla/accessible/src/base/nsAccessNode.cpp:645
2 	xul.dll 	nsHTMLTableAccessible::IsProbablyForLayout 	mozilla/accessible/src/html/nsHTMLTableAccessible.cpp:1144
3 	xul.dll 	nsHTMLTableAccessible::GetAttributesInternal 	mozilla/accessible/src/html/nsHTMLTableAccessible.cpp:233
4 	xul.dll 	nsCOMPtr<nsIPersistentProperties>::operator= 	nsCOMPtr.h:780
5 	xul.dll 	nsAccessible::GetAttributes 	mozilla/accessible/src/base/nsAccessible.cpp:2064
6 	xul.dll 	nsAccessibleWrap::get_attributes 	mozilla/accessible/src/msaa/nsAccessibleWrap.cpp:1580
7 	FSDomNodeFirefox.DLL 	FSDomNodeFirefox.DLL@0x3f49 	

1) Is this crash covered by this bug?
2) Is this fix in the 1.9.1 branch?

Comment 15

[alexander :surkov (:asurkov)]

It isnt' seemed to me patch of the bug address problem shown by this stack trace. I think we might want to put null check into start of method nsAccUtils::GetDOMElementFor(nsIDOMNode *aNode) and put an assertion or precondition to notify about incorrect usage of the method.