Closed Bug 429617 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Disability Access APIs, defect, major)

x86
Windows XP
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: MarcoZ, Assigned: MarcoZ)

References

Details

(Keywords: access, crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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.
Flags: blocking1.9? → blocking1.9-
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.
Attached patch NULL check (obsolete) — Splinter Review
Assignee: aaronleventhal → marco.zehe
Status: NEW → ASSIGNED
Attachment #317476 - Flags: review?(aaronleventhal)
Marco, every accessible object implements nsPIAccessNode, at least it must.
(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?
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.
Could the passed-in node be null already?
(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?
Check if aAccessNode is not NULL instead.
Attachment #317476 - Attachment is obsolete: true
Attachment #317487 - Flags: review?(aaronleventhal)
Attachment #317476 - Flags: review?(aaronleventhal)
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 on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

Alexander, will you investigate this more please?
Attachment #317487 - Flags: review?(aaronleventhal) → review?(surkov.alexander)
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 on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

r=me - it's better to be safe
Attachment #317487 - Flags: review?(surkov.alexander) → review+
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.
Attachment #317487 - Flags: approval1.9?
Comment on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

a1.9=beltzner
Attachment #317487 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Macro, the assertion you put is reversed.
It should be NS_ASSERTION(aAccessNode, ....)

Please fix it.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Whiteboard: [missed 1.9 checkin]
Comment on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

Removing approval since we are now closed for 1.9.
Attachment #317487 - Flags: approval1.9+
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

OK, am I allowed to check in the fix for the assertion still?
Comment on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

This already landed. Approval shouldn't have been removed.
Attachment #317487 - Flags: approval1.9?
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Comment on attachment 317487 [details] [diff] [review]
Alternative NULL pointer check

Making sure approval matches the real world.
Attachment #317487 - Flags: approval1.9? → approval1.9+
So if this did get checked in for 1.9 in the end, maybe update the whiteboard? :)
Whiteboard: [missed 1.9 checkin]
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?
Crash Signature: [@ nsAccessNode::ClearCacheEntry(void const*, nsCOMPtr<nsIAccessNode>&, void*) ]
You need to log in before you can comment on or make changes to this bug.