543961 - nsAccessibilityService::GetAccessible() shouldn't try to get document accessible from global cache twice

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

nsAccessibilityService::GetAccessible() shouldn't try to get document accessible from global cache if it's unable to get it from GetCachedAccessNode because the document accessible cache itself in own cache.

It's not good the document is cached twice in the document cache and in global cache. So we might want to fix this.

Ideally I would like GetCachedAccessNode() should return an accessible if it was cached somewhere.

Comment 1

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

(In reply to comment #0)
> Ideally I would like GetCachedAccessNode() should return an accessible if it
> was cached somewhere.

A util method?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> (In reply to comment #0)
> > Ideally I would like GetCachedAccessNode() should return an accessible if it
> > was cached somewhere.
> 
> A util method?

It's kind of util aready :) I didn't get your question.

Comment 3

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

(In reply to comment #2)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > Ideally I would like GetCachedAccessNode() should return an accessible if it
> > > was cached somewhere.
> > 
> > A util method?
> 
> It's kind of util aready :) I didn't get your question.

OK that's fine. Why isn't the method static now?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)

> OK that's fine. Why isn't the method static now?

It can be I think.

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch

I think it doesn't make sense to keep the doc accessible in two caches. Removing it from the document cache.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > (In reply to comment #0)
> > > > Ideally I would like GetCachedAccessNode() should return an accessible if it
> > > > was cached somewhere.
> > > 
> > > A util method?
> > 
> > It's kind of util aready :) I didn't get your question.
> 
> OK that's fine. Why isn't the method static now?

I think it's not necessary since we guarantee only one instance of nsAccessibilityService exists.

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 425011 [details] [diff] [review]
patch

something wrong with my patch

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 9

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

Comment on attachment 426217 [details] [diff] [review]
patch2

r=me thanks.

>-  // Check to see if we already have an accessible for this
>-  // node in the cache
>-  nsCOMPtr<nsIAccessNode> accessNode;
>-  GetCachedAccessNode(aNode, aWeakShell, getter_AddRefs(accessNode));
>+  // Check to see if we already have an accessible for this node in the cache.
>+  nsCOMPtr<nsIAccessNode> cachedAccessNode =
>+    GetCachedAccessNode(aNode, aWeakShell);
>+  if (cachedAccessNode) {
>+    // XXX: the cache might contain the access node for the DOM node but
>+    // accessible because of wrong cache update. In this case try to create new
>+    // accessible.

Please change "DOM node but" to "DOM node that is not"

>+already_AddRefed<nsIAccessNode>
>+nsDocAccessible::GetCachedAccessNode(void *aUniqueID)
> {
>-  GetCacheEntry(mAccessNodeCache, aUniqueID, aAccessNode); // Addrefs for us
>+  nsIAccessNode* accessNode = nsnull;
>+  GetCacheEntry(mAccessNodeCache, aUniqueID, &accessNode);
>+
>+  // No accessible in the cache, check if the given ID is unique ID of this
>+  // document accesible.
>+  if (!accessNode) {
>+    void* thisUniqueID = nsnull;
>+    GetUniqueID(&thisUniqueID);
>+    if (thisUniqueID == aUniqueID) {
>+      NS_ADDREF(this);
>+      return this;
>+    }
>+  }

Hrmmm ok... should be quick enough. Cool.

> #define NS_DOCACCESSIBLE_IMPL_CID                       \
>-{  /* 5641921c-a093-4292-9dca-0b51813db57d */           \
>-  0x5641921c,                                           \
>-  0xa093,                                               \
>-  0x4292,                                               \
>-  { 0x9d, 0xca, 0x0b, 0x51, 0x81, 0x3d, 0xb5, 0x7d }    \
>+{  /* 6baed0f5-3285-4a84-88b0-004fddad7f03 */           \
>+  0x6baed0f5,                                           \
>+  0x3285,                                               \
>+  0x4a84,                                               \
>+  { 0x88, 0xb0, 0x00, 0x4f, 0xdd, 0xad, 0x7f, 0x03 }    \
> }

Not sure this is necessary but probably doesn't hurt anything.

Comment 10

[alexander :surkov (:asurkov)]

try-server build - http://build.mozilla.org/tryserver-builds/[e-mail]-try-d04ebbeae973

I'm not sure why but this patch makes mochitests faster into 2 times. The trunk build takes 15 minutes on my Mac, the patched build takes 7 minutes.

Comment 11

[Marco Zehe (:MarcoZ)]

Comment on attachment 426217 [details] [diff] [review]
patch2

r+ for functionality. I tried several constellations of documents in different tabs and windows, and they all work as expected still with JAWS and NVDA.

Comment 12

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

(In reply to comment #10)
> try-server build -
> http://build.mozilla.org/tryserver-builds/[e-mail]-try-d04ebbeae973
> 
> I'm not sure why but this patch makes mochitests faster into 2 times. The trunk
> build takes 15 minutes on my Mac, the patched build takes 7 minutes.

I expected a performance win, but that seems quite substantial -- although our cache is a hot path.

Great!

Comment 13

[alexander :surkov (:asurkov)]

Great.

Landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/72b38a63ccc6