search

eclipse / nebula

Nebula Project
https://eclipse.org/nebula
Eclipse Public License 2.0
84 stars 98 forks source link

Resource leak in VControl #513

Closed gbburkhardt closed 1 year ago

gbburkhardt commented 1 year ago

The dispose() method of VControl should dispose of the Image if 'image' is not null.

laeubi commented 1 year ago

@gbburkhardt can you explain more, e.g. give an example snippet or reference the code? In general if you set an image you should dispose it (e.g. with a dispose listener) and not the control.

gbburkhardt commented 1 year ago

I discovered this when using the CDateTime widget. One could argue that CDateTime should dispose of the image passed to 'timeButton' at line 586 of DatePicker. There is code to dispose of the image provided by org.eclipse.nebula.widgets.cdatetime.Resources, but that will only be called when an application exits. If a CDateTime widget is created, and then disposed of, the image GDI object lingers. The 'dispose' method of VControl is called when the widget is destroyed, so it's safer to put the image dispose there. Calling dispose on an Image that has been disposed does nothing.

dplaliberte commented 1 year ago

As I understand, a control should only dispose of an image, or any other resource, it it has created the resource. The control can not know if the resource is being used elsewhere.

------ Original Message ------ From "Glenn Burkhardt" @.> To "eclipse/nebula" @.> Cc "Subscribed" @.***> Date 4/18/2023 8:11:29 AM Subject Re: [eclipse/nebula] Resource leak in VControl (Issue #513)

I discovered this when using the CDateTime widget. One could argue that CDateTime should dispose of the image passed to 'timeButton' at line 586 of DatePicker. There is code to dispose of the image provided by org.eclipse.nebula.widgets.cdatetime.Resources, but that will only be called when an application exits. If a CDateTime widget is created, and then disposed of, the image GDI object lingers. The 'dispose' method of VControl is called when the widget is destroyed, so it's safer to put the image dispose there. Calling dispose on an Image that has been disposed does nothing.

— Reply to this email directly, view it on GitHub https://github.com/eclipse/nebula/issues/513#issuecomment-1512984980, or unsubscribe https://github.com/notifications/unsubscribe-auth/AOZTXXTCRZYQOCKM3KHUKTLXB2AHDANCNFSM6AAAAAAXBV23GY. You are receiving this because you are subscribed to this thread.Message ID: @.***>

laeubi commented 1 year ago

As I understand, a control should only dispose of an image, or any other resource, it it has created the resource.

Exactly!

gbburkhardt commented 1 year ago

Ok, so you choose that CDateTime has the resource leak. That's fine with me.

The root of the problem is in org.eclipse.nebula.widgets.cdatetime.Resources. Its disposeListener is only triggered when the display is disposed, which will typically only happen when the program exits, so what's the point? Any widget in the CDateTime collection that uses Resources to create an image needs to have a disposeListner created on the widget (e.g., CDateTime, DatePicker), that calls a dispose method in Resources. It looks to me that everywhere Resources.getImage() is called is leaking GDI objects, when the widget is destroyed.

lcaron commented 1 year ago

@dplaliberte This is the philosophy of SWT : if you set an image to a Button (for example), when you dispose the button the associated image is not disposed.

@gbburkhardt I'll have a look at this bug

laeubi commented 1 year ago

@gbburkhardt It is not clear for me (baybe @lcaron knows better) who creates the button, also you are talking about VControl in the title but say there is a leak in CDateTime, so it is unclear for me who is creating the image and why do you think there is a leak, usually images are keept around if they are shared, maybe up to the life-time of the application to not create them for each control individually.

gbburkhardt commented 1 year ago

There are several methods in Resources that create images, and then cache the created Image for reuse. See all the Resources.getIconXXXX() methods. I don't see any of those images being disposed when either CDateTime or DatePicker is disposed. There's a minor complication in that all those methods are static as is the cache, so if both CDateTime and DatePicker were to use the same image, one wouldn't want to dispose of the image when DatePicker is disposed.

Probably Resources could use an disposeImage(String) to get rid of a particular image if it was cached.

laeubi commented 1 year ago

If the images are shared they are also used to reuse them, disposing them would defeat the whole purpose of this. Except you assume that an application can only ever have exactly one CDateTime and never create one again.

gbburkhardt commented 1 year ago

@laeubi : that's a good point. So much for shared images.

BTW, the Resources.getIconXXXX() methods will always create an image if none exists in the cache. But if multiple instances of CDateTime are in use, disposing of any of the images is an issue.

So I withdraw my original complaint, since only a limited number of images will be created during the lifetime of an application, and new instances of CDateTime won't create new ones.

Thanks.