Rendering contexts are leaky

RESOLVED FIXED

Status

()

Core
Widget
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Biesinger, Assigned: Joshua Welderson)

Tracking

({fixed1.8})

Trunk
fixed1.8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 4 obsolete attachments)

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.
OS: Linux → All
Hardware: PC → All
Whiteboard: [good first bug]
(Assignee)

Comment 1

12 years ago
Created attachment 184958 [details] [diff] [review]
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.
Attachment #184958 - Flags: review?(blizzard)
> 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.

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

Comment 4

12 years ago
Created attachment 185114 [details] [diff] [review]
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)?
Attachment #185114 - Flags: review?(blizzard)
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...
(Assignee)

Comment 6

12 years ago
Created attachment 185230 [details] [diff] [review]
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.
Attachment #185114 - Attachment is obsolete: true
Attachment #185230 - Flags: review?(blizzard)
Comment on attachment 185114 [details] [diff] [review]
Patch to Original Problem

looks good to me, thanks!
Attachment #185114 - Flags: review?(blizzard)
(Assignee)

Updated

12 years ago
Attachment #184958 - Flags: review?(blizzard) → review?(vladimir)
(Assignee)

Updated

12 years ago
Attachment #185230 - Flags: review?(blizzard) → review?(vladimir)
Attachment #184958 - Flags: review?(vladimir)
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.
Attachment #185230 - Flags: review?(vladimir) → review-
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.
(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.
(Assignee)

Comment 11

12 years ago
Created attachment 187297 [details] [diff] [review]
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.
Attachment #184958 - Attachment is obsolete: true
Attachment #185230 - Attachment is obsolete: true
Attachment #187297 - Flags: review?(shriyash_21)
(Assignee)

Updated

12 years ago
Attachment #187297 - Flags: review?(shriyash_21) → review?(vladimir)
why shouldn't we change widget apis? clearly, the current api makes it very easy
to leak.
(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
I think we should probably fix the API since it's not clear when we'll get
around to using a Thebes type here.
(Assignee)

Comment 15

12 years ago
Created attachment 198532 [details] [diff] [review]
Unbitrotted Fix

Unbitrotted.
Attachment #187297 - Attachment is obsolete: true
Attachment #198532 - Flags: review?(vladimir)
(Assignee)

Updated

12 years ago
Attachment #198532 - Flags: superreview?(roc)
(Assignee)

Updated

12 years ago
Attachment #187297 - Flags: review?(vladimir)
Comment on attachment 198532 [details] [diff] [review]
Unbitrotted Fix

Trivial fix to a leak in accessibility.
Attachment #198532 - Flags: superreview?(roc)
Attachment #198532 - Flags: superreview+
Attachment #198532 - Flags: review?(vladimir)
Attachment #198532 - Flags: review+
Attachment #198532 - Flags: approval1.8rc1?
(Assignee)

Updated

12 years ago
Assignee: general → pythonesque+bugzilla
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED

Updated

12 years ago
Attachment #198532 - Flags: approval1.8rc1? → approval1.8rc1+

Comment 17

12 years ago
The time to get this in is now. Please land ASAP.
Checked in on trunk and branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.