429617 - Crash [@ nsAccessNode::ClearCacheEntry(void const*, nsCOMPtr<nsIAccessNode>&, void*) ]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

Found these happening in 3.0b5.
http://crash-stats.mozilla.com/report/list?range_unit=weeks&query_search=signature&query_type=contains&product=Firefox&platform=windows%2Cmac%2Clinux&version=Firefox%3A3.0b5%2CFirefox%3A3.0pre&branch=1.9&signature=nsAccessNode%3A%3AClearCacheEntry(void+const*%2C+nsCOMPtr%3CnsIAccessNode%3E%26%2C+void*)&query=Access&range_value=1

The crash stats imply this spot:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/accessible/src/base/nsAccessNode.cpp&rev=1.77&mark=797#797

Comment 1

[Damon Sicore (:damons)]

Doesn't look like those crashes are appearing any longer, or am I reading the graph incorrectly?  

Please re-nom when we have more information on this crash.

Comment 2

[Marco Zehe (:MarcoZ)]

Crash stats for 3.0b5 still shows at least 1 crash per day, none however for nightlies. Last report is bp-5f700230-1154-11dd-903f-001321b13766, pointing to
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/accessible/src/base/nsAccessNode.cpp&rev=1.77&mark=797#797
Looks like privateAccessNode is being used without checking if the QI succeeded.

Comment 3

[Marco Zehe (:MarcoZ)]

Attachment: NULL check

Comment 4

[alexander :surkov (:asurkov)]

Marco, every accessible object implements nsPIAccessNode, at least it must.

Comment 5

[Marco Zehe (:MarcoZ)]

(In reply to comment #4)
> Marco, every accessible object implements nsPIAccessNode, at least it must.

Then why is this one second on our top crashers in b5 in the last week alone?

Comment 6

[Aaron Leventhal]

Pointer could be bad. But as Surkov says it's impossible for an accessible not to support that. (BTW in the future I'd rather we just use nsRefPtr<nsAccessNode> and nsRefPtr<nsAccessible> etc. instead of these nsPI interfaces.

Comment 7

[Aaron Leventhal]

Could the passed-in node be null already?

Comment 8

[Marco Zehe (:MarcoZ)]

(In reply to comment #7)
> Could the passed-in node be null already?

I'm ready to bet on anything. :-) Would this indicate a race condition, or would it simply mean that the processing in nsRootAccessible.cpp, which is indicated by the above crash report, is already dealing with a NULL pointer?

Comment 9

[Marco Zehe (:MarcoZ)]

More info: Several stack traces also point to this line:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/accessible/src/base/nsAccessNode.cpp&rev=1.77&mark=805#805

Comment 10

[Marco Zehe (:MarcoZ)]

Attachment: Alternative NULL pointer check

Check if aAccessNode is not NULL instead.

Comment 11

[Marco Zehe (:MarcoZ)]

Lines that initiate the shutdown are either:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/accessible/src/base/nsRootAccessible.cpp&rev=1.265&mark=626#626
or
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/accessible/src/base/nsDocAccessible.cpp&rev=1.238&mark=616#616
There are also two stack traces that are inconclusive, but the two above appear at least twoce each for the past week.

Comment 12

[Aaron Leventhal]

Comment on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

Alexander, will you investigate this more please?

Comment 13

[alexander :surkov (:asurkov)]

Really I haven't idea why the cache entry may be null. Nowhere we put null to the cache. Since we haven't way to reproduce this then I would suggest to put additionally assertion.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

r=me - it's better to be safe

Comment 15

[Marco Zehe (:MarcoZ)]

Comment on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

Second on the accessibility crashers for beta 5. The only reason I can think of why this hasn't surfaced in 3.0pre builds at all is that this person(s) are doing something we've never done. Unfortunately, none of the crash reports contain any comments, so we don't have exact steps to reproduce. So all we can do is fix the obvious symptom without being able to reliably cure the underlying cause. Basically, we are trying to clear the cache of a NULL reference.
Risk: It's a simple NULL check.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

a1.9=beltzner

Comment 17

[Marco Zehe (:MarcoZ)]

Checking in accessible/src/base/nsAccessNode.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessNode.cpp,v  <--  nsAccessNode.cpp
new revision: 1.79; previous revision: 1.78
done

Comment 18

[Ginn Chen]

Macro, the assertion you put is reversed.
It should be NS_ASSERTION(aAccessNode, ....)

Please fix it.

Comment 19

[Jason Orendorff [:jorendorff]]

(1.9 triage) Patch approved, but what landed is not the same as the patch:
  http://tinyurl.com/4jddl2  (<--- what landed)

And it contains a bogus assertion as Ginn Chen says.

Comment 20

[Mike Schroepfer]

Comment on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

Removing approval since we are now closed for 1.9.

Comment 21

[Kai Engert (:KaiE:)]

Gavin, thanks for pointing me to this bug.

I'm getting this assertion a lot on my system (local linux debug build, 2 days old):

###!!! ASSERTION: Calling ClearCacheEntry with a NULL pointer!: '!aAccessNode', file /home/kaie/moz/head/mozilla/accessible/src/base/nsAccessNode.cpp, line 797

Comment 22

[Marco Zehe (:MarcoZ)]

OK, am I allowed to check in the fix for the assertion still?

Comment 23

[Reed Loden [:reed]]

Comment on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

This already landed. Approval shouldn't have been removed.

Comment 24

[Marco Zehe (:MarcoZ)]

Checking in accessible/src/base/nsAccessNode.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessNode.cpp,v  <--  nsAccessNode.cpp
new revision: 1.80; previous revision: 1.79
done

Landed assertion fix.

Comment 25

[Mike Schroepfer]

Comment on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

Making sure approval matches the real world.

Comment 26

[AndrewM]

So if this did get checked in for 1.9 in the end, maybe update the whiteboard? :)

Comment 27

[Marco Zehe (:MarcoZ)]

I'm still seeing this one creep up even after the bug fix, even with 3.0RC1. An example report is
bp-095198ca-23ea-11dd-96f6-001cc4e2bf68.

It would still imply that the QI to nsPIAccessNode didn't succeed, even though the rule is that we should have a private access node with each accessible.

This is still the top crasher after a known bug that the NVDA guys are exposing.

Ginn, any ideas?