163816 - Leaking 1 nsAccessibilityService and a bunch of nsGenericAccessible's

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

When testing with the environment variable 
XPCOM_MEM_LEAK_LOG=leak.log

I get the following output:
    
|<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total     
Rem      Mean       StdDev     Total      Rem      Mean       StdDev
 118 nsAccessibilityService                         12       12        1       
1 (    1.00 +/-     0.00)    44109      938 (  497.73 +/-   278.58)
 299 nsGenericAccessible                            12    37596    19025    
3133 ( 3107.55 +/-  1283.20)   595336     4391 ( 5541.70 +/-  2108.62)

Need to get to the bottom of it.

Comment 1

[Aaron Leventhal]

Anyone from Sun have time for this in the next week?
Otherwise, I'll take it.

Comment 2

[Kyle Yuan]

ok, I'll take a look at this.

Comment 3

[Aaron Leventhal]

Useful articles for finding sources of leaks:
http://www.mozilla.org/performance/leak-tutorial.html
http://mozilla.org/performance/refcnt-balancer.html
http://www.mozilla.org/performance/leak-brownbag.html

Comment 4

[Kyle Yuan]

Simford, can you take a look at this memory leak problem? You can get help from 
our performance team.

Comment 5

[Simford]

taking

Comment 6

[Kyle Yuan]

Attachment: patch

two reasons for the leaks:
1. nsHTMLIFrameRootAccessible has a strong comptr to nsHTMLIFrameAccessible,
but the later is used for the platform related code (refer to:
nsAccessibilityService::CreateIFrameAccessible), so the nsHTMLIFrameAccessible
object can't be released correctly;
2. in nsRootAccessible, some pointer castings are not consistent in
AddAccessibleEventListener/RemoveAccessibleEventListener, so some listeners
can't be unregistered correctly.

My patch did:
1. make nsGenericAccessible instead of nsRootAccessible support
nsISupportsWeakReference;
2. change the strong comptr to a weak reference in nsHTMLIFrameRootAccessible;
3. correct the type casting in AddAccessibleEventListener;
4. clean up a few old-fashion XPCOM code.

Comment 7

[Aaron Leventhal]

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

Kyle, thank you for finding the problems. However, this patch increases the
size of every accessible by 8 bytes. Since the problem is only with
HTMLIFrame/HTMLIFrameRoot do we have to use nsISupportsWeakReference on
nsGenericAccessible?

Comment 8

[Kyle Yuan]

Attachment: make nsHTMLIFrameAccessible, instead of nsGenericAccessible, inherit from nsISupportsWeakReference

no change for other part.

Comment 9

[Aaron Leventhal]

Comment on attachment 108826 [details] [diff] [review]
make nsHTMLIFrameAccessible, instead of nsGenericAccessible, inherit from nsISupportsWeakReference

Couple of questions:

Why this change?
-    nsCOMPtr<nsIAccessibleEventReceiver>
accessibleEventReceiver(do_QueryInterface(this));
+    nsCOMPtr<nsIAccessibleEventReceiver>
accessibleEventReceiver(do_QueryInterface((nsIAccessible *)this));

I think you should change the name from Init() to GetOuterAccessible(), because
it only initializes the first time. Right now, it looks like some
initialization happens each time.

Comment 10

[Kyle Yuan]

Attachment: rename Init to GetOuterAccessible

1. it was used to avoid ambiguous conversion of |this| when nsGenericAccessible
inherited from nsISupportWeakReference, but no longer needed now. I forgot to
get rid of it in the previous patch.

2. the function name was changed to GetOuterAccessible.

Comment 11

[Kyle Yuan]

*** Bug 182980 has been marked as a duplicate of this bug. ***

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 108855 [details] [diff] [review]
rename Init to GetOuterAccessible

>Index: src/base/nsAccessibilityService.cpp
>===================================================================

>@@ -305,10 +304,11 @@
>                                        outerWeakShell, sub_doc);
> 
>           if (outerRootAccessible) {
>-            innerRootAccessible->Init(outerRootAccessible);
>             *_retval = outerRootAccessible;
>             if (*_retval) {

Isn't this if unnecesary?

>Index: src/html/nsHTMLIFrameRootAccessible.cpp
>===================================================================

>-void nsHTMLIFrameRootAccessible::Init()
>+nsresult nsHTMLIFrameRootAccessible::GetOuterAccessible(nsIAccessible **aOuterAcc)
> {

>+  if (mOuterAccessibleWeak) {
>+    nsCOMPtr<nsIAccessible> outerAcc = do_QueryReferent(mOuterAccessibleWeak);
>+    *aOuterAcc = outerAcc;
>+    NS_IF_ADDREF(*aOuterAcc);

I think you can use

  CallQueryReferent(mOuterAccessibleWeak, aOuterAcc);

>+  }
>+  else {
>+    nsCOMPtr<nsIDOMDocument> domDoc;
>+    mOuterNode->GetOwnerDocument(getter_AddRefs(domDoc));
>+    nsCOMPtr<nsIDocument> doc(do_QueryInterface(domDoc));
>+    if (doc) {
>+      nsCOMPtr<nsIPresShell> parentShell;
>+      doc->GetShellAt(0, getter_AddRefs(parentShell));
>+      if (parentShell) {
>+        nsCOMPtr<nsIContent> content(do_QueryInterface(mOuterNode));
>+        nsIFrame* frame = nsnull;
>+        parentShell->GetPrimaryFrameFor(content, &frame);
>+        NS_ASSERTION(frame, "No outer frame.");
>+        if (!frame)
>+          return NS_ERROR_FAILURE;
>+        frame->GetAccessible(aOuterAcc);
>+        NS_ASSERTION(*aOuterAcc, "Something's wrong - there's no accessible for the outer parent of this frame.");
>+        NS_IF_ADDREF(*aOuterAcc);

This will leak, since GetAccessible already addrefs.

>+        mOuterAccessibleWeak = do_GetWeakReference(*aOuterAcc);
>+      }
>     }
>   }
>+
>+  if (*aOuterAcc == nsnull)

Use ! instead of == nsnull.

>Index: src/html/nsHTMLIFrameRootAccessible.h
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/html/nsHTMLIFrameRootAccessible.h,v
>retrieving revision 1.16
>diff -u -r1.16 nsHTMLIFrameRootAccessible.h
>--- src/html/nsHTMLIFrameRootAccessible.h	30 Oct 2002 07:46:34 -0000	1.16
>+++ src/html/nsHTMLIFrameRootAccessible.h	10 Dec 2002 06:40:52 -0000
>@@ -53,7 +53,8 @@
>                                public nsIAccessibleDocument,
>                                public nsIAccessibleHyperText,
>                                public nsIAccessibleEventReceiver,
>-                               public nsDocAccessibleMixin
>+                               public nsDocAccessibleMixin,
>+                               public nsSupportsWeakReference
> {
>   NS_DECL_ISUPPORTS_INHERITED
>   NS_DECL_NSIACCESSIBLEDOCUMENT
>@@ -110,17 +111,17 @@
>     NS_IMETHOD RemoveAccessibleEventListener();
> 
>   protected:
>-    void Init();
>+    nsresult GetOuterAccessible(nsIAccessible **aOuterAcc);

Can't this be a private method? Also, you always seem to ignore the exact error
this returns. I would make this return an already_AddRefed<nsIAccessible>
instead, and return nsnull for an error.

> 
>   public:
>-    void Init(nsIAccessible *aOuterAccessible);
>+    void Init(nsIWeakReference *aOuterAccessibleWeak);

I think you shouldn't change the signature of this function, just get the weak
reference inside this function and pass in the nsIAccessible.

Comment 13

[Kyle Yuan]

Attachment: totally get rid of the pointer to nsHTMLIFrameAccessible

I realized that we shouldn't expose nsHTMLIFrameRootAccessible to AT apps
anyway. (Now it only happened through nsAccessible::CacheOptimizations. I used
ROLE_NOTHING for identifying nsHTMLIFrameRootAccessible to prevent this from
happening.) nsHTMLIFrameAccessible is the right object for that purpose. So I
totally removed the mOuterAccessible and mOuterNode from
nsHTMLIFrameRootAccessible.

Comment 14

[Peter Van der Beken [:peterv]]

Comment on attachment 109499 [details] [diff] [review]
totally get rid of the pointer to nsHTMLIFrameAccessible

>Index: src/base/nsAccessibilityService.cpp
>===================================================================
>@@ -296,8 +295,11 @@
>         nsCOMPtr<nsIWeakReference> innerWeakShell =
>           do_GetWeakReference(innerPresShell);
> 
>+        // In these variable names, "outer" relates to the nsHTMLIFrameAccessible, as opposed to the 
>+        // nsHTMLIFrameRootAccessible which is "inner".
>+        // The outer node is a <browser> or <iframe> tag, whereas the inner node corresponds to the inner document root.

Wrap this comment properly (< 80 chars).

>@@ -305,12 +307,9 @@
>
>+            NS_ADDREF(*_retval);
>+            return NS_OK;
>           }
>           else // don't leak the innerRoot

No else after return.

>Index: src/base/nsAccessible.cpp
>===================================================================
>
> NS_IMETHODIMP nsAccessible::CacheOptimizations(nsIAccessible *aParent, PRInt32 aSiblingIndex, nsIDOMNodeList *aSiblingList)
> {
>-  if (aParent)
>-    mParent = aParent;
>+  if (aParent){

Add a space after the parens.

>+    PRUint32 role = 0;
>+    aParent->GetAccRole(&role);
>+    // prevent from invalid caching nsHTMLIFrameRootAccessible
>+    if (role != ROLE_NOTHING)
>+      mParent = aParent;

Add braces around this line.

Comment 15

[Kyle Yuan]

Attachment: final patch for checking in, addressed peterv's review comment

Comment 16

[Kyle Yuan]

checked in!