dotnet / winforms

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

Metroframework library worked in .NET 6 and Framework -- Crashes on .NET 7 #8803

Closed adegeo closed 1 year ago

adegeo commented 1 year ago

.NET version

7.0

Did it work in .NET Framework?

Yes

Did it work in any of the earlier releases of .NET Core or .NET 5+?

Worked on .NET 6

Issue description

I was updating documentation on how to upgrade from .net 4 to .NET 6, updating the steps to .NET 7. The app I was using worked fine on .NET 6, but on .NET 7 it crashes unable to create the starting window.

I'm using a 3rd party library from NuGet that was made for .NET Framework, I chose it so that I could demonstrate upgrading NuGet packages. The bug is most certainly from the usage of that library, but the library hasn't been updated in a decade. But, like I was saying, it was written for .NET Framework, worked fine on .NET Core 3.1/.NET 5/.NET 6, but no longer with .NET 7. The designer seems to be fine.

The library is this: Metroframework (thielj.github.io) NuGet: NuGet Gallery | MetroFramework 1.2.0.3

Steps to reproduce

MatchingGame_Net7.zip code

Here is a sample project. If you change the TFM to .NET 6, it works fine. When you keep it as .NET 7, you get this crash

image

adegeo commented 1 year ago

@JeremyKuhne

JeremyKuhne commented 1 year ago

A simpler repro:

  1. Create a WinForms app.
  2. Add the MetroFramework nuget package.
  3. Change Form1 to derive from MetroForm.
  4. Run the app.

CreateWindowEx returns ERROR_INVALID_WINDOW_HANDLE when targeting .NET 7. Nothing obvious sticks out as to why unfortunately.

JeremyKuhne commented 1 year ago

https://github.com/dotnet/winforms/commit/834d0a0d364c82bf70803706886ff9a40bd3e090 is where this started failing. Still not sure why precisely.

JeremyKuhne commented 1 year ago

Well, this is fun. I built MetroFramework to try debug what is going on and it doesn't repro. That said, I did let the solution migrate forward to 4.8 (as 4.0 is out of support), and I did have to fix an issue with type loading where it was assuming all System.Drawing types come from the same assembly as Color.

It was building for the .NET client profile, but I can't imagine any of the target framework stuff was causing the problem.

JeremyKuhne commented 1 year ago

I'm going to try and build to the same commit as in the released version (there are no tags / branches for the released version). Fwiw- fixing master to work on .NET core requires the following in the MetroStyles class:

        private static Type GetType(string type)
        {
            if (type.StartsWith("System.Drawing.") && type.IndexOf(',') < 0)
            //    type += ", " + typeof(Color).Assembly.FullName;
            {
                // In .NET Core System.Drawing types are in two different assemblies. Font is
                // in System.Drawing.Common and Color is in System.Drawing.Primitives.
                Type result = typeof(Font).Assembly.GetType(type, throwOnError: false);
                if (result != null)
                {
                    return result;
                }

                return typeof(Color).Assembly.GetType(type, throwOnError: true);
            }

            return Type.GetType(type, true);
        }
JeremyKuhne commented 1 year ago

Ok, set to commit 12eb532971caab842b3ab58803e58d72c30e2e37 it repros (even upgraded to 4.8).

JeremyKuhne commented 1 year ago

Stand alone repro:

MyForm form = new();
form.ShowDialog();

public partial class MyForm : Form
{
    protected override void OnHandleCreated(EventArgs e)
    {
        base.OnHandleCreated(e);
        _ = new ParentForm(this);
    }
}

public class ParentForm : Form
{
    public ParentForm(Form targetForm)
    {
        targetForm.Owner = this;
        ShowInTaskbar = false;
    }
}
kirsan31 commented 1 year ago

There was a pr about ShowInTaskbar : https://github.com/dotnet/winforms/pull/6989 May be it's related?

----- UPD -----

Oh https://github.com/dotnet/winforms/commit/834d0a0d364c82bf70803706886ff9a40bd3e090 was eralyer, so so it's unlikely...

JeremyKuhne commented 1 year ago

Ugh- I can't seem to figure out how my change may have messed this up. Maybe I traced to the wrong commit? It isn't particularly easy to rebuild these old commits as you have to fiddle with package dependency versions. In any case I'm going to try and compare the code flow with this smaller repro now. This isn't fun as the failure point is disconnected from whatever caused it.

JeremyKuhne commented 1 year ago

ShowInTaskBar is a bit of a red herring. The problem is RecreateHandle on the Form when it is the parent of another Form.

    public ParentForm(Form targetForm)
    {
        targetForm.Owner = this;
        RecreateHandle();
    }
JeremyKuhne commented 1 year ago

It was my fault. I looked at the change so many times I couldn't see the obvious mistake. As the child window is still being created when it creates its parent, not properly updating the parent HWND when it is recreated causes CreateWindow to return null.

This is a particularly nasty bug so I'm going to put it up for servicing.

MelonWang1 commented 1 year ago

Verified this on .NET 8.0 latest build: 8.0.100-preview.4.23179.4, issue is fixed. Running app with no error and designer works well. 8803

Nora-Zhou01 commented 1 year ago

Verified this on .NET 8.0 Preview 3.0 TP build: 8.0.100-preview.3.23178.7, issue was fixed. The screenshot as same as above.

Thraka commented 1 year ago

@JeremyKuhne if it's in servicing, does that mean there'll be a 7.0 release with the fix?

dreddy-work commented 1 year ago

@JeremyKuhne if it's in servicing, does that mean there'll be a 7.0 release with the fix?

Yes. 7.0 servicing fix is scheduled for May release. 7.0.6