Closed Bug 366000 Opened 13 years ago Closed 13 years ago

MaiHyperlink class memory leak

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: nian.liu, Assigned: ginnchen+exoracle)

References

Details

(Keywords: access, memory-leak)

Attachments

(1 file, 6 obsolete files)

MaiHyperlink object is created and never deleted
Attached patch patch (obsolete) — Splinter Review
Attachment #250555 - Flags: review?(ginn.chen)
My concern is adding a pointer to nsAccessibleWrap object will use some memory unnecessarily.

Another solution is to delete MaiHyperlink object in MaiAtkHyperlink's finalizeCB.
But in this way, we will create multiple MaiHyperlink and MaiAtkHyperlink objects for same nsAccessibleWrap object since we don't have a cache in nsAccessibleWrap.

Aaron, do you have a suggestion here?
(In reply to comment #2)
> Another solution is to delete MaiHyperlink object in MaiAtkHyperlink's
> finalizeCB.
> But in this way, we will create multiple MaiHyperlink and MaiAtkHyperlink
> objects for same nsAccessibleWrap object since we don't have a cache in
> nsAccessibleWrap.

This solution will not work.
To trigger MaiAtkHyperlink's finailizeCB, need ~MaiHyperlink() first to unref mMaiAtkHyperlink.

I don't have a suggestion. Ginn, you understand this code better than I do.
Comment on attachment 250555 [details] [diff] [review]
patch

This patch doesn't work.
It's still a circular reference situation.
Attachment #250555 - Flags: review?(ginn.chen) → review-
Blocks: newatk
Attached patch patch v2 (obsolete) — Splinter Review
use weak ref to solve ref count.
Attachment #250555 - Attachment is obsolete: true
(In reply to comment #6)
> Created an attachment (id=251608) [details]
> patch v2
> 
> use weak ref to solve ref count.
> 

the code sequence is incorrect here.

     if (mMaiAtkHyperlink)
         g_object_unref(mMaiAtkHyperlink);
+    MAI_ATK_HYPERLINK(mMaiAtkHyperlink)->maiHyperlink = nsnull;
> the code sequence is incorrect here.
> 
>      if (mMaiAtkHyperlink)
>          g_object_unref(mMaiAtkHyperlink);
> +    MAI_ATK_HYPERLINK(mMaiAtkHyperlink)->maiHyperlink = nsnull;
> 
yep, memory may be released earlier.
Blocks: atk
No longer blocks: newatk
Keywords: access
Summary: MaiHyperlink class memeory leak → MaiHyperlink class memory leak
Assignee: nian.liu → ginn.chen
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #265662 - Flags: review?(aaronleventhal)
Attachment #251608 - Attachment is obsolete: true
I tried to use weak ref to solve this issue, but it could not work. Because we create MaiAtkHyperlink in getLinkCB and getHyperlinkCB. It is a "get" function not "ref" function, we are responsible to clean MaiAtkHyperlink. MaiAtkHyperlink's finalizeCB will not be called, unless we unref it.

My patch is based on patch v2.
The change is I used g_object_get_qdata/g_object_set_qdata to record MaiHyperlink pointer instead of add a pointer for every nsAccessibleWrap. I guess in this way, we can save memory, because we only record the pointer when it is needed.

I also removed MaiHyperlink::GetUniqueID, because it's never used.
I renamed some "maiHyperlink" to "maiAtkHyperlink" because it is a pointer of MaiAtkHyperlink there.
The code is very hard to read if it is misleading.

MaiHyperlink is a bridge between MaiAtkHyperlink and nsAccessibleWrap (with nsIAccessibleHyperlink interface).
It's not necessary to have such a class.
We could get it removed.
I don't want do it for now, unless it's highly recommended.
Comment on attachment 265662 [details] [diff] [review]
patch v3

Looks good, although I think all the code in ~nsAccessibleWrap() should be in nsAccessibleWrap::Shutdown()

Internally we call Shutdown() when an object is no longer valid. At that point mDOMNode is nsnull. Also, GetMaiHyperlink() should always return nsnull when mDOMNode == nsnull (the accessible is shutdown)

One other thing:
+        MaiHyperlink* maiHyperlink = GetMaiHyperlink(PR_FALSE);
+        if (maiHyperlink) {
+            SetMaiHyperlink(nsnull);
+            delete maiHyperlink;
+        }

You should just be able to call SetMaiHyperlink(nsnull) and have it do all the work. Whenever you SetMaiHyperlink() it should destroy any old hyperlink if there was one.

Please post a new patch and request sr= (perhaps from neil@httl.net).
Attachment #265662 - Flags: review?(aaronleventhal) → review+
Attached patch patch v4 (obsolete) — Splinter Review
Thanks for Aaron's comments.
Attachment #265662 - Attachment is obsolete: true
Attachment #265769 - Flags: superreview?(neil)
Attached patch patch v5 (obsolete) — Splinter Review
Correction for nsMaiInterfaceHypertext.cpp getLinkCB

Aaron, should we avoid using NS_STATIC_CAST(nsAccessibleWrap *, ...) ?
Attachment #265769 - Attachment is obsolete: true
Attachment #265777 - Flags: superreview?(neil)
Attachment #265777 - Flags: review?(aaronleventhal)
Attachment #265769 - Flags: superreview?(neil)
Attached patch patch v6 (obsolete) — Splinter Review
there's a chance GetMaiHyperlink is called before GetNativeInterface,
so we have to add a line to make sure mAtkObject is created.

Sorry for my spams.
Attachment #265777 - Attachment is obsolete: true
Attachment #265783 - Flags: superreview?(neil)
Attachment #265783 - Flags: review?(aaronleventhal)
Attachment #265777 - Flags: superreview?(neil)
Attachment #265777 - Flags: review?(aaronleventhal)
Assignee: ginn.chen → aaronleventhal
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis
Comment on attachment 265783 [details] [diff] [review]
patch v6

In nsMaiInterfaceHypertext.cpp getLinkCB we're using the same variable name for 2 different accWrap's. I recommend using 2 different variable names to avoid confusion.
Attachment #265783 - Flags: review?(aaronleventhal) → review+
(In reply to comment #13)
>Aaron, should we avoid using NS_STATIC_CAST(nsAccessibleWrap *, ...) ?
I'd like to see an answer to this too ;-)
> Aaron, should we avoid using NS_STATIC_CAST(nsAccessibleWrap *, ...) ?

Ginn, I guess that you, Hakan and Bsmedberg decided to do something different when you needed to be able to get to an nsRootAccesible.
See https://bugzilla.mozilla.org/show_bug.cgi?id=341463#c15

I wasn't involved in the discussion and don't know how we decided that NS_STATIC_CAST is not okay, but every hyperlink should be an nsAccessibleWrap. 

If it's bad C++ (I'm not sure) than you could same technique you used before:

nsCOMPtr<nsAccessibleWrap> foo;
hyperLink->QueryInterface(NS_GET_IID(foo), getter_AddRefs(foo));

(In reply to comment #17)
>See https://bugzilla.mozilla.org/show_bug.cgi?id=341463#c15
Although nowadays Bugzilla recognises its own links and adds the appropriate tooltip you can just type bug 341463 comment 15 for the same effect.

>nsCOMPtr<nsAccessibleWrap> foo;
>hyperLink->QueryInterface(NS_GET_IID(foo), getter_AddRefs(foo));
I think this is wrong. Perhaps you meant
nsRefPtr<nsAccessibleWrap> foo; ?
Comment on attachment 265783 [details] [diff] [review]
patch v6

>+    // make sure mAtkObject is created
>+    void *dummy;
>+    GetNativeInterface(&dummy);
This AddRefs, doesn't it? So you need to Release it.
sr=me with this and the static cast fixed.
Attachment #265783 - Flags: superreview?(neil) → superreview+
Attached patch patch v7Splinter Review
1) changed variable names in getLinkCB
2)
>+    void *dummy;
>+    GetNativeInterface(&dummy);
atk version of GetNativeInterface doesn't addref, so it's ok here.
(msaa version does)
In v7, I simplified it to GetAtkObject();
3)
For NS_STATIC_CAST, it need a lot of work to get QueryInterface(NS_GET_IID(nsAccessibleWrap)) work.
And NS_STATIC_CAST is widely used in accessible/src/atk
I suggest we file another bug to fix it.
For this patch, I used GetNativeInterface and GetAccessibleWrap to get around.
Assignee: aaronleventhal → ginn.chen
Attachment #265783 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #266372 - Flags: superreview?(neil)
Comment on attachment 266372 [details] [diff] [review]
patch v7

>+    // make sure mAtkObject is created
>+    GetAtkObject();
I like this change :-)
Attachment #266372 - Flags: superreview?(neil) → superreview+
Neil, yes I meant nsRefPtr<>
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Keywords: mlk
You need to log in before you can comment on or make changes to this bug.