Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maybe fix memory leak caused by not disposing Icons and Bitmaps. #262

Merged
merged 1 commit into from
May 11, 2018

Conversation

Niko-O
Copy link
Contributor

@Niko-O Niko-O commented May 11, 2018

Icon implements IDisposable and must therefore be disposed.
AudioDeviceIconExtractor creates and caches a lot of icons but never
frees old cache entries. TrayIcon.UpdateIcon also creates many copies of
Resources.SoundSwitch16. Getters in the generated Resource class always
return new instances of Bitmaps and Icons.
This commit removes the caching and adds code to manually dispose icons
used in various locations. Some uses of the Resource class are cached
instead (where the resource is not disposed anyways) to avoid loading
the same resource multiple times from disk. I didn't touch generated
files, though.

This commit is untested since I cannot compile the project.

Icon implements IDisposable and must therefore be disposed.
AudioDeviceIconExtractor creates and caches a lot of icons but never
frees old cache entries. TrayIcon.UpdateIcon also creates many copies of
Resources.SoundSwitch16. Getters in the generated Resource class always
return new instances of Bitmaps and Icons.
This commit removes the caching and adds code to manually dispose icons
used in various locations. Some uses of the Resource class are cached
instead (where the resource is not disposed anyways) to avoid loading
the same resource multiple times from disk. I didn't touch generated
files, though.

This commit is untested since I cannot compile the project.
@Niko-O
Copy link
Contributor Author

Niko-O commented May 11, 2018

Leaving Icons laying around eventually triggers an ExternalException when calling Bitmap.GetHicon, caused by GDI+ not wanting to give you any more handles. This has happened to a friend, who has asked me to maybe try and fix it.
While I was at it, and since it overlaps in one specific instance, I have changed all uses of the generated Resource class (except in generated code). Getters in this class always load the resource from disk. I have added rudimentary code to cache the returned objects.

Since I cannot compile the project (Visual Studio can't find some of the referenced assemblies, like NAudio) I cannot test this fix. Please make sure to test everything before merging.
There may be some cases I have missed, so the issue might not be completely fixed.

So in short:
Using a getter in the generated Resource class creates a new object.
Calling Bitmap.GetHicon creates a new unmanaged resource, which is automatically freed when the Icon created by Icon.FromHandle with the returned handle is disposed.
Calling Icon.ToBitmap creates a new object.
And it might even be the case that the setter of NotifyIcon.Icon internally creates a new object too. This does happen with the setter of Form.Icon.

@Belphemur Belphemur merged commit 4a7504d into Belphemur:dev May 11, 2018
@Belphemur
Copy link
Owner

Hello Niko,

Thanks for this, I see the problem.

I've merged your changes, but with keeping the cache, only a different implementation, an expirable one. If the Icon is not retrieved for 5 minutes, it's disposed and removed from the cache.

This should resolve any issue there were with the icons.

@Belphemur
Copy link
Owner

Also about NAudio, you need to fetch the NuGet package for the project to build.

@Niko-O
Copy link
Contributor Author

Niko-O commented May 12, 2018

Great to hear that. Caching icons for 5 minutes should work fine.
If you have any questions about my pull request or if there are any problems with it, don't hesitate to contact me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants