How to free CWin objects created in a child diaolog to avoid memory leaks

Question

I'm creating a a CStatic conrol in a child dialog, which works fine. The problem is that after closing the child dialog the memory is not freed correctly.

I tried to overwrite PostNCDestoy like described in the this thread: Is this a memory leak in MFC But this gives me an unhandled exception by calling "delete this".

Any idea what the right way is to release CStatic, CButtons to avoid memory leaks?

CChildDlg.h

class CChildDlg
{
  std::vector<CStatic*> m_images;
  void addNewImage(const int &xPos, const int &yPos)
  ...
}

CChildDlg.cpp

void CChildDlg::addNewImage(const int &xPos, const int &yPos){

  CImage imgFromFile;
  CStatic *img = new CStatic;

  imgFromFile.Load(_T("someImg.jpg"));
  int width = imgFromFile.GetWidth();
  int height = imgFromFile.GetHeight();


  img->Create(_T("Image"), WS_CHILD | WS_VISIBLE | SS_BITMAP,
    CRect(xPos, yPos, xPos + width, yPos + height), this, 10910);

  HBITMAP hbmOld = img->SetBitmap(imgFromFile.Detach());
  if (hbmOld != nullptr){
    ::DeleteObject(hbmOld);
  }
  m_images.pushback(img);
}

Based on the recommendations in this thread I changed the code like followed:

CChildDlg.h

class CChildDlg
{
private:
  typedef std::vector<std::unique_ptr <CStatic>> CStaticImgs;
  CStaticImgs m_images;
  void addNewImage(const int &xPos, const int &yPos)
  ...
}

CChildDlg.cpp

void CChildDlg::addNewImage(const int &xPos, const int &yPos){

  CImage imgFromFile;
  std::unique_ptr<CStatic> img(new CStatic);

  imgFromFile.Load(_T("someImg.jpg"));
  int width = imgFromFile.GetWidth();
  int height = imgFromFile.GetHeight();

  img->Create(_T("Image"), WS_CHILD | WS_VISIBLE | SS_BITMAP,
     CRect(xPos, yPos, xPos + width, yPos + height), this, 10910);

  HBITMAP hbmOld = img->SetBitmap(imgFromFile.Detach());
  if (hbmOld != nullptr){
     ::DeleteObject(hbmOld);
  }
  m_images.pushback(std::move(img));
}

The code works fine, but the leak is still there. Only if I'm removing the line where I'm setting the Bitmap to the CStatic the leak disappears:

//HBITMAP hbmOld = img->SetBitmap(imgFromFile.Detach());
//if (hbmOld != nullptr){
  //::DeleteObject(hbmOld);
//}

So, it has to be related to taking over the ownership of the CImage to the CStatic somehow. I'm loading up to 100 images to the dialog. By every opening of the dialog I still can see a significant raise of the memory, which not drops after closing it.

Any other suggestion what might be wrong of missing?

Don't create CStatic dynamically, put it into CChildDlg as a member. — Werner Henze, Jun 30 '16 at 09:31
Are you really sure that you have a memory leak? Did you debug and set and not hit a breakpoint in the destructor of your child window (add any custom window to your window to test this)? The answer you reference is about modeless dialog windows, not child windows of main frames (which the poster specifically excluded from his answer.) — BeyelerStudios, Jun 30 '16 at 12:42
Your container class should be storing the objects themselves, not pointers to them. Then, when the container goes out of scope (i.e., when the class's destructor is called), it will automatically free all of the `CStatic` objects that it contains. — Cody Gray - on strike, Jun 30 '16 at 13:06
Where is your code to delete the CStatic in m_images? The vector's dtor will not delete the CStatics automatically. — Werner Henze, Jun 30 '16 at 15:48
@CodyGray this is what I tried a first. Unfortunately CObject are not copyable. — GregPhil, Jul 01 '16 at 06:39
@WernerHenze, that's a good point. I thought the vector dtor will do so. I will dig in the code later this day. — GregPhil, Jul 01 '16 at 06:40
Hmm, good point. Although copyable isn't technically a requirement for the standard containers, I suppose these MFC classes are not moveable either. Use smart pointers, then. Make a vector of `std::unique_ptr` objects, which will automatically delete the CStatic objects through the pointer once the vector goes out of scope. (The fundamental point is, make use of [SBRM](http://stackoverflow.com/questions/2321511/what-is-meant-by-resource-acquisition-is-initialization-raii) and modern C++ idioms, rather than manually calling delete.) — Cody Gray - on strike, Jul 01 '16 at 06:55
@WernerHenze I checked the number of GDI objects using processexplorer. I can see a significant raise of the objects by the amount of loaded images, but no decrease after closing the dialog. Any idea what is missing? — GregPhil, Jul 04 '16 at 09:15

score 3 · Answer 1 · edited May 23 '17 at 12:34

The naïve solution would be to simply iterate through your container class, calling delete on each pointer. Something like:

for (auto i : m_images)                       { delete i; }            // on C++11

for (size_t i = 0; i < m_images.size(); ++i)  { delete m_images[i]; }  // on C++03

If you do this in your destructor or in response to the WM_DESTROY message (OnDestroy in MFC), it will ensure that each of your CStatic instances are destroyed, solving the memory leak problem.

But this is not the best solution. In C++, you should be taking advantage of Scope-Bound Resource Management (SBRM), also more commonly known as RAII. This involves the use of language features to automatically clean up objects, rather than having to do it manually. Not only does this make the code cleaner, but it ensures that you never forget, that your code is fully exception-safe, and may even be more efficient.

Typically, you would just store the objects themselves in the container class. That is, instead of std::vector<CStatic*>, you would just have std::vector<CStatic>. That way, whenever the vector container is destroyed (which happens automatically when it goes out of scope, thanks to SBRM), all of the objects that it contains get destroyed as well (i.e., their destructors are called automatically).

However, the standard-library containers require that objects be either copyable or movable. The MFC classes derived from CObject are not copyable, and presumably not movable either (given the age of MFC and the standard idioms for making an object non-copyable implicitly making it non-movable). That means this won't work: you can't store CStatic objects themselves in a vector or other container class.

Fortunately, modern C++ has a solution for this problem: the smart pointer. Smart pointers are just what they sound like: a class that wraps the bare ("dumb") pointer, giving it superpowers. Chief among the smarts offered by a smart pointer is SBRM. Whenever the smart pointer object gets destroyed, it automatically deletes its underlying dumb pointer. This frees you, the humble programmer, from ever having to write a single delete statement. In fact, two things you should almost never have to do in C++ are:

explicitly write delete
use raw pointers

So my recommendation, given the limitations of MFC's CObject-derived classes, is to store smart pointers in your vector. The syntax isn't pretty, but that's trivially solved with a typedef:

typedef std::vector<std::unique_ptr<CStatic>> CStaticVector;   // for C++11

(For C++03, since std::auto_ptr cannot be used in a standard container [again, because it is not copyable], you would need to use a different smart pointer, such as the Boost Smart Pointers library.)

No more manual memory-management, no more memory leaks, no more headaches. Working in C++ literally goes from being a pain in the butt to being fast, fun, and efficient.

@Anatoly Ugh, I always forget about how dumb auto_ptr is. Thanks for pointing that out! — Cody Gray - on strike, Jul 01 '16 at 10:39
@CodyGray thank you very much for your vary detailed answer. I changed my code, where I'm using smart pointers now. One note on this: std::unique_ptr is also not copyable just moveable, quite rightly! For details see the following thread: http://stackoverflow.com/questions/3283778/why-can-i-not-push-back-a-unique-ptr-into-a-vector. However, using smart pointers didn't solve my problem. I will update my post with my new insights. — GregPhil, Jul 02 '16 at 14:56

score 0 · Answer 2 · answered Jun 30 '16 at 12:49

0

In CChildDlg destructor add for(auto img : m_images) delete img;.

answered Jun 30 '16 at 12:49

Adrian Roman

534
4
8

score 0 · Answer 3 · answered Jul 06 '16 at 15:21

What I realised is that only by taking care of a proper clean up of the HBITMAPs which are returned while calling .Detach() the number of GDI objects is decreasing to a correct value and the memory leak disappears.

CChildDlg.h

HBITMAT m_deleteMeWhenClosingDlg;
....

CChildDlg.cpp

  ...
  m_deleteMeWhenClosingDlg = imgFromFile.Detach();
  HBITMAP hbmOld = img->SetBitmap(m_deleteMeWhenClosingDlg);
  ...

Later in OnDestroy() for example

  ::DeleteObject(m_deleteMeWhenClosingDlg)

How to free CWin objects created in a child diaolog to avoid memory leaks

3 Answers3

Linked