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

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

Open
DmitryGaravsky opened this Issue Dec 5, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@DmitryGaravsky

DmitryGaravsky commented Dec 5, 2018

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);
}
  1. Press the "Open View" button - red panel appears.
  2. 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.

@merriemcgaw merriemcgaw added the bug label Dec 5, 2018

@zsd4yr

This comment has been minimized.

Member

zsd4yr commented Dec 5, 2018

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?

@merriemcgaw merriemcgaw added this to the Future milestone Dec 5, 2018

@DmitryGaravsky

This comment has been minimized.

DmitryGaravsky commented Dec 6, 2018

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();
}
  1. Press the "Add View to Tab" button.
  2. 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 KlausLoeffelmann self-assigned this Dec 6, 2018

@KlausLoeffelmann

This comment has been minimized.

Member

KlausLoeffelmann commented Dec 6, 2018

I'll take a look!

@weltkante

This comment has been minimized.

weltkante commented Dec 6, 2018

@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

This comment has been minimized.

DmitryGaravsky commented Dec 6, 2018

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment