Control can't be properly GC-collected after disposing because of it referenced via the System.Windows.Forms.Control.cachedLayoutEventArgs field

[DmitryGaravsky]

The same bug is reproducible with .NET Framework but it seems it has no chance to be ever fixed.
But the .NET Core looks like a good place for introducing some improvements in this area.

Problem description

Each instance of the System.Windows.Forms.Control type contains a private member variable of the LayoutEventArgs type - cachedLayoutEventArgs . And, the LayoutEventArgs instance typically contains a reference to some specific control.
Sometimes, the cachedLayoutEventArgs field is not cleared when the child control disposing of does not affect the layout process of the parent control due to some reasons.

Reproduction

Here are the minimal reproduction-steps:

1. Create a form with two buttons.
2. Add the following code for the corresponding Button.Click handlers:

void btnOpenView_Click(object sender, System.EventArgs e) {
    var view = new Panel() { BackColor = Color.Red };
    view.Name = "View";
    view.Bounds = new Rectangle(100, 100, 100, 100);
    view.Parent = this;
}
void btnCloseView_Click(object sender, System.EventArgs e) {
    SuspendLayout();
    var view = this.Controls.Find("View", false)[0];
    if(view != null)
        view.Dispose();
    ResumeLayout(false);
}

3. Press the "Open View" button - red panel appears.
4. Press the "Close View" button - red panel disappears.

Actual behavior:
The panel is still referencing via the System.Windows.Forms.Control.cachedLayoutEventArgs field at the form level. As result, it can't be GC-collected properly and still in memory.

Expected behavior:
The panel should not be referenced.

Proposal for Fix

It looks like we should use the WeakReference when caching the LayoutEventArgs.
As an alternative solution, we can validate the cachedLayoutEventArgs value on child control removing and clear the problematical field.

[zsd4yr]

Sometimes, the cachedLayoutEventArgs field is not cleared when the child control disposing does not affect the layout process of the parent control due to some reasons.

Could you be more clear about these scenarios? What is not triggering when the child control disposing does not affect the layout process of the parent control?

[DmitryGaravsky]

Hi, @zsd4yr

Could you be more clear about these scenarios?

From my experience, there are two cases when this issue is 100% reproducible:

• manual locking of layout processing(already demonstrated in the Reproduction section);
• removing a control from an invisible container (inactive tab-page for example);

Here are the steps for last case reproduction:

1. Create a form with two buttons and TabControl with two pages.
2. Add the following code for the corresponding Button.Click handlers:

void btnAddViewToTab_Click(object sender, System.EventArgs e) {
    var view = new Panel();
    view.Name = "View";
    view.Dock = DockStyle.Fill;
    view.Parent = tabPage2;
}
void btnRemoveViewFromTab_Click(object sender, System.EventArgs e) {
    var view = tabPage2.Controls.Find("View", false)[0];
    if(view != null)
        view.Dispose();
}

3. Press the "Add View to Tab" button.
4. Press the "Remove View from Tab" button.
   After this, the panel is still referenced via the System.Windows.Forms.Control.cachedLayoutEventArgs field at the tabPage2 level.

Also, there are some scenarios when this issue is reproducible from time to time:

• closing an MDI-child form within the MDI-parent form;
• removing the control from the controls hierarchy when it placed at the 12-15 level (x64 windows only).
  IMO, the last case is related to the known windows design limitation.

[KlausLoeffelmann]

I'll take a look!

[weltkante]

@KlausLoeffelmann @DmitryGaravsky

Proposal for Fix
It looks like we should use the WeakReference when caching the LayoutEventArgs.

I think this won't work, if nobody holds a reference to the LayoutEventArgs it will be collected immediately, making the cache useless. What you want is a ConditionalWeakTable where the key is the Control and the value the LayoutEventArgs (ConditionalWeakReference does not exist unfortunately, and the framework API required to build one is internal).

[DmitryGaravsky]

@weltkante

if nobody holds a reference to the LayoutEventArgs it will be collected immediately, making the cache useless.

Yes, you're absolutely correct, but I proposed not the exact replacement of LayoutEventArgs cachedEventArgs; with the WeakReference cachedEventArgs; but "something based on weak-references" that can resolve this issue.
As an alternative solution, we can validate the cachedLayoutEventArgs value on child control removing and clear the problematical field.

[zsd4yr]

@KlausLoeffelmann, we have been very busy these last few weeks! When you get a chance, could you update this thread with any progress?

[KlausLoeffelmann]

Sure. We should re-prioritize issues this week, anyway, but I am guessing, this will be waiting a little time longer.

[elachlan]

Could we refactor the LayoutEventArgs so that AffectedComponent uses a WeakReference backing field?

weltkante also raised an issue for the ConditionalWeakReference in dotnet/runtime#30759.

[elachlan]

@Olina-Zhang can your team verify this issue?

[weltkante]

weltkante also raised an issue for the ConditionalWeakReference in dotnet/runtime#30759.

While there isn't a ConditionalWeakReference the previously internal API is now public and you could use System.Runtime.DependentHandle to get the underlying functionality or build an internal version of ConditionalWeakReference.

Note there are GC issues around dependent handles that can cause memory leaks if used incorrectly, since their original design was for static caches (ConditionalWeakTable). See this comment for a summary or the dotnet/runtime#12255 for the main issue.

Could we refactor the LayoutEventArgs so that AffectedComponent uses a WeakReference backing field?

I don't see any immediate problem, could work

[elachlan]

See #9393 for the PR, I would love some feedback.

[Amy-Li03]

Hi @elachlan @DmitryGaravsky, we try to verify this issue on WinDbg, no matter before and after disposing Panel, we all get 1 object using !DumpHeap -type Panel command. If we have done something wrong, please tell us how we should check the behavior: The panel is still referencing via the System.Windows.Forms.Control.cachedLayoutEventArgs field at the form level.

[elachlan]

@Amy-Li03 maybe try: #165 (comment)

[weltkante]

Not clear if its been considered but for WeakReference to be cleared you may have to wait (or induce) a GC. This wasn't relevant before, but with the proposed fix you have to consider this.

GC.Collect(); // clear normal weak references, collect normal objects, mark objects with destructor for finalization
GC.WaitForPendingFinalizers(); // wait until the finalizer callbacks have been processed
GC.Collect(); // clear long weak references, removes finalizeable objects from memory
// now take e.g. a memory snapshot in VS and look at whats still holding the objects alive

[Olina-Zhang]

Verified in the latest 8.0 SDK build: 8.0.100-rc.1.23375.11, it was fixed: there is no related panel leaks after GC-collected.

[Nora-Zhou01]

Verified with .NET SDK 8.0.100-preview.7.23376.3 test pass build, this issue was fixed, test result same as above.