348158 - Accessibility cache gets out of sync

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

I have some new test code that helps determine when the accessibility cache is out of sync.

By out of sync, I mean the following:
An accessible's parent does not have the original accessible in its children (mParent->mFirstChild, mParent->mFirstChild->mNextSibling, mParent->mFirstChild->mNextSibling->mNextSibling, etc.)

It should be one of those. I've already seen it broken for a menuitem accessible, and for a listbox accessible in the customize character encodings dialog.

Comment 1

[Aaron Leventhal]

Attachment: Add testing code (does not fix problems, just shows where they are)

Comment 2

[Aaron Leventhal]

Neat, this causes a crash when you do:
Alt+V (view menu)
Right arrow to move to next menu

We have seen some crashes in menuitems because of a corrupted cache but never been able to track them down. I think this will help.

Comment 3

[Ginn Chen]

Could we do it without crash Firefox?
It crashed too bad, I've to switch to another tty to kill it.

Comment 4

[Aaron Leventhal]

Ginn, if you r= it I will check it in with 
#ifdef A11Y_CACHE_TEST

That way it will not be built for you, but you can easily use it.

Comment 5

[Aaron Leventhal]

Attachment: Does not crash (at least for me). Improves testing so it doesn't create side effects, improves caching based on some things I found

Ginn, this patch generally tries to clean up our debug output. It looks long, but most of the changes are: use a temporary var for childCount in CacheChildren() and always use CacheChildren() instead of GetChildCount() to build cache.

I also changed the way mutations are handled so that we don't create extra accessibles when we don't need to.

Also, I didn't like the hack in nsXULMenuitemAccessible::CacheChildren() where we simulate a popup event. That seems to be for Seamonkey where we have the Zoom menuitem who's test changes dynamically. I've removed it. If we don't get the text in Seamonkey for that we can file a separate bug to figure out a clean way to do it.

Comment 6

[Aaron Leventhal]

Getting this in will help avoid instability in the future, as well as issues like bug 348155 (which was caused because only the visible list items had frames).

Comment 7

[Ginn Chen]

Comment on attachment 233121 [details] [diff] [review]
Does not crash (at least for me). Improves testing so it doesn't create side effects, improves caching based on some things I found

1)
+#ifdef DEBUG
+  nsAutoString localName;
+  childNode->GetLocalName(localName);
+  char *hasAccessible = childAccessible ? " (acc)" : "";
+  if (aChangeEventType == nsIAccessibleEvent::EVENT_HIDE) {
+    printf("[Hide %s %s]", NS_ConvertUTF16toUTF8(localName).get(), hasAccessible);
+  }

should use |const char *hasAccessible|, otherwise it won't compile.
I'd like to have "\n" after each "]".
I suggest we use #ifdef A11Y_DEBUG instead for this block.

2) 
   nsAutoString localName;
-  targetNode->GetLocalName(localName);
 #ifdef DEBUG_aleventhal

We should not remove this line.

3)
nsXULMenuAccessible.cpp and nsXULSelectAccessible.cpp
+  if (mAccChildCount == eChildCountUninitialized) {

use
if (mAccChildCount != eChildCountUninitialized)
  return;
is better for CVS history.

Comment 8

[Ginn Chen]

One more thing:
-    case nsIAccessibleProvider::XULTooltip:
-      *aAccessible = new nsXULTooltipAccessible(aNode, weakShell);
-      break;
It doesn't belong to this bug, right?

Comment 9

[Aaron Leventhal]

Ginn, I noticed that tooltips cause the assertion, but I will put them back in and we will find out why. For a moment I thought maybe it's better not to support tooltips, but I have been convinced otherwise.

Comment 10

[Aaron Leventhal]

Testing code has been checked in. Leaving open so we fix the actual problems.

Comment 11

[Aaron Leventhal]

I have not seen this assertion for a while. I don't see it for tooltip as mentioned above. We can open new bugs when we see it again.