Closed Bug 163816 Opened 22 years ago Closed 22 years ago

Leaking 1 nsAccessibilityService and a bunch of nsGenericAccessible's

Categories

(Core :: Disability Access APIs, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: yuanyi21)

References

Details

(Keywords: memory-leak)

Attachments

(2 files, 3 obsolete files)

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.
Keywords: mlk
Anyone from Sun have time for this in the next week?
Otherwise, I'll take it.
Blocks: atfmeta
ok, I'll take a look at this.
Assignee: aaronl → kyle.yuan
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
Summary: Leaking 1 nsAcessibilityService and a bunch of nsGenericAccessible's → Leaking 1 nsAccessibilityService and a bunch of nsGenericAccessible's
Simford, can you take a look at this memory leak problem? You can get help from 
our performance team.
taking
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #108345 - Flags: review?(aaronl)
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?
Attachment #108345 - Flags: review?(aaronl) → review-
no change for other part.
Attachment #108345 - Attachment is obsolete: true
Attachment #108826 - Flags: review?(aaronl)
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.
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.
Attachment #108826 - Attachment is obsolete: true
Attachment #108855 - Flags: review?(aaronl)
Attachment #108826 - Flags: review?(aaronl)
Attachment #108855 - Flags: review?(aaronl) → review+
Attachment #108855 - Flags: superreview?(peterv)
*** Bug 182980 has been marked as a duplicate of this bug. ***
Attachment #108855 - Flags: superreview?(peterv) → superreview?(dbaron)
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.
Attachment #108855 - Flags: superreview?(dbaron) → superreview-
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.
Attachment #108855 - Attachment is obsolete: true
Attachment #109499 - Flags: review?(aaronl)
Attachment #109499 - Flags: superreview?(peterv)
Attachment #109499 - Flags: review?(aaronl)
Attachment #109499 - Flags: review+
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.
Attachment #109499 - Flags: superreview?(peterv) → superreview+
checked in!
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: