How to free CWin objects created in a child diaolo

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?

Answer 1

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

Answer 2

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.

Answer 3

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)