From e4df485906961bd727b6a3fb5d42ed3c5ebd1303 Mon Sep 17 00:00:00 2001 From: Alexander Surkov Date: Thu, 11 Feb 2010 21:56:01 +0800 Subject: [PATCH] Bug 543961 - nsAccessibilityService::GetAccessible() shouldn't try to get document accessible from global cache twice, r=davidb, marcoz --- accessible/public/nsIAccessibleDocument.idl | 11 +-- accessible/public/nsIAccessibleRetrieval.idl | 22 +----- accessible/src/base/nsAccessNode.cpp | 5 +- .../src/base/nsAccessibilityService.cpp | 74 ++++++++----------- accessible/src/base/nsAccessibilityService.h | 11 +++ accessible/src/base/nsDocAccessible.cpp | 40 +++++----- accessible/src/base/nsDocAccessible.h | 12 +++ accessible/src/msaa/nsDocAccessibleWrap.cpp | 11 +-- 8 files changed, 87 insertions(+), 99 deletions(-) diff --git a/accessible/public/nsIAccessibleDocument.idl b/accessible/public/nsIAccessibleDocument.idl index fdbd4b6360a..ab0e878af2c 100644 --- a/accessible/public/nsIAccessibleDocument.idl +++ b/accessible/public/nsIAccessibleDocument.idl @@ -58,7 +58,7 @@ interface nsIDOMWindow; * * @status UNDER_REVIEW */ -[scriptable, uuid(b7ae45bd-21e9-4ed5-a67e-86448b25d56b)] +[scriptable, uuid(427597a3-1737-4743-bf43-2311a1ed5fbd)] interface nsIAccessibleDocument : nsISupports { /** @@ -102,15 +102,6 @@ interface nsIAccessibleDocument : nsISupports */ [noscript] readonly attribute voidPtr windowHandle; - /** - * Returns the access node cached by this document - * @param aUniqueID The unique ID used to cache the node. - * This matches up with the uniqueID attribute on - * nsIAccessNode. - * @return The nsIAccessNode cached for this particular unique ID. - */ - [noscript] nsIAccessNode getCachedAccessNode(in voidPtr aUniqueID); - /** * Returns the first accessible parent of a DOM node. * Guaranteed not to return nsnull if the DOM node is in a document. diff --git a/accessible/public/nsIAccessibleRetrieval.idl b/accessible/public/nsIAccessibleRetrieval.idl index 4f182cf87d8..0b9d03cbca2 100644 --- a/accessible/public/nsIAccessibleRetrieval.idl +++ b/accessible/public/nsIAccessibleRetrieval.idl @@ -56,7 +56,7 @@ interface nsIDOMDOMStringList; * * @status UNDER_REVIEW */ -[scriptable, uuid(7eb49afb-6298-4ce6-816f-9615936540f4)] +[scriptable, uuid(5f5a6f98-835d-4771-8bfe-a0c7cdc85fec)] interface nsIAccessibleRetrieval : nsISupports { /** @@ -101,26 +101,6 @@ interface nsIAccessibleRetrieval : nsISupports * @return The nsIAccessible for the given DOM node. */ nsIAccessible getAccessibleInShell(in nsIDOMNode aNode, in nsIPresShell aPresShell); - - /** - * Return an nsIAccessNode for an already created DOM node in the given weak shell. - * Does not create a new one -- only returns cached access nodes. - * @param aNode The DOM node to get an access node for. - * @param aPresShell The presentation shell which contains layout info for the DOM node. - * @return The nsIAccessNode for the given DOM node or null if - * an access node does not already exist for this DOM node. - */ - nsIAccessNode getCachedAccessNode(in nsIDOMNode aNode, in nsIWeakReference aShell); - - /** - * Return an nsIAccessible for an already created DOM node in the given weak shell. - * Does not create a new one -- only returns cached accessibles. - * @param aNode The DOM node to get an accessible for. - * @param aPresShell The presentation shell which contains layout info for the DOM node. - * @return The nsIAccessible for the given DOM node or null if - * an accessible does not already exist for this DOM node. - */ - nsIAccessible getCachedAccessible(in nsIDOMNode aNode, in nsIWeakReference aShell); /** * Returns accessible role as a string. diff --git a/accessible/src/base/nsAccessNode.cpp b/accessible/src/base/nsAccessNode.cpp index 87d71114458..d8c52f1293b 100644 --- a/accessible/src/base/nsAccessNode.cpp +++ b/accessible/src/base/nsAccessNode.cpp @@ -464,9 +464,8 @@ nsAccessNode::MakeAccessNode(nsIDOMNode *aNode, nsIAccessNode **aAccessNode) { *aAccessNode = nsnull; - nsCOMPtr accessNode; - GetAccService()->GetCachedAccessNode(aNode, mWeakShell, - getter_AddRefs(accessNode)); + nsCOMPtr accessNode = + GetAccService()->GetCachedAccessNode(aNode, mWeakShell); if (!accessNode) { nsCOMPtr accessible; diff --git a/accessible/src/base/nsAccessibilityService.cpp b/accessible/src/base/nsAccessibilityService.cpp index bdc173566bd..7798043de8e 100644 --- a/accessible/src/base/nsAccessibilityService.cpp +++ b/accessible/src/base/nsAccessibilityService.cpp @@ -938,30 +938,19 @@ nsAccessibilityService::CreateHTMLCaptionAccessible(nsIFrame *aFrame, nsIAccessi return NS_OK; } -NS_IMETHODIMP nsAccessibilityService::GetCachedAccessible(nsIDOMNode *aNode, - nsIWeakReference *aWeakShell, - nsIAccessible **aAccessible) -{ - nsCOMPtr accessNode; - nsresult rv = GetCachedAccessNode(aNode, aWeakShell, getter_AddRefs(accessNode)); - nsCOMPtr accessible(do_QueryInterface(accessNode)); - NS_IF_ADDREF(*aAccessible = accessible); - return rv; -} - -NS_IMETHODIMP nsAccessibilityService::GetCachedAccessNode(nsIDOMNode *aNode, - nsIWeakReference *aWeakShell, - nsIAccessNode **aAccessNode) +already_AddRefed +nsAccessibilityService::GetCachedAccessNode(nsIDOMNode *aNode, + nsIWeakReference *aWeakShell) { nsCOMPtr accessibleDoc = nsAccessNode::GetDocAccessibleFor(aWeakShell); - if (!accessibleDoc) { - *aAccessNode = nsnull; - return NS_ERROR_FAILURE; - } + if (!accessibleDoc) + return nsnull; - return accessibleDoc->GetCachedAccessNode(static_cast(aNode), aAccessNode); + nsRefPtr docAccessible = + nsAccUtils::QueryObject(accessibleDoc); + return docAccessible->GetCachedAccessNode(static_cast(aNode)); } NS_IMETHODIMP @@ -1294,24 +1283,20 @@ nsAccessibilityService::GetAccessible(nsIDOMNode *aNode, } #endif - // Check to see if we already have an accessible for this - // node in the cache - nsCOMPtr accessNode; - GetCachedAccessNode(aNode, aWeakShell, getter_AddRefs(accessNode)); - - nsCOMPtr newAcc; - if (accessNode) { - // Retrieved from cache. QI might not succeed if it's a node that's not - // accessible. In this case try to create new accessible because one and - // the same DOM node may be accessible or not in time (for example, - // when it is visible or hidden). - newAcc = do_QueryInterface(accessNode); - if (newAcc) { - NS_ADDREF(*aAccessible = newAcc); + // Check to see if we already have an accessible for this node in the cache. + nsCOMPtr cachedAccessNode = + GetCachedAccessNode(aNode, aWeakShell); + if (cachedAccessNode) { + // XXX: the cache might contain the access node for the DOM node that is not + // accessible because of wrong cache update. In this case try to create new + // accessible. + CallQueryInterface(cachedAccessNode, aAccessible); + NS_ASSERTION(*aAccessible, "Bad cache update!"); + if (*aAccessible) return NS_OK; - } } + nsCOMPtr newAcc; nsCOMPtr content(do_QueryInterface(aNode)); // No cache entry, so we must create the accessible @@ -1323,15 +1308,15 @@ nsAccessibilityService::GetAccessible(nsIDOMNode *aNode, nodeIsDoc = do_QueryInterface(aNode); NS_ENSURE_TRUE(nodeIsDoc, NS_ERROR_FAILURE); // No content, and not doc node +#ifdef DEBUG + // XXX: remove me if you don't see an assertion. nsCOMPtr accessibleDoc = - nsAccessNode::GetDocAccessibleFor(aWeakShell); - if (accessibleDoc) { - newAcc = do_QueryInterface(accessibleDoc); - NS_ASSERTION(newAcc, "nsIAccessibleDocument is not an nsIAccessible"); - } - else { - CreateRootAccessible(aPresShell, nodeIsDoc, getter_AddRefs(newAcc)); // Does Init() for us - } + nsAccessNode::GetDocAccessibleFor(nodeIsDoc); + NS_ASSERTION(!accessibleDoc, + "Trying to create already cached accessible document!"); +#endif + + CreateRootAccessible(aPresShell, nodeIsDoc, getter_AddRefs(newAcc)); // Does Init() for us NS_IF_ADDREF(*aAccessible = newAcc); return NS_OK; @@ -1385,7 +1370,10 @@ nsAccessibilityService::GetAccessible(nsIDOMNode *aNode, PRInt32 childCount; imageAcc->GetChildCount(&childCount); // area accessible should be in cache now - return GetCachedAccessible(aNode, aWeakShell, aAccessible); + nsCOMPtr cachedAreaAcc = + GetCachedAccessNode(aNode, aWeakShell); + if (cachedAreaAcc) + CallQueryInterface(cachedAreaAcc, aAccessible); } } diff --git a/accessible/src/base/nsAccessibilityService.h b/accessible/src/base/nsAccessibilityService.h index 6e21ed4013b..dc39aa53ae2 100644 --- a/accessible/src/base/nsAccessibilityService.h +++ b/accessible/src/base/nsAccessibilityService.h @@ -110,6 +110,17 @@ public: nsIWeakReference *aPresShell, nsIAccessible **aAccessible); + /** + * Return an access node for the DOM node in the given presentation shell if + * the access node already exists, otherwise null. + * + * @param aNode [in] the DOM node to get an access node for + * @param aPresShell [in] the presentation shell which contains layout info + * for the DOM node + */ + already_AddRefed GetCachedAccessNode(nsIDOMNode *aNode, + nsIWeakReference *aShell); + private: /** * Return presentation shell, DOM node for the given frame. diff --git a/accessible/src/base/nsDocAccessible.cpp b/accessible/src/base/nsDocAccessible.cpp index e5bab8bb225..b9e872e6496 100644 --- a/accessible/src/base/nsDocAccessible.cpp +++ b/accessible/src/base/nsDocAccessible.cpp @@ -539,16 +539,30 @@ NS_IMETHODIMP nsDocAccessible::GetAssociatedEditor(nsIEditor **aEditor) return NS_OK; } -NS_IMETHODIMP nsDocAccessible::GetCachedAccessNode(void *aUniqueID, nsIAccessNode **aAccessNode) +already_AddRefed +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; + } + } + #ifdef DEBUG // All cached accessible nodes should be in the parent // It will assert if not all the children were created // when they were first cached, and no invalidation // ever corrected parent accessible's child cache. nsRefPtr acc = - nsAccUtils::QueryObject(*aAccessNode); + nsAccUtils::QueryObject(accessNode); if (acc) { nsAccessible* parent(acc->GetCachedParent()); @@ -556,7 +570,7 @@ NS_IMETHODIMP nsDocAccessible::GetCachedAccessNode(void *aUniqueID, nsIAccessNod parent->TestChildCache(acc); } #endif - return NS_OK; + return accessNode; } // nsDocAccessible public method @@ -601,9 +615,6 @@ nsDocAccessible::Init() GetParent(); // Ensure outer doc mParent accessible. - nsresult rv = nsHyperTextAccessibleWrap::Init(); - NS_ENSURE_SUCCESS(rv, rv); - // Initialize event queue. mEventQueue = new nsAccEventQueue(this); @@ -1763,8 +1774,7 @@ nsDocAccessible::ProcessPendingEvent(nsAccEvent *aEvent) void nsDocAccessible::InvalidateChildrenInSubtree(nsIDOMNode *aStartNode) { - nsCOMPtr accessNode; - GetCachedAccessNode(aStartNode, getter_AddRefs(accessNode)); + nsCOMPtr accessNode = GetCachedAccessNode(aStartNode); nsRefPtr acc(nsAccUtils::QueryAccessible(accessNode)); if (acc) acc->InvalidateChildren(); @@ -1785,8 +1795,7 @@ void nsDocAccessible::RefreshNodes(nsIDOMNode *aStartNode) return; // All we have is a doc accessible. There is nothing to invalidate, quit early } - nsCOMPtr accessNode; - GetCachedAccessNode(aStartNode, getter_AddRefs(accessNode)); + nsCOMPtr accessNode = GetCachedAccessNode(aStartNode); // Shut down accessible subtree, which may have been created for // anonymous content subtree @@ -1942,8 +1951,7 @@ nsDocAccessible::InvalidateCacheSubtree(nsIContent *aChild, } // Update last change state information - nsCOMPtr childAccessNode; - GetCachedAccessNode(childNode, getter_AddRefs(childAccessNode)); + nsCOMPtr childAccessNode = GetCachedAccessNode(childNode); nsCOMPtr childAccessible = do_QueryInterface(childAccessNode); #ifdef DEBUG_A11Y @@ -2124,8 +2132,7 @@ nsDocAccessible::GetAccessibleInParentChain(nsIDOMNode *aNode, aAccessible); } else { // Only return cached accessibles, don't create anything - nsCOMPtr accessNode; - GetCachedAccessNode(currentNode, getter_AddRefs(accessNode)); // AddRefs + nsCOMPtr accessNode = GetCachedAccessNode(currentNode); if (accessNode) { CallQueryInterface(accessNode, aAccessible); // AddRefs } @@ -2149,8 +2156,7 @@ nsDocAccessible::FireShowHideEvents(nsIDOMNode *aDOMNode, if (!aAvoidOnThisNode) { if (aEventType == nsIAccessibleEvent::EVENT_HIDE) { // Don't allow creation for accessibles when nodes going away - nsCOMPtr accessNode; - GetCachedAccessNode(aDOMNode, getter_AddRefs(accessNode)); + nsCOMPtr accessNode = GetCachedAccessNode(aDOMNode); accessible = do_QueryInterface(accessNode); } else { // Allow creation of new accessibles for show events diff --git a/accessible/src/base/nsDocAccessible.h b/accessible/src/base/nsDocAccessible.h index 6fbe869ebfc..7e9f3b6446b 100644 --- a/accessible/src/base/nsDocAccessible.h +++ b/accessible/src/base/nsDocAccessible.h @@ -150,6 +150,18 @@ public: */ void InvalidateCacheSubtree(nsIContent *aContent, PRUint32 aEvent); + /** + * Return the cached access node by the given unique ID if it's in subtree of + * this document accessible or the document accessible itself, otherwise null. + * + * @note the unique ID matches with the uniqueID attribute on nsIAccessNode + * + * @param aUniqueID [in] the unique ID used to cache the node. + * + * @return the access node object + */ + already_AddRefed GetCachedAccessNode(void *aUniqueID); + /** * Cache access node. * diff --git a/accessible/src/msaa/nsDocAccessibleWrap.cpp b/accessible/src/msaa/nsDocAccessibleWrap.cpp index 0d22004b91c..b814dcd876f 100644 --- a/accessible/src/msaa/nsDocAccessibleWrap.cpp +++ b/accessible/src/msaa/nsDocAccessibleWrap.cpp @@ -286,16 +286,17 @@ static PLDHashOperator SearchAccessibleInCache(const void* aKey, nsIAccessNode* aAccessNode, void* aUserArg) { - nsCOMPtr docAccessible(do_QueryInterface(aAccessNode)); - NS_ASSERTION(docAccessible, + nsCOMPtr accessibleDoc(do_QueryInterface(aAccessNode)); + NS_ASSERTION(accessibleDoc, "No doc accessible for the object in doc accessible cache!"); + nsRefPtr docAccessible = + nsAccUtils::QueryObject(accessibleDoc); if (docAccessible) { nsSearchAccessibleInCacheArg* arg = static_cast(aUserArg); - nsCOMPtr accessNode; - docAccessible->GetCachedAccessNode(arg->mUniqueID, - getter_AddRefs(accessNode)); + nsCOMPtr accessNode = + docAccessible->GetCachedAccessNode(arg->mUniqueID); if (accessNode) { arg->mAccessNode = accessNode; return PL_DHASH_STOP;