[Bug]: Memory leak when disposing Krypton.Toolkit.PaletteBase and derived classes.

[MarthinusR]

Describe the bug
The class "Krypton.Toolkit.PaletteBase" does not unsubscribe its "OnUserPreferenceChanged" method from the static rooted event "SystemEvents.UserPreferenceChanged", leading to objects persistently maintained in second generation garbage, i.e. memory leaks.

Derived classes also instantiate member controls that do not get disposed and become dangling objects.

To Reproduce
Steps to reproduce the behavior:

1. Start the "TestForm" project.
2. Click on "Buttons" to open the "ButtonsTest" form.
3. Click on the close button.
4. "PaletteBase" objects will not be collected on close.

Expected behavior
All "PaletteBase" objects must be garbage collected.

Screenshots

Desktop:

• OS: Windows 11
• Version: 10.0.22621
• Framework/.NET Version: 4.7.2
• Toolkit Version: 90.25.1.27

Additional context
Some work has been done (Krypton_memory_fix.patch) to clean up resources, but due to unfamiliarity with the project it is unclear who the oner of created objects are. KryptonReadOnlyControls espesially is a point of concern and should be addressed. The provided patch might not be stable.

[giduac]

Discussion opened on the KDev server to review this proposal:
https://discord.com/channels/1261930150585569350/1333697703154286652

[MarthinusR]

The patch "Krypton_master_abfebc02_memory_fix_20250210_0809.patch" roughly provides a 5 fold reduction in memory usage in our software. Very basic testing was done and it seems to be stable. For the full effect to take place, one must ensure that all controls that are created in the designer gets disposed when a form closes (take note that some controls created in the InitializeComponent(), might not be attached to the form's "Controls", and these might have to be disposed manually).

This patch is intended to be used on the master branch at commit id abfebc0. A corresponding patch for the alpha branch is provided at commit id 2803aec, though it was not tested, as this revision cannot run on our software at this time.

Take note that the intent of this patch is to get a reasonable amount of memory to be deallocated, and there is place for improvement as it was done without finesse to get at a reasonable position.

Krypton_master_abfebc02_memory_fix_20250210_0809.patch

Krypton_alpha_2803aec6_memory_fix_20250210_0945.patch

[giduac]

Hi @MarthinusR,

Thanks for the additional testing and info. We did not discussed this so far (time is rationed...).

• Could you please illustrate the memory reduction with some numbers so we can this into account as well ?
• Is it only the memory usage that is affected or are there other aspects this has a positive effect on?

Thanks.

[MarthinusR]

Hi @giduac,

Sure thing, though as stated the testing was rudimentary and there probably are things lurking that should be addressed. Memory usage was the focus, but I suspect that performance might be negatively impacted with the additional housekeeping. The changes might also interfere with caching.

Test 1

Testing was done manually where 10 "files" were opened and closed to perform the comparison, with basic interaction. Said files have a workspace with 13 pages/tabs, though for this test only one page was loaded (take note that these sub pages have lazy loading). Afterwards garbage collection was forced until no more objects could be collected.

Improvements

With the changes applied there is a difference of 40.25 MB total used (316.4 MB - 276.15 MB) when comparing the total memory used before opening the files and after the 10 files have been opened. This indicates that roughly 4 MB is being leaked per file opened (this is not perfect but effectively good enough)

Original

Comparing the same procedure with the unchanged program, we see that there is 213.5 MB (504.6 MB - 291.1 MB) remaining after the process. That is 21.35 MB per file. Comparing this with the improvement it is roughly 5 times better (21 MB ÷ 4 MB ~= 5)

Test 2

Testing was done manually where 4 "files" and 1 "file" was opened and closed to perform the comparison, for the applied changes and original respectively, with basic interaction. Said files have a workspace with 13 pages/tabs, and all pages were loaded. Afterwards garbage collection was forced until no more objects could be collected.
NOTE: Only one sample was taken for the original, as there was an issue with some of the sub pages that would freeze on the second time opened (later it was discovered that this was caused by a concurrent thread, where Invoke() was causing apparent deadlock)

Improvements

With the changes applied there is a difference of 28.58 MB total used (313.4 MB - 284.82 MB) when comparing the total memory used before opening the files and after the 4 files have been opened. This indicates that roughly 7 MB is being leaked per opened file and sub pages.

Original

Comparing the same procedure with the unchanged program, we see that there is 72.4 MB (438.5 MB - 366.1 MB) remaining after the process. Comparing this with the improvement it is roughly 10 times better (72 MB ÷ 7 MB ~= 10)

[giduac]

Hi @MarthinusR,
cc: @PWagner1 & @Smurf-IV

Unsubscribing from events is something that should take place when the item is not "needed" any more.

Not sure if all those dispose routines should be called manually taking over the work of the GC. Which might have an adverse effect on performance. Those objects inside these classes with a single reference can just as well be nulled and left to the GC which will schedule them for disposal.

Also needs to show if the memory savings come from manual/forced disposal. I'm on the fence here. Although application wise it's quite a saving. Considering system memory sizes nowadays it's not much. Just have a look how much your web browser slurps away.

...... ??

[MarthinusR]

Hi @giduac
cc: @PWagner1 & @Smurf-IV

Unsubscribing from events is something that should take place when the item is not "needed" any more.

Ideally yes, though as stated in the issue the method "OnUserPreferenceChanged" gets bound to the static rooted event "SystemEvents.UserPreferenceChanged", so the system will never know when it is not "needed" any more, as there will always be at least one reference to the object.

Not sure if all those dispose routines should be called manually taking over the work of the GC. Which might have an adverse effect on performance. Those objects inside these classes with a single reference can just as well be nulled and left to the GC which will schedule them for disposal.

Manually calling dispose is needed to detach from said static rooted event. If you can manage this without dispose then by all means.

I guess you could create a test program that will create new forms and close them continuously. You can observe the memory usage over time and the leak should be visible.

[MarthinusR]

Hi @giduac
cc: @PWagner1 & @Smurf-IV

I have performed a simple test (take note that this was done on x86) and confirmed that the issue can be reproduced:

(task manager extract)

(exception)

The following changes were applied:

[giduac]

Hi @MarthinusR,

Did it also crash if an empty form is used?

Something like

var f = new KryptonForm();
f.Show().
f.Close().

instead of workspaceTest()

Or try this with a button or any other control?

[MarthinusR]

Hi @giduac

With the following changes:

The system exhausted the available win32 handles:

[giduac]

it crashes in under 10,000 iterations.
Interestingly when swapping Kcombo with a winforms combo it runs without problem. Tried this up to 100K iterations.

If Dispose is called on the kcombo all is clear...

Is it possible for you (or are you interested) to create a test branch which solve this problem for 1 component only that could serve as an example of how to approach this ?

If so please read the chapter/pages on contributing for the details and how to get going:
https://krypton-suite.github.io/Standard-Toolkit-Online-Help/Source/Help/Output/articles/Contributing.html

[giduac]

Hi @MarthinusR,
cc: @PWagner1 & @Smurf-IV && @Ahmed-Abdelhameed

We are all busy outside of this project and already have plenty of work planned in for V100, the next release. So there's no way this will be addressed any time soon.

Since you have already put in quite a bit of work on this you have become more or less familiar with the matter.
All can be discussed a little further to organize and plan this change, make it go smooth.
It should be a change for at least V95 (V90 patch releases) and V100.

So I will ask you directly, would you like to contribute by making this change yourself. The work can be done as you have time for it of course. And you will have our full support and we will get you started.

Let us know ....