dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.42k stars 984 forks source link

NullReferenceException in ToolStripControlHost #2466

Closed SirIntellegence closed 1 year ago

SirIntellegence commented 4 years ago

Problem description: NullReferenceException while calling various methods on ToolStripControlHost after disposal.

Actual behavior: NullReferenceException thrown

System.NullReferenceException: Object reference not set to an instance of an object. + System.Windows.Forms.ToolStripControlHost.SetVisibleCore(bool)

Expected behavior: ObjectDisposedException thrown

Minimal repro: The following code was used in Roslin to reproduce the issue

r "System.Windows.Forms"

using System.Windows.Forms; Button btnTest = new Button(); ToolStripControlHost host = new ToolStripControlHost(btnTest); host.Dispose(); host.Visible = true;

Note: From what I see in the source code, it does not need to be on a visible form to reproduce.

Suspected Cause Dispose sets control to null

Suggested Solution Check for null in the Control property and throw an ObjectDisposedException if it is null

RussKie commented 4 years ago

Looking at the code, a NRE is expected, since the control is set to null. https://github.com/dotnet/winforms/blob/971343425f4e76dfc4b8fccfb87cbca278245fdc/src/System.Windows.Forms/src/System/Windows/Forms/ToolStripControlHost.cs#L605-L607

I venture to say ToolStripControlHost was designed as primarily used by the designer. Few subclasses create their own controls, so their disposal is expected.

Could you please provide more information on your use case?

weltkante commented 4 years ago

Could you please provide more information on your use case?

Not speaking for OP but considering his suggested solution is

Check for null in the Control property and throw an ObjectDisposedException if it is null

I would guess he wants things to be easier to diagnose. You sometimes have event handlers triggering after disposal and when you just get a NRE you have to manually debug the problem since you don't see in the stack trace of the event handler that the control was disposed, if you were getting an ODE you would know right away what the problem is.

SirIntellegence commented 4 years ago

What @weltkante said is correct. I was notified of this via a crash report and the stack trace doesn't give me much beyond that it was called from BackgroundWorker.OnRunWorkerCompleted. Maybe the cause is related to the program exiting while it is starting? I am not sure, but there isn't much I can do about that. It would just be nice if a more specific exception was thrown instead of the common and generic NRE.

RussKie commented 4 years ago

I'd like to understand the use case for ToolStripControlHost why and how it is used.

SirIntellegence commented 4 years ago

I honestly don't know. It just showed up in a crash report. I don't see any references to it in our code either. If I take a look at the objects in a Snapshot, it looks like almost everything has weak references of that type (530 instances), but no instances... I suspect that some internal .net code decided to use it for something. Maybe if we added a stock control to a toolstrip?

weltkante commented 4 years ago

According to doc this class seems to be used for custom controls in toolbars. There are quite a few builtin subclasses as well, e.g. ToolStripComboBox, ToolStripTextBox, etc. - basically anything that isn't a simple toolstrip button/dropdown will subclass this to host a "normal" control in the toolbar.

I assume you have to go looking for subclasses if you want to find where/how its used, while it can be used standalone for custom controls it seems subclassing is the more common usage.

SirIntellegence commented 4 years ago

That makes sense. We do use ToolStripComboBox and some others. That is likely how it is happening.

KlausLoeffelmann commented 4 years ago

@RussKie: Can this be closed or is this a bug?

weltkante commented 4 years ago

Its an improvement request to throw a better exception.

It is a user mistake triggering a bug in WinForms, calling into a disposed control throws NRE instead of ObjectDisposedException (or similar). This makes it impossible to identify the cause from stack traces, you have to reconstruct the error scenario and debug it to figure out you were doing something wrong.

If WinForms would check control status and throw ObjectDisposedException then it would be clear from the stacktrace that the user code is operating on a disposed control.

elachlan commented 1 year ago

I think this property could be refactored to throw an ObjectDisposedException: https://github.com/dotnet/winforms/blob/23d780b983842318627793577e1a6bb5e4673453/src/System.Windows.Forms/src/System/Windows/Forms/ToolStripControlHost.cs#L133

I am unsure on the private field though, We would have to add a check for each.

Should we also make use of ObjectDisposedException.ThrowIf()?

Philip-Wang01 commented 1 year ago

Verified this on .NET latest build: 8.0.100-preview.2.23116.28, issue was fixed. image

Amy-Li03 commented 1 year ago

Verified this on .NET 8.0 Preview 2.0 TP build: 8.0.100-preview.2.23151.8, issue was fixed, ObjectDisposedException thrown while calling methods on ToolStripControlHost after disposal. Test result is same as above screenshot.