307534 - Accessibility cache becomes corrupt leading to crash

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

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.

Comment 1

[Aaron Leventhal]

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

Comment 2

[Aaron Leventhal]

Attachment: 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.

Comment 3

[parente]

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.

Comment 4

[Aaron Leventhal]

(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 5

[Johnny Stenback (:jst)]

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.

Comment 6

[parente]

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

Comment 7

[Aaron Leventhal]

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  

Comment 8

[Aaron Leventhal]

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).

Comment 9

[Aaron Leventhal]

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?

Comment 10

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381