3

Windows API resources can cause memory leaks if not taken care of properly. Whether that's the case or not in this issue, I'm assuming it's related. Although I show how I pinned down to where the issue is coming from, I haven't been able to solve it.

I have two types of static class controls using the Win32 API, which is abstracted in my classes:

Label
LinkLabel 

The issue: Whenever I add these two controls, Visual Studio 2017's Diagnostic Tools shows Process Memory (MB) increasing from 3MB to 11MB when I have either both setFont() or setHoverColor() lines enabled, and eventually everything in my GUI space disappears -- gone from existence, like some well-known bookstores.

This code is fine (3MB stays the same constant rate in Process Memory):

 // Linked Label
 myLinkLabel.init("http://www.google.com", 50, 450);
 myLinkLabel.setColor(0, 0, 255);
 myLinkLabel.onClick(lbl_Click);
 myLinkLabel.setFont("Arial", 40, true);
 //lbl.setHoverColor(255, 0, 0);  

 // label
 myLabel.init("A regular static label", 0, 0);
 myLabel.setColor(0, 255, 0);
 myLabel.setFont("Arial", 40);
 //myLabel.setHoverColor(255, 0, 0);

This next code uncomments the last line. After hovering over myLabel, and the red highlight color appears, Process Memory's 3MB increases to 7MB+. It sits for a bit, then goes up to 9MB+. So, something is wrong in it. 

// Linked Label
myLinkLabel.init("http://www.google.com", 50, 450);
myLinkLabel.setColor(0, 0, 255);
myLinkLabel.onClick(lbl_Click);
myLinkLabel.setFont("Arial", 40, true);
//lbl.setHoverColor(255, 0, 0);  

// label
myLabel.init("A regular static label", 0, 0);
myLabel.setColor(0, 255, 0);
myLabel.setFont("Arial", 48);
myLabel.setHoverColor(255, 0, 0);

So, let's dig into my setHoverColor():

void Label::setHoverColor(const BYTE red, const BYTE blue, const BYTE green)
{
 m_hoverColorEnabled = true;
 m_hoverColor = RGB(red, green, blue);
}

Okay, nothing too amazing in the code above. This tells me to look in WndProc. 

The events this static control uses is WM_SETCURSOR. 

 case WM_SETCURSOR:
 { 
 HWND m_handle = (HWND)wParam; 
 // Label
 for (int i = 0; i < frm.getLabelControlCount(); i++)
 {
     if (frm.getLabelControl(i).getHandle() == m_handle)
     {
          if (frm.getLinkLabelControl(i).isLink())
          {
               // Set hover color to link  
              if (frm.getLabelControl(i).isHoverColorEnabled())  
                       frm.getLabelControl(i).setColor(frm.getLabelControl(i).getHoverColor());
                       // Update cursor to hand 
                       SetClassLongPtr(frm.getLabelControl(i).getHandle(),   GCLP_HCURSOR, (LONG_PTR)frm.getLabelControl(i).getHoverCursor());
          }
      }
      else
      {
          // Set link to blue and use default arrow  
          if (frm.getLabelControl(i).isHoverColorEnabled())
             frm.getLabelControl(i).setColor(0, 0, 255);
          SetClassLongPtr(frm.getLabelControl(i).getHandle(), GCLP_HCURSOR,
         (LONG_PTR)LoadCursor(NULL, IDC_ARROW));
      }
}

  When commenting this section of code, Process Memory stays constant at 3MB. When uncommenting this section, Process Memory increases. So, this is the main code that's causing the problem apparently. 

This section of code is basically updating the label's text color based on its current mouse hovering state. It's blue when not hovered over, and it's red when hovered over.

setColor() is the following code:

void Label::setColor(const COLORREF color)
{
 m_foreColor = color;
 setFont(m_fontName, m_fontSize, m_bold, m_italic, m_underlined);
}

Which is also calling setFont() to update it:

bool Label::setFont(const std::string &fontName, const int size, const bool bold, 
 const bool italic, const bool underlined)
{  
 DWORD dwItalic;
 DWORD dwBold;
 DWORD dwUnderlined;
 SIZE linkSize;
 dwItalic = (italic) ? TRUE : FALSE;
 dwBold = (bold) ? FW_BOLD : FW_DONTCARE;
 dwUnderlined = (underlined) ? TRUE : FALSE;
 m_font = CreateFont(size, 0, 0, 0, dwBold, dwItalic, dwUnderlined, FALSE,
  ANSI_CHARSET, OUT_DEFAULT_PRECIS, CLIP_DEFAULT_PRECIS, DEFAULT_QUALITY,
  DEFAULT_PITCH | FF_SWISS, fontName.c_str());
 SendMessage(m_handle, WM_SETFONT, WPARAM(m_font), TRUE);
 
 // Calculate the correct width and height size  
 HDC hDC = GetDC(m_handle);
 SelectFont(hDC, m_font);
 GetTextExtentPoint32(hDC, m_text.c_str(), (int) m_text.length(), &linkSize);
 setSize(linkSize.cx, size);  
  
 // Store font information
 m_fontName = fontName;
 m_fontSize = size;
 m_bold = bold;
 m_underlined = underlined;
 m_italic = italic; 
  
 return true;
}

My guess is this is a lot of updating for creating a font and re-creating it based on every hover. My reasoning was it wouldn't update the font color unless setting the font again. Although I see room for this in the near future, am I forgetting to delete a resource or something here? Any thoughts are welcomed. Pretty sure this will solve LinkLabel as well. 

Phil
  • 137
  • 2
  • 10

1 Answers1

4

Your basic problem is that you keep generating new fonts and never releasing the old ones.

Each time setfont is called you allocate and select a new font. But when you select the NEW font into the HDC you never clean up the old font.

SelectFont returns the previously selected font which you need (unless it is a stock font) to do a DeleteFont on.

Additionally you have a bigger resource leak on the GetDC call - the MS documentation for getDC suggests that you use a releaseDC when you have completed the usage.

As far as I understand it is NOT required to reset the font just to reset the color.

Elemental
  • 7,365
  • 2
  • 28
  • 33
  • The font is set with `SendMessage` (which does not return the previous font). The DC part, as I understand, is only to calculate the metrics. – GSerg Jul 06 '17 at 14:30
  • 2
    There might also be another leak if getHoverCursor is creating a new cursor each time. – Adrian McCarthy Jul 06 '17 at 18:27
  • That worked! Though reseting the hovering cursor is causing issues, I believe. Using... SetClassLongPtr(frm.getLinkLabelControl(i).getHandle(), GCLP_HCURSOR, (LONG_PTR)LoadCursor(NULL, IDC_ARROW)); – Phil Jul 08 '17 at 02:00