545465 - clean up the code of accessible initialization and binding to the tree

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

nsAccTreeWalker uses nsAccessiblityService::GetAccessible to get an accessible, some accessible classes rejects some accessible children (for example, nsXULButtonAccessible::CacheChildren()). So the child was rejected but it's in the cache.

GetAccessible() should be more friendly to CacheChildren() so that it shouldn't return accessible that cannot be accessible.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

So at the time of caching children we should determine if each potential child is in fact accessible, and only cache it if it is, correct?

Comment 2

[alexander :surkov (:asurkov)]

correct

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Alexander, what is the severity of having these bogus children in the cache? Are you sure you would want to block a release on this bug?

Comment 4

[alexander :surkov (:asurkov)]

extra memory usage, wrong information returned to AT, huge assertion number. That's wanted for release and I definitely want to address this before release, but I need to deal with other blockings before. Taking into account we don't have much time and we wouldn't block whole Firefox release by this one then I lean towards it shouldn't be blocking.

Comment 5

[alexander :surkov (:asurkov)]

I've changed my mind on blocking status of this bug. That's more than assertions and little extra memory usage. We do accessible tree traversal for accessibles that are not in tree yet so while we are constructing the accessible tree we may start to traverse/construct accessible again. While we should be protected from second reconstruction but it leads to extra perf problems. We should try to stop accessible tree crawling while we initialize an accessible, at this point parent can provide all information needed for accessible initialization. That's a perf and logic vitality issue. We need to rework accessible initialization and children caching code to finish all under-the-hood improvements of accessibility engine ongoing for Firefox 4.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Approving blocking based on comment 5 and IRC discussion.

It is a mistake to walk the accessible tree during the initialization of an accessible object and I worry about the unknown side effects. Assigning to Alexander.

Comment 7

[alexander :surkov (:asurkov)]

changing summary per comment #5

Comment 8

[alexander :surkov (:asurkov)]

Attachment: part1: don't keep rejected accessible in the cache

Comment 9

[alexander :surkov (:asurkov)]

Attachment: part2: don't travese tree in Init

Comment 10

[Marco Zehe (:MarcoZ)]

Comment on attachment 487952 [details] [diff] [review]
part2: don't travese tree in Init

So the idea about the test is that setting and removing the onclick handler alone should not change the accessible, but the action, and inserting the role should cause a tree update, right? What was the behavior before this patch?

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> Comment on attachment 487952 [details] [diff] [review]
> part2: don't travese tree in Init
> 
> So the idea about the test is that setting and removing the onclick handler
> alone should not change the accessible, but the action,

Actually the accessible tree is recreated in these cases. So the patch doesn't affect on things here.

> and inserting the role
> should cause a tree update, right? What was the behavior before this patch?

But role changing makes accessible created for span but text accessible isn't recreated and it gets adopted by new accessible (in opposite to onclick setting/removement on accessible element). So this test fails prior this patch.

Comment 12

[Marco Zehe (:MarcoZ)]

Comment on attachment 487952 [details] [diff] [review]
part2: don't travese tree in Init

Thanks! r=me for the tests.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 487905 [details] [diff] [review]
part1: don't keep rejected accessible in the cache

> nsHTMLLIAccessible::
>   nsHTMLLIAccessible(nsIContent* aContent, nsIWeakReference* aShell) :
>   nsHyperTextAccessibleWrap(aContent, aShell)
> {
>   nsBlockFrame* blockFrame = do_QueryFrame(GetFrame());
>   if (blockFrame && !blockFrame->BulletIsEmptyExternal()) {
>     mBulletAccessible = new nsHTMLListBulletAccessible(mContent, mWeakShell);
>-    if (mBulletAccessible)
>-      mBulletAccessible->Init();
>+    GetDocAccessible()->BindToDocument(mBulletAccessible, nsnull);
>   }

Why don't we call BindToDocument in the nsHypertextAccessible constructor?

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Nevermind :)

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 487905 [details] [diff] [review]
part1: don't keep rejected accessible in the cache

r=me

Optional:
Aside: I suppose we could SetRoleMapEntry in Init?

>+bool
>+nsDocAccessible::BindToDocument(nsAccessible* aAccessible,
>+                                nsRoleMapEntry* aRoleMapEntry)

I can't think of a better name after all.

>+  // Put into unique ID cache.
>+  if (!mAccessibleCache.Put(aAccessible->UniqueID(), aAccessible)) {
>+    if (aAccessible->IsPrimaryForNode())
>+      mNodeToAccessibleMap.Remove(aAccessible->GetNode());
>+    return false;
>+  }

Aside: I would prefer {} around the single line if block ;)

>+void
>+nsDocAccessible::UnbindFromDocument(nsAccessible* aAccessible)
>+{
>+  // Remove an accessible from node to accessible map if it is presented there.

Maybe:  Remove an accessible from node-to-accessible map if it exists there.


>+#ifdef DEBUG
>+  NS_ASSERTION(mAccessibleCache.GetWeak(aAccessible->UniqueID()),
>+               "Illegitimate illegitimated accessible!");

Nit: I find this message confusing can you reword it? :)

>+#endif

>+
>+  void* uniqueID = aAccessible->UniqueID();
>+  aAccessible->Shutdown();
>+  mAccessibleCache.Remove(uniqueID);

Why not Shutdown after removal?

>diff --git a/accessible/src/base/nsDocAccessible.h b/accessible/src/base/nsDocAccessible.h
>--- a/accessible/src/base/nsDocAccessible.h
>+++ b/accessible/src/base/nsDocAccessible.h

>   /**
>-   * Shutdown the accessible and remove it from document cache.
>+   * Remove the existing accessible from document caches and shutdown it.

Maybe: Remove from document and shutdown the given accessible.
(Note: maybe we should call it UnbindAndShutdown)

>@@ -109,41 +110,35 @@ nsHTMLImageMapAccessible::CacheChildren(

>+  nsDocAccessible* document = GetDocAccessible();

Can we put error check here, just in case (before we dereference the document ptr)?

>+++ b/accessible/src/xul/nsXULTreeAccessible.cpp
>@@ -467,61 +467,58 @@ nsXULTreeAccessible::GetTreeItemAccessib
>     return nsnull;
> 
>   PRInt32 rowCount = 0;
>   nsresult rv = mTreeView->GetRowCount(&rowCount);
>   if (NS_FAILED(rv) || aRow >= rowCount)
>     return nsnull;
> 
>   void *key = reinterpret_cast<void*>(aRow);
>-  nsRefPtr<nsAccessible> accessible = mAccessibleCache.GetWeak(key);
>+  nsAccessible* cachedTreeItem = mAccessibleCache.GetWeak(key);
>+  if (cachedTreeItem)
>+    return cachedTreeItem;
> 
>-  if (!accessible) {
>-    accessible = CreateTreeItemAccessible(aRow);
>-    if (!accessible)
>-      return nsnull;
>+  nsRefPtr<nsAccessible> treeItem = CreateTreeItemAccessible(aRow);
>+  if (treeItem) {
>+    if (mAccessibleCache.Put(key, treeItem)) {
>+      if (GetDocAccessible()->BindToDocument(treeItem, nsnull))
>+        return treeItem;
> 
>-    if (!accessible->Init()) {
>-      accessible->Shutdown();
>-      return nsnull;
>+      mAccessibleCache.Remove(key);
>     }

This is much nicer to read. I would prefer {} around the return though ;)

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)

> Optional:
> Aside: I suppose we could SetRoleMapEntry in Init?

yes, though we should change Init() to take an argument, but SetRoleMapEntry can be used separately from Init() in the case of document accessibles. Perhaps it's ok to call SetRoleMapEntry outside of Init().

> >+  // Put into unique ID cache.
> Aside: I would prefer {} around the single line if block ;)

I'll put empty line before return false ;)

> >+#ifdef DEBUG
> >+  NS_ASSERTION(mAccessibleCache.GetWeak(aAccessible->UniqueID()),
> >+               "Illegitimate illegitimated accessible!");
> 
> Nit: I find this message confusing can you reword it? :)

Unbinding the unbound accessible. That's ok?

> >+#endif
> 
> >+
> >+  void* uniqueID = aAccessible->UniqueID();
> >+  aAccessible->Shutdown();
> >+  mAccessibleCache.Remove(uniqueID);
> 
> Why not Shutdown after removal?

I'm trying to not Shutdown destroyed accessible.

> (Note: maybe we should call it UnbindAndShutdown)

I'd like BindToDocument and UnbindFromDocument similar. I'm fine with it but we should change BindToDocument then?

> >@@ -109,41 +110,35 @@ nsHTMLImageMapAccessible::CacheChildren(
> 
> >+  nsDocAccessible* document = GetDocAccessible();
> 
> Can we put error check here, just in case (before we dereference the document
> ptr)?

We should be safe, not defunct accessible doesn't exists outside the document.

Comment 17

[alexander :surkov (:asurkov)]

landed 

http://hg.mozilla.org/mozilla-central/rev/b22e3d33e8b1
http://hg.mozilla.org/mozilla-central/rev/3e941917e221