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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: aaronlev, Assigned: yuanyi21)
References
Details
(Keywords: memory-leak)
Attachments
(2 files, 3 obsolete files)
12.15 KB,
patch
|
aaronlev
:
review+
peterv
:
superreview+
|
Details | Diff | Splinter Review |
12.28 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•22 years ago
|
||
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
Reporter | ||
Comment 3•22 years ago
|
||
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.
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)
Reporter | ||
Comment 7•22 years ago
|
||
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)
Reporter | ||
Comment 9•22 years ago
|
||
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.
Assignee | ||
Comment 10•22 years ago
|
||
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)
Reporter | ||
Updated•22 years ago
|
Attachment #108855 -
Flags: review?(aaronl) → review+
Attachment #108855 -
Flags: superreview?(peterv)
Assignee | ||
Comment 11•22 years ago
|
||
*** Bug 182980 has been marked as a duplicate of this bug. ***
Attachment #108855 -
Flags: superreview?(peterv) → superreview?(dbaron)
Comment 12•22 years ago
|
||
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-
Assignee | ||
Comment 13•22 years ago
|
||
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)
Reporter | ||
Updated•22 years ago
|
Attachment #109499 -
Flags: superreview?(peterv)
Attachment #109499 -
Flags: review?(aaronl)
Attachment #109499 -
Flags: review+
Comment 14•22 years ago
|
||
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+
Assignee | ||
Comment 15•22 years ago
|
||
Assignee | ||
Comment 16•22 years ago
|
||
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.
Description
•