Closed
Bug 288761
Opened 19 years ago
Closed 19 years ago
Rendering contexts are leaky
Categories
(Core :: Widget, defect)
Core
Widget
Tracking
()
RESOLVED
FIXED
People
(Reporter: Biesinger, Assigned: pythonesque+bugzilla)
Details
(Keywords: fixed1.8, Whiteboard: [good first bug])
Attachments
(1 file, 4 obsolete files)
1.19 KB,
patch
|
roc
:
review+
roc
:
superreview+
asa
:
approval1.8rc1+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•19 years ago
|
OS: Linux → All
Hardware: PC → All
Reporter | ||
Updated•19 years ago
|
Whiteboard: [good first bug]
Assignee | ||
Comment 1•19 years ago
|
||
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)
Reporter | ||
Comment 2•19 years ago
|
||
> 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.
Reporter | ||
Comment 3•19 years ago
|
||
hm, I believe the code in accessible/src/msaa/nsTextAccessibleWrap.cpp is still leaking the pointer...
Assignee | ||
Comment 4•19 years ago
|
||
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)
Reporter | ||
Comment 5•19 years ago
|
||
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•19 years ago
|
||
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)
Reporter | ||
Comment 7•19 years ago
|
||
Comment on attachment 185114 [details] [diff] [review] Patch to Original Problem looks good to me, thanks!
Attachment #185114 -
Flags: review?(blizzard)
Assignee | ||
Updated•19 years ago
|
Attachment #184958 -
Flags: review?(blizzard) → review?(vladimir)
Assignee | ||
Updated•19 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-
Reporter | ||
Comment 9•19 years ago
|
||
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•19 years ago
|
||
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•19 years ago
|
Attachment #187297 -
Flags: review?(shriyash_21) → review?(vladimir)
Reporter | ||
Comment 12•19 years ago
|
||
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•19 years ago
|
||
Unbitrotted.
Attachment #187297 -
Attachment is obsolete: true
Attachment #198532 -
Flags: review?(vladimir)
Assignee | ||
Updated•19 years ago
|
Attachment #198532 -
Flags: superreview?(roc)
Assignee | ||
Updated•19 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•19 years ago
|
Assignee: general → pythonesque+bugzilla
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Updated•19 years ago
|
Attachment #198532 -
Flags: approval1.8rc1? → approval1.8rc1+
Comment 17•19 years ago
|
||
The time to get this in is now. Please land ASAP.
Checked in on trunk and branch.
You need to log in
before you can comment on or make changes to this bug.
Description
•