Opened 8 months ago

Closed 7 months ago

Last modified 7 months ago

#16068 closed defect (fixed)

Resource leak in wxStaticBitmap on changing the bitmap

Reported by: awi Owned by:
Priority: normal Milestone:
Component: wxMSW Version: dev-latest
Keywords: wxStaticBitmap alpha 32-bit bitmap Cc:
Blocked By: Blocking:
Patch: yes

Description

When wxStaticBitmap holds 32-bit bitmap with alpha then when this bitmap is replaced with the new one using wxStaticBitmap::SetBitmap() the previous bitmap is not released.
This is due to the fact that STM_SETIMAGE message handler creates a "secret copy" when 32-bit bitmap is loaded into the control and this copy is not released properly (in wxStaticBitmap::SetImageNoCopy()) when the new bitmap arrives.

Test case to reproduce the issue below:

  • samples/minimal/minimal.cpp

    old new  
    172172    CreateStatusBar(2); 
    173173    SetStatusText("Welcome to wxWidgets!"); 
    174174#endif // wxUSE_STATUSBAR 
     175    wxImage::AddHandler( new wxPNGHandler ); 
     176    SetBackgroundColour(wxColour(180, 210, 230)); 
     177 
     178    wxBitmap bmpCross32("cross_32.png", wxBITMAP_TYPE_PNG); 
     179    wxBitmap bmpArrow32("arrow_32.png", wxBITMAP_TYPE_PNG); 
     180    wxBitmap bmpCross24("cross_24.png", wxBITMAP_TYPE_PNG); 
     181    wxBitmap bmpArrow24("arrow_24.png", wxBITMAP_TYPE_PNG); 
     182 
     183    const int limit = 5 * 1000; 
     184    new wxStaticText(this, wxID_ANY, wxT("32 -> 32"), wxPoint(10, 0)); 
     185    wxStaticBitmap *sb1 = new wxStaticBitmap(this, wxID_ANY, bmpCross32, wxPoint(10, 20)); 
     186    for( int i = 0; i < limit; i++) 
     187    { 
     188        sb1->SetBitmap(bmpArrow32); 
     189        sb1->SetBitmap(bmpCross32); 
     190    } 
     191 
     192    new wxStaticText(this, wxID_ANY, wxT("32 -> 24"), wxPoint(100, 0)); 
     193    wxStaticBitmap *sb2 = new wxStaticBitmap(this, wxID_ANY, bmpCross32, wxPoint(100, 20)); 
     194    for( int i = 0; i < limit; i++) 
     195    { 
     196        sb2->SetBitmap(bmpArrow24); 
     197        sb2->SetBitmap(bmpCross32); 
     198    } 
     199 
     200    new wxStaticText(this, wxID_ANY, wxT("24 -> 24"), wxPoint(10, 80)); 
     201    wxStaticBitmap *sb3 = new wxStaticBitmap(this, wxID_ANY, bmpCross24, wxPoint(10, 100)); 
     202    for( int i = 0; i < limit; i++) 
     203    { 
     204        sb3->SetBitmap(bmpArrow24); 
     205        sb3->SetBitmap(bmpCross24); 
     206    } 
     207 
     208    new wxStaticText(this, wxID_ANY, wxT("24 -> 32"), wxPoint(100, 80)); 
     209    wxStaticBitmap *sb4 = new wxStaticBitmap(this, wxID_ANY, bmpCross24, wxPoint(100, 100)); 
     210    for( int i = 0; i < limit; i++) 
     211    { 
     212        sb4->SetBitmap(bmpArrow32); 
     213        sb4->SetBitmap(bmpCross24); 
     214    } 
    175215} 
    176216 
    177217 

On my machine (Win 7 32-bit) after about 5000 iterations the static bitmaps are no longer displayed and also some artifacts appear (screenshot attached).

Patch to fix the issue is also attached.

Attachments (7)

cross_24.png download (638 bytes) - added by awi 8 months ago.
24-bit image.
arrow_24.png download (615 bytes) - added by awi 8 months ago.
24-bit image.
cross_32.png download (510 bytes) - added by awi 8 months ago.
32-bit image.
arrow_32.png download (597 bytes) - added by awi 8 months ago.
32-bit image.
staticbmp.png download (14.5 KB) - added by awi 8 months ago.
Screenshots with result output.
statbmp-load-image.patch download (736 bytes) - added by awi 8 months ago.
Patch to fix releasing 32-bit bitmap.
statbmp-free.patch download (655 bytes) - added by awi 8 months ago.
Release 32-bit bitmap when the control is destroying.

Download all attachments as: .zip

Change History (15)

Changed 8 months ago by awi

24-bit image.

Changed 8 months ago by awi

24-bit image.

Changed 8 months ago by awi

32-bit image.

Changed 8 months ago by awi

32-bit image.

Changed 8 months ago by awi

Screenshots with result output.

Changed 8 months ago by awi

Patch to fix releasing 32-bit bitmap.

comment:1 Changed 8 months ago by VZ

  • Resolution set to fixed
  • Status changed from new to closed

(In [76141]) Fix resource leak in wxMSW wxStaticBitmap when using RGBA icons.

Release the old handle if it wasn't freed by STM_SETIMAGE itself.

Closes #16068.

comment:2 Changed 8 months ago by VZ

(In [76142]) Fix resource leak in wxMSW wxStaticBitmap when using RGBA icons.

Release the old handle if it wasn't freed by STM_SETIMAGE itself.

Closes #16068.

Changed 8 months ago by awi

Release 32-bit bitmap when the control is destroying.

comment:3 Changed 8 months ago by awi

  • Keywords 32-bit bitmap added; SetBitmap removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

The same resource leak occurs not only if bitmap is replaced but also if wxStaticBitmap object is destroyed.
On my machine, after ~10000 deletions of wxStaticBitamp object the application hangs and many messages like this one are recorded in the debugger:
..\..\src\msw\brush.cpp(211): 'CreateXXXBrush()' failed with error 0x00000006 (invalid handle.).
Apparently, the GDI resources have been exhausted.

Patch to reproduce the issue:

  • samples/minimal/minimal.cpp

    old new  
    172172    CreateStatusBar(2); 
    173173    SetStatusText("Welcome to wxWidgets!"); 
    174174#endif // wxUSE_STATUSBAR 
     175    wxImage::AddHandler( new wxPNGHandler ); 
     176    SetBackgroundColour(wxColour(180, 210, 230)); 
     177 
     178    wxBitmap bmpARGB(wxT("arrow_32.png"),  wxBITMAP_TYPE_PNG); 
     179    wxASSERT( bmpARGB.HasAlpha() && bmpARGB.GetDepth() == 32 ); 
     180 
     181    for(int i = 0; i < 10000; i++) 
     182    { 
     183        wxStaticBitmap *sb = new wxStaticBitmap(this, wxID_ANY, bmpARGB, wxPoint(115, 5)); 
     184        delete sb; 
     185    } 
     186    new wxStaticBitmap(this, wxID_ANY, bmpARGB, wxPoint(115, 5)); 
    175187} 
    176188 
    177189 

Patch to fix the issue attached (attachment:ticket:16068:statbmp-free.patch).

comment:4 follow-up: Changed 7 months ago by vadz

Thanks! This looks correct to me, but I don't like the code duplication, so I've refactored the code freeing the old handle in its own function. Please let me know if I broke anything in the process.

And thanks again!

comment:5 Changed 7 months ago by VZ

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [76145]) Also free internal handlers when wxMSW wxStaticBitmap is destroyed.

r76141 fixed the resource leak when wxStaticBitmap image was replaced by
another one but the leak still happened at the end, when the wxStaticBitmap
was destroyed.

Fix it there as well in the same way.

Closes #16068.

comment:6 Changed 7 months ago by VZ

(In [76147]) Also free internal handlers when wxMSW wxStaticBitmap is destroyed.

r76142 fixed the resource leak when wxStaticBitmap image was replaced by
another one but the leak still happened at the end, when the wxStaticBitmap
was destroyed.

Fix it there as well in the same way.

Closes #16068.

comment:7 in reply to: ↑ 4 Changed 7 months ago by awi

Replying to vadz:

Thanks! This looks correct to me, but I don't like the code duplication, so I've refactored the code freeing the old handle in its own function. Please let me know if I broke anything in the process.

Everything look fine.

I always have doubts whether introducing new functions is allowed.
Definitely, I need to read ABI rules. :)

comment:8 Changed 7 months ago by vadz

Introducing new (non virtual) functions is fine, it's the new member which is not, because it changes the size of the object. See this tech note for more information.

Note: See TracTickets for help on using tickets.