CPen leaking memory - ARGH !!!!
-
I am trying to write a safe DC class, so I can avoid the hassle of keeping track of pens and brushes returned to me, as well as a one step create/select bitmap process. Here is what I am doing: CPen* CGDC::SafeSelect( CPen* pPen ) { if (m_pOldPen) { return SelectObject(pPen); } else { m_pOldPen = SelectObject(pPen); return m_pOldPen; } } CBrush* CGDC::SafeSelect( CBrush* pBrush ) { if (m_pOldBrush) return SelectObject(pBrush); else { m_pOldBrush = SelectObject(pBrush); return m_pOldBrush; } } m_pOldPen and m_pOldBrush are both NULL, I have stepped through and both functions behave as I would expect, as does the destructor: CGDC::~CGDC() { if (m_pOldPen) SelectObject(m_pOldPen); if (m_pOldBrush) SelectObject(m_pOldBrush); if (m_pOldFont) SelectObject(m_pOldFont); DeleteDC(); } Here's the thing. I tested it with this: // for (int j = 0; j < 500; j++) { CGDC dc; for (int i =0; i<500; i++) { CPen pen; pen.CreatePen(PS_SOLID, 1, RGB(0,0,0)); /*CPen* pPen = */dc.SafeSelect(&pen); CBrush brush; brush.CreateSolidBrush(RGB(0,0,0)); dc.SafeSelect(&brush); // dc.SelectObject(pPen); } } Now, no matter which loop I comment in, if I comment out the SafeSelect on the pen it works fine. If I include the pen select and the call to SelectObject, it works fine. If I declare pPen here or not, if I call SafeSelect on the pen, it doesn't matter if I grab pPen here or not - unless I select it here, although it gets selected in CGDC, I lose about 30% of my GDI rsources when this code is called. I have checked using GDI usage and the number of sequential ( in memory ) pens I leak varies, but is between 450 and 470. I don't leak any brushes. Can anyone tell me what is going on ? I have experimented with deleteobject, etc., but the fact is I seem to do the same thing inside the class and outside, but inside works for brushes, not pens. Thanks Christian The content of this post is not necessarily the opinion of my yadda yadda yadda. To understand recursion, we must first understand recursion.
-
I am trying to write a safe DC class, so I can avoid the hassle of keeping track of pens and brushes returned to me, as well as a one step create/select bitmap process. Here is what I am doing: CPen* CGDC::SafeSelect( CPen* pPen ) { if (m_pOldPen) { return SelectObject(pPen); } else { m_pOldPen = SelectObject(pPen); return m_pOldPen; } } CBrush* CGDC::SafeSelect( CBrush* pBrush ) { if (m_pOldBrush) return SelectObject(pBrush); else { m_pOldBrush = SelectObject(pBrush); return m_pOldBrush; } } m_pOldPen and m_pOldBrush are both NULL, I have stepped through and both functions behave as I would expect, as does the destructor: CGDC::~CGDC() { if (m_pOldPen) SelectObject(m_pOldPen); if (m_pOldBrush) SelectObject(m_pOldBrush); if (m_pOldFont) SelectObject(m_pOldFont); DeleteDC(); } Here's the thing. I tested it with this: // for (int j = 0; j < 500; j++) { CGDC dc; for (int i =0; i<500; i++) { CPen pen; pen.CreatePen(PS_SOLID, 1, RGB(0,0,0)); /*CPen* pPen = */dc.SafeSelect(&pen); CBrush brush; brush.CreateSolidBrush(RGB(0,0,0)); dc.SafeSelect(&brush); // dc.SelectObject(pPen); } } Now, no matter which loop I comment in, if I comment out the SafeSelect on the pen it works fine. If I include the pen select and the call to SelectObject, it works fine. If I declare pPen here or not, if I call SafeSelect on the pen, it doesn't matter if I grab pPen here or not - unless I select it here, although it gets selected in CGDC, I lose about 30% of my GDI rsources when this code is called. I have checked using GDI usage and the number of sequential ( in memory ) pens I leak varies, but is between 450 and 470. I don't leak any brushes. Can anyone tell me what is going on ? I have experimented with deleteobject, etc., but the fact is I seem to do the same thing inside the class and outside, but inside works for brushes, not pens. Thanks Christian The content of this post is not necessarily the opinion of my yadda yadda yadda. To understand recursion, we must first understand recursion.
Modify your SafeSelect functions like this: if (m_pOldPen!=NULL) SelectObject(m_pOldPen); if (pPen!=NULL) m_pOldPen = SelectObject(pPen); else m_pOldPen=NULL; return m_pOldPen; Same for the brush version. Now, if you are detaching the DC handle from the CDC base class, then your destructor is worthless, since the DC handle is gone. The other problem is that I did not see a constructor for your CGDC class. Assuming that you're using the base CDC, then the DC handle is null, and SelectObject will either (a) do nothing (b) bomb (c) leak. Just my $0.02
-
Modify your SafeSelect functions like this: if (m_pOldPen!=NULL) SelectObject(m_pOldPen); if (pPen!=NULL) m_pOldPen = SelectObject(pPen); else m_pOldPen=NULL; return m_pOldPen; Same for the brush version. Now, if you are detaching the DC handle from the CDC base class, then your destructor is worthless, since the DC handle is gone. The other problem is that I did not see a constructor for your CGDC class. Assuming that you're using the base CDC, then the DC handle is null, and SelectObject will either (a) do nothing (b) bomb (c) leak. Just my $0.02
Thanks for the comment, but apart from the fact that I'd not yet checked for NULL 'incoming', I had tried doing it this way previously, tried it again with your code, and it still leaks pens like craazy, but not brushes. Here is my constructor: CGDC::CGDC(CDC* pDC /* = NULL */) { CreateCompatibleDC(pDC); SetStretchBltMode(COLORONCOLOR); m_pOldBitmap = NULL; m_pOldBrush = NULL; m_pOldPen = NULL; m_pOldFont = NULL; } P.S. Why won't Code Project remember me under IE 4 ? Christian
-
Thanks for the comment, but apart from the fact that I'd not yet checked for NULL 'incoming', I had tried doing it this way previously, tried it again with your code, and it still leaks pens like craazy, but not brushes. Here is my constructor: CGDC::CGDC(CDC* pDC /* = NULL */) { CreateCompatibleDC(pDC); SetStretchBltMode(COLORONCOLOR); m_pOldBitmap = NULL; m_pOldBrush = NULL; m_pOldPen = NULL; m_pOldFont = NULL; } P.S. Why won't Code Project remember me under IE 4 ? Christian
Heck it doesn't remember it under IE55 either. I think I spotted your problem; it's in the loop: for (int i=0;i<500;i++) { CPen pen; pen.CreatePen(PS_SOLID, 1, RGB(0,0,0)); dc.SafeSelect(&pen); /* Okay, here is the problem as I see it: the first time this is called, there is no pen to release, so the pen handle is selected into the device context, which basically takes ownership of it. Then your loop exits, and the CPen object is destroyed, taking the handle with it - but more than likly just a COPY of the handle, leaving the other one still selected into the device context, since it was never selected out. Then the 2nd time into the loop you create a new CPen, and then select it in, but you never got rid of the other one. */ } I've never written code like this; I've always done it like, the scope of the function: CPen *pOldPen = dc.SelectObject(&pen); ...dowhatever.... dc.SelectObject(pOldPen); If I was going to attempt to re-write the SafeSelect, I would probably NOT do it like this (IMHO), but instead, make a helper class (or nested class) that does the need balanced SelectObject's on construct and destruct.
-
I am trying to write a safe DC class, so I can avoid the hassle of keeping track of pens and brushes returned to me, as well as a one step create/select bitmap process. Here is what I am doing: CPen* CGDC::SafeSelect( CPen* pPen ) { if (m_pOldPen) { return SelectObject(pPen); } else { m_pOldPen = SelectObject(pPen); return m_pOldPen; } } CBrush* CGDC::SafeSelect( CBrush* pBrush ) { if (m_pOldBrush) return SelectObject(pBrush); else { m_pOldBrush = SelectObject(pBrush); return m_pOldBrush; } } m_pOldPen and m_pOldBrush are both NULL, I have stepped through and both functions behave as I would expect, as does the destructor: CGDC::~CGDC() { if (m_pOldPen) SelectObject(m_pOldPen); if (m_pOldBrush) SelectObject(m_pOldBrush); if (m_pOldFont) SelectObject(m_pOldFont); DeleteDC(); } Here's the thing. I tested it with this: // for (int j = 0; j < 500; j++) { CGDC dc; for (int i =0; i<500; i++) { CPen pen; pen.CreatePen(PS_SOLID, 1, RGB(0,0,0)); /*CPen* pPen = */dc.SafeSelect(&pen); CBrush brush; brush.CreateSolidBrush(RGB(0,0,0)); dc.SafeSelect(&brush); // dc.SelectObject(pPen); } } Now, no matter which loop I comment in, if I comment out the SafeSelect on the pen it works fine. If I include the pen select and the call to SelectObject, it works fine. If I declare pPen here or not, if I call SafeSelect on the pen, it doesn't matter if I grab pPen here or not - unless I select it here, although it gets selected in CGDC, I lose about 30% of my GDI rsources when this code is called. I have checked using GDI usage and the number of sequential ( in memory ) pens I leak varies, but is between 450 and 470. I don't leak any brushes. Can anyone tell me what is going on ? I have experimented with deleteobject, etc., but the fact is I seem to do the same thing inside the class and outside, but inside works for brushes, not pens. Thanks Christian The content of this post is not necessarily the opinion of my yadda yadda yadda. To understand recursion, we must first understand recursion.
Actually, it's not really necessary to remember to reselect the original pens. All you need to do is use the SaveDC() and RestoreDC() functions at the beginning and end of your draw routines. Unfortunately, it's hard to build these into your DC class because pens allocated on the stack might get destroyed before you DC class gets destroyed, and if those pens are currently selected, then you get a resource leak. You have to call RestoreDC() before the objects go out of scope.
-
Heck it doesn't remember it under IE55 either. I think I spotted your problem; it's in the loop: for (int i=0;i<500;i++) { CPen pen; pen.CreatePen(PS_SOLID, 1, RGB(0,0,0)); dc.SafeSelect(&pen); /* Okay, here is the problem as I see it: the first time this is called, there is no pen to release, so the pen handle is selected into the device context, which basically takes ownership of it. Then your loop exits, and the CPen object is destroyed, taking the handle with it - but more than likly just a COPY of the handle, leaving the other one still selected into the device context, since it was never selected out. Then the 2nd time into the loop you create a new CPen, and then select it in, but you never got rid of the other one. */ } I've never written code like this; I've always done it like, the scope of the function: CPen *pOldPen = dc.SelectObject(&pen); ...dowhatever.... dc.SelectObject(pOldPen); If I was going to attempt to re-write the SafeSelect, I would probably NOT do it like this (IMHO), but instead, make a helper class (or nested class) that does the need balanced SelectObject's on construct and destruct.
That is how I have done it in the past too, but what I do not understand is why recreating this process in my class works for brushes, but not pens. If you follow my logic, I am doing the exact same thing in safeselect for brushes ( which work ) and pens ( which don't ) and the same thing as what I do externally ( which works ). I tried Deleteing whatever gets returned to me in SafeSelect, which does not leak, so I may think along those lines instead.