Last Comment Bug 288761 - Rendering contexts are leaky
: Rendering contexts are leaky
Status: RESOLVED FIXED
[good first bug]
: fixed1.8
Product: Core
Classification: Components
Component: Widget (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Joshua Welderson
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-04-02 07:16 PST by Christian :Biesinger (don't email me, ping me on IRC)
Modified: 2005-10-12 12:13 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed Patch (13.58 KB, patch)
2005-05-31 15:03 PDT, Joshua Welderson
no flags Details | Diff | Splinter Review
Patch to Original Problem (652 bytes, patch)
2005-06-01 20:55 PDT, Joshua Welderson
no flags Details | Diff | Splinter Review
Patch to Original Problem v1.1 (1.20 KB, patch)
2005-06-02 17:33 PDT, Joshua Welderson
vladimir: review-
Details | Diff | Splinter Review
Patch to Original Problem v1.2 (1.22 KB, patch)
2005-06-25 12:04 PDT, Joshua Welderson
no flags Details | Diff | Splinter Review
Unbitrotted Fix (1.19 KB, patch)
2005-10-04 17:52 PDT, Joshua Welderson
roc: review+
roc: superreview+
asa: approval1.8rc1+
Details | Diff | Splinter Review

Description Christian :Biesinger (don't email me, ping me on IRC) 2005-04-02 07:16:27 PST
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 2005-05-31 15:03:57 PDT
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.
Comment 2 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-01 10:48:51 PDT
> 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) 2005-06-01 10:49:41 PDT
hm, I believe the code in accessible/src/msaa/nsTextAccessibleWrap.cpp is still
leaking the pointer...
Comment 4 Joshua Welderson 2005-06-01 20:55:24 PDT
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)?
Comment 5 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-02 14:02:54 PDT
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 2005-06-02 17:33:40 PDT
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.
Comment 7 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-03 08:34:46 PDT
Comment on attachment 185114 [details] [diff] [review]
Patch to Original Problem

looks good to me, thanks!
Comment 8 Vladimir Vukicevic [:vlad] [:vladv] 2005-06-25 00:57:07 PDT
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) 2005-06-25 04:29:57 PDT
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] 2005-06-25 10:06:47 PDT
(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 2005-06-25 12:04:35 PDT
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.
Comment 12 Christian :Biesinger (don't email me, ping me on IRC) 2005-06-25 15:15:22 PDT
why shouldn't we change widget apis? clearly, the current api makes it very easy
to leak.
Comment 13 Vladimir Vukicevic [:vlad] [:vladv] 2005-06-25 21:29:17 PDT
(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) 2005-06-26 15:27:27 PDT
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 2005-10-04 17:52:40 PDT
Created attachment 198532 [details] [diff] [review]
Unbitrotted Fix

Unbitrotted.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-10-05 13:41:08 PDT
Comment on attachment 198532 [details] [diff] [review]
Unbitrotted Fix

Trivial fix to a leak in accessibility.
Comment 17 Asa Dotzler [:asa] 2005-10-12 12:03:44 PDT
The time to get this in is now. Please land ASAP.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2005-10-12 12:13:25 PDT
Checked in on trunk and branch.

Note You need to log in before you can comment on or make changes to this bug.