Opened 16 months ago

Closed 16 months ago

Last modified 16 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 16 months ago.
24-bit image.
arrow_24.png download (615 bytes) - added by awi 16 months ago.
24-bit image.
cross_32.png download (510 bytes) - added by awi 16 months ago.
32-bit image.
arrow_32.png download (597 bytes) - added by awi 16 months ago.
32-bit image.
staticbmp.png download (14.5 KB) - added by awi 16 months ago.
Screenshots with result output.
statbmp-load-image.patch download (736 bytes) - added by awi 16 months ago.
Patch to fix releasing 32-bit bitmap.
statbmp-free.patch download (655 bytes) - added by awi 16 months ago.
Release 32-bit bitmap when the control is destroying.

Download all attachments as: .zip

Change History (15)

Changed 16 months ago by awi

24-bit image.

Changed 16 months ago by awi

24-bit image.

Changed 16 months ago by awi

32-bit image.

Changed 16 months ago by awi

32-bit image.

Changed 16 months ago by awi

Screenshots with result output.

Changed 16 months ago by awi

Patch to fix releasing 32-bit bitmap.

comment:1 Changed 16 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 16 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 16 months ago by awi

Release 32-bit bitmap when the control is destroying.

comment:3 Changed 16 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 16 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 16 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 16 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 16 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 16 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.