288761 - Rendering contexts are leaky

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

nsIWidget::GetRenderingContext returns an addrefed context as a raw pointer.
this forces callers to write dont_AddRef(GetRenderingContext()) if they access
it and want to store it in an nsCOMPtr, unless they want to leak it. not all
code does that
(http://lxr.mozilla.org/seamonkey/source/accessible/src/msaa/nsTextAccessibleWrap.cpp#244
leaks it, apparently).

This should return an already_AddRefed.

Comment 1

[Joshua Welderson]

Attachment: Proposed Patch

I assume, from a look at the other Mozilla files and an attempt to compile
without it, that the get() method must be called in order to use the pointer
component of an already_AddRefed pointer.  If this is true, then this patch
should work nicely.  Otherwise, please tell me how to fix it, as I'm still
rather new at this.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

> that the get() method must be called in order to use the pointer
> component of an already_AddRefed pointer

yes, that's correct. the reason for that is so that people do not accidentally
leak the pointer.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

hm, I believe the code in accessible/src/msaa/nsTextAccessibleWrap.cpp is still
leaking the pointer...

Comment 4

[Joshua Welderson]

Attachment: Patch to Original Problem

Ah, yes.  I suspected that I might not have fixed the original problem by
keeping everything the same.  I assume, by the way, that I can call an
NS_RELEASE here with no fear of retribution (the rendering context in question
appears to be used only twice, in calls to GetPointFromOffset, which itself
does not appear to use it)?

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

if callees want to use passed-in objects after they return, they should add a
reference themselves, so that should be fine, although maybe using an nsCOMPtr
would be better here...

Comment 6

[Joshua Welderson]

Attachment: Patch to Original Problem v1.1

Done (methinks).  If I'm misunderstanding the role of nsCOMPtr, or haven't done
the patch properly because of some odd order-of-application thing, please tell
me.  Otherwise, this is hopefully fixed.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 185114 [details] [diff] [review]
Patch to Original Problem

looks good to me, thanks!

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 185230 [details] [diff] [review]
Patch to Original Problem v1.1

>Index: nsTextAccessibleWrap.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/accessible/src/msaa/nsTextAccessibleWrap.cpp,v
>retrieving revision 1.12
>diff -u -8 -p -r1.12 nsTextAccessibleWrap.cpp
>--- nsTextAccessibleWrap.cpp	22 Jan 2005 16:00:07 -0000	1.12
>+++ nsTextAccessibleWrap.cpp	3 Jun 2005 00:15:41 -0000
>@@ -236,18 +236,17 @@ nsresult nsTextAccessibleWrap::GetCharac

>-  nsIRenderingContext *rendContext;
>-  rendContext = widget->GetRenderingContext();
>+  nsCOMPtr<nsIRenderingContext> rendContext(widget->GetRenderingContext());

refcounting gets tricky ;)

nsCOMPtr<> will always take a ref to any raw pointers assigned to it. 
GetRenderingContext() returns an pointer that it has already called AddRef on,
so doing it this way would continue to cause the leak (there would still be a
ref on that RenderingContext that was never Release'd).

That's where already_AddRefed() comes into play:

  nsCOMPtr<nsIRenderingContext>
rendContext(already_AddRefed(widget->GetRenderingContext()));

which tells nsCOMPtr that the thing its getting has already had a ref taken,
and it should just take ownership of that ref.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

vlad: my understanding is that the patch was supposed to be applied together
with "Proposed patch" (for which you cancelled the review request). Also, note
that already_AddRefed is a template, so I think the syntax there is wrong, I
think you mean dont_AddRef here.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #9)
> vlad: my understanding is that the patch was supposed to be applied together
> with "Proposed patch" (for which you cancelled the review request). Also, note
> that already_AddRefed is a template, so I think the syntax there is wrong, I
> think you mean dont_AddRef here.

Yes, sorry.. should be dont_AddRef  (or
already_AddRefed<nsIRenderingContext>(...->GetRenderingContext())).  But either
way the proposed patch isn't needed, and changing the gfx api at this point is
not a good idea.

Comment 11

[Joshua Welderson]

Attachment: Patch to Original Problem v1.2

So would you prefer that I fix it solely for this leak?  If so, I would just
use getter_AddRefs just for consistency with the rest of the code.

Insofar as the API goes, while I realize changing it would be a bad idea, it
still might be a better long-term solution to have it return an
already_AddRefed when it, er, is returning an already_AddRefed.

Anyway, unless something I've said is wrong, this patch should work.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

why shouldn't we change widget apis? clearly, the current api makes it very easy
to leak.

Comment 13

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #12)
> why shouldn't we change widget apis? clearly, the current api makes it very easy
> to leak.

Because we're a) going to branch for 1.1 in less than a week, and b) we're going
to throw the whole lot of gfx out after that

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should probably fix the API since it's not clear when we'll get
around to using a Thebes type here.

Comment 15

[Joshua Welderson]

Attachment: Unbitrotted Fix

Unbitrotted.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 198532 [details] [diff] [review]
Unbitrotted Fix

Trivial fix to a leak in accessibility.

Comment 17

[Asa Dotzler [:asa]]

The time to get this in is now. Please land ASAP.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in on trunk and branch.