Accessibility cache becomes corrupt leading to crash

RESOLVED FIXED

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

({access, fixed1.8, sec508})

Trunk
x86
Windows XP
access, fixed1.8, sec508
Points:
---
Bug Flags:
blocking1.8b5 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

12 years ago
Recent talkback reports show crashes in nsAccessible::GetNextSibling()

When I ran the testcase I saw an assertion that some elements being initialized
and put in the cache were already in there.

In turns out that we were creating an entry in the current document's cache for
calls into get_accFocus(), even though the focused element may be outside of the
current document.

After speaking with AT vendors, they want get_accFocus() to return the current
focused element, even if it is not in the current document. We just need to make
sure it is cached under the right doc accessible.
(Assignee)

Comment 1

12 years ago
The test case is:
Run Window-Eyes 5.5 beta (you can get it from www.gwmicro.com)
Arrow through the Tools menu
Go to Help -> About 
Hit Okay
Go to Help -> About a 2nd time
Flags: blocking1.8b5?
(Assignee)

Comment 2

12 years ago
Created attachment 195299 [details] [diff] [review]
1) Create focused accessible in document/presshell it belongs in, 2) Add safeguards to prevent this from happening again, 3) Fix another talkback crash where infinite reentrancy caused a stack overflo

Corrupted cached talkback report:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=9015268


Reentrancy overflow:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=9125014


I couldn't see from the stack of the reentrancy overflow how it could really
happen, but that's what was happening. What's strange is that it seemed to be
treating have created an HTML4 <button> accessible for an <input
type="button">, but I can't reproduce that one or see how it would occur. The
reentrance check should fix it though, and I've only seen that crash once in
the history of talkback reports.
Attachment #195299 - Flags: superreview?(jst)
Attachment #195299 - Flags: review?(parente)
(Assignee)

Updated

12 years ago
Severity: normal → critical

Comment 3

12 years ago
Comment on attachment 195299 [details] [diff] [review]
1) Create focused accessible in document/presshell it belongs in, 2) Add safeguards to prevent this from happening again, 3) Fix another talkback crash where infinite reentrancy caused a stack overflo

Are we talking about thread re-entrancy here? If so, is the line "isAlreadyHere
= PR_TRUE;" guaranteed to be an atomic op? If not, re-entrancy can still occur.
(Assignee)

Comment 4

12 years ago
(In reply to comment #3)
> (From update of attachment 195299 [details] [diff] [review] [edit])
> Are we talking about thread re-entrancy here? If so, is the line "isAlreadyHere
> = PR_TRUE;" guaranteed to be an atomic op? If not, re-entrancy can still occur.
> 

No, sorry not thread reentrancy. It's just getting in an infinite loop.
See the stack for this report:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=9125014
Comment on attachment 195299 [details] [diff] [review]
1) Create focused accessible in document/presshell it belongs in, 2) Add safeguards to prevent this from happening again, 3) Fix another talkback crash where infinite reentrancy caused a stack overflo

sr=jst, but:

- In nsAccessible::AppendFlatStringFromSubtree():

+  static isAlreadyHere;
+  NS_ASSERTION(!isAlreadyHere, "How did reentrancy happen? It shouldn't");
+  if (isAlreadyHere) {
+    return NS_OK; // Prevent stack overflow from reentrency
+  }
+  isAlreadyHere = PR_TRUE;
   nsresult rv = AppendFlatStringFromSubtreeRecurse(aContent, aFlatString);
+  isAlreadyHere = PR_FALSE;

Is this really the right fix for this re-entrancy? Isn't the problem rather the
fact that this method is called for a node from within a call to this method
for the same node? I.e. shouldn't the caller be fixed instead?

sr=jst with that figured out.
Attachment #195299 - Flags: superreview?(jst) → superreview+

Comment 6

12 years ago
Comment on attachment 195299 [details] [diff] [review]
1) Create focused accessible in document/presshell it belongs in, 2) Add safeguards to prevent this from happening again, 3) Fix another talkback crash where infinite reentrancy caused a stack overflo

(In reply to comment #5)
> Is this really the right fix for this re-entrancy? Isn't the problem rather the
> fact that this method is called for a node from within a call to this method
> for the same node? I.e. shouldn't the caller be fixed instead?
> 
> sr=jst with that figured out.

Yes. This has me confused too. Shouldn't AppendFlatStringFromSubtreeRecurse be
fixed so it doesn't call AppendFlatStringFromSubtree when it shouldn't? It just
looks like the check for the terminal condition of the recursion is faulty.

r=parente assuming the right fix
Attachment #195299 - Flags: review?(parente) → review+
(Assignee)

Comment 7

12 years ago
I'll just leave that part of the fix out when I check in, and I'll see if it
comes up again with a reproduceable testcase, because I can't figure out how to
fix it the "right" way from the info I have. The crash has only ever happened
once in talkback reports. This was just to fix it in whatever way I could.

If you look at the stack trace below you see that we're calling GetName from
AppendFlatStringFromContentNode.
Funny thing is, the code for AppendFlatStringFromContentNode never calls GetName.

nsAccessible::AppendFlatStringFromContentNode 
nsAccessible::AppendFlatStringFromSubtreeRecurse 
nsAccessible::AppendFlatStringFromSubtree
nsAccessible::GetHTMLName
nsHTML4ButtonAccessible::GetName
nsAccessible::AppendFlatStringFromContentNode 
nsAccessible::AppendFlatStringFromSubtreeRecurse 
nsAccessible::AppendFlatStringFromSubtree
nsAccessible::GetHTMLName
nsHTML4ButtonAccessible::GetName  
(Assignee)

Comment 8

12 years ago
Comment on attachment 195299 [details] [diff] [review]
1) Create focused accessible in document/presshell it belongs in, 2) Add safeguards to prevent this from happening again, 3) Fix another talkback crash where infinite reentrancy caused a stack overflo

Seeking a= for the the parts Johnny and Peter didn't have any issues with (not
the nsAccessible.cpp part).
Attachment #195299 - Flags: approval1.8b5?
(Assignee)

Comment 9

12 years ago
I checked in everything but the nsAccessible.cpp recursion check.

Could the talkback stack have occured because of some kind of memory corruption
which changed the code?
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Attachment #195299 - Flags: approval1.8b5? → approval1.8b5+

Updated

12 years ago
Flags: blocking1.8b5? → blocking1.8b5+

Updated

12 years ago
Keywords: fixed1.8
You need to log in before you can comment on or make changes to this bug.