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.
12 years ago
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.
> 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...
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)?
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...
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 on attachment 185114 [details] [diff] [review] Patch to Original Problem looks good to me, thanks!
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.
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.
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.
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.
Created attachment 198532 [details] [diff] [review] Unbitrotted Fix Unbitrotted.
Comment on attachment 198532 [details] [diff] [review] Unbitrotted Fix Trivial fix to a leak in accessibility.
The time to get this in is now. Please land ASAP.
Checked in on trunk and branch.