Closed Bug 545465 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: access, memory-footprint, perf)

Attachments

(2 files)

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.
Blocks: 592189
blocking2.0: --- → ?
Version: unspecified → Trunk
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?
correct
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?
blocking2.0: ? → -
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.
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.
blocking2.0: - → ?
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.
Assignee: nobody → surkov.alexander
blocking2.0: ? → betaN+
Blocks: 576931
No longer blocks: 592189
changing summary per comment #5
Status: NEW → ASSIGNED
Summary: do not cache accessibles rejected by CacheChildren → clean up the code of accessible initialization and binding to the tree
Attachment #487952 - Flags: review?(marco.zehe)
Attachment #487952 - Flags: review?(ginn.chen)
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?
(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 on attachment 487952 [details] [diff] [review]
part2: don't travese tree in Init

Thanks! r=me for the tests.
Attachment #487952 - Flags: review?(marco.zehe) → review+
Attachment #487905 - Flags: review?(bolterbugz)
Attachment #487952 - Flags: review?(bolterbugz)
Attachment #487952 - Flags: review?(ginn.chen) → review+
Attachment #487905 - Flags: review?(ginn.chen) → review+
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 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 ;)
Attachment #487905 - Flags: review?(bolterbugz) → review+
Attachment #487952 - Flags: review?(bolterbugz) → review+
(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.
landed 

http://hg.mozilla.org/mozilla-central/rev/b22e3d33e8b1
http://hg.mozilla.org/mozilla-central/rev/3e941917e221
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
Target Milestone: --- → mozilla2.0b8
You need to log in before you can comment on or make changes to this bug.