microsoft / CsWin32

A source generator to add a user-defined set of Win32 P/Invoke methods and supporting types to a C# project.
MIT License
2.08k stars 87 forks source link

Reconsider taking HandeRef as input parameters when the parameter must be a handle of some sort. #202

Open AraHaan opened 3 years ago

AraHaan commented 3 years ago

Currently in CsWin32 no overloads taking in HandleRefs are generated.

The use cases:

This is where my code uses it: https://github.com/Elskom/Els_kom_new/search?q=HandleRef&type=code (it seems to not show every usage of it for some reason in my NativeMethods.cs file.)

Got 616 results for it as well too https://github.com/search?q=org%3Adotnet+HandleRef&type=code, a lot of commits, and issues regarding it as well.

More results could be in the code bases within all of the repositories in the dotnet organization on github, one way to find out for sure how many results come up is to clone them all, and then within Visual Studio search for HandleRef in every last repository cloned from there to get the absolute number of usages in the code within that organization.

I also encourage an in general look to see how many other people might also use HandleRef as well too.

As seen above, HandleRef while documented as deprecated in .NET 2.0, is still widely used everywhere even within the runtime itself with valid use cases that make it impossible to use say SafeHandle, expecially when you own the object that made the handle but not the handle itself and as such using HandleRef-or GC.KeepAlive(objectmadeinthiscode); are the only options to force the GC to not finalize the object (with said handle), before the native operating system calls finish with it and possibly cause undefined behavior on user's applications (and possibly the runtime itself as well).

This is why one or the other should be done, either tell the user to tell the GC to keep that object alive. or use a HandeRef that would need to be generated by CsWin32.

Another option for this, is to have it not generate HandleRef overloads by default but have the user opt-in for them if they specifically ask for said overload to be generated within the configuration for specific members only (opt-in per p/invoke function name) within the text file that controls the source generator.

There was more discussion about this in #125.

AArnott commented 3 years ago

Use within the runtime itself is not an argument that applies to this repo as the runtime repo is not our target audience. Use by the public API that user applications must interact with is a potential reason. Do those exist?

We need a good reason to generate what was deprecated so long ago. Can you explain a scenario where HandleRef does the job better than SafeHandle?

jnm2 commented 3 years ago

@AArnott Again, this is for scenarios where you do not have control of the code that created the handle, such as System.Windows.Forms.Form. HandleRef keeps you from forgetting to do GC.KeepAlive(form); after the native call that uses the form's handle. SafeHandle is never going to help with safety here because it doesn't prevent the object from being concurrently collected and it doesn't prevent the existing Windows Forms library from freeing the handle when the object is collected.

AraHaan commented 3 years ago

oh and speaking of which it's also for when you need to use the underlying os apis to do extra work that the Windows Forms library does not do (like calling undocumented apis to make every control and the window itself dark instead of light like how Windows Explorer does).

I am however tempted to change that eventually by making an fork then apply the logic from here to make everything even the scrollbars dark which would be epic to do then file an issue for it to be approved first then if approved it being pull requested in to winforms (and possible wpf as well) that way it can abstract away the need to make the stuff dark when dark mode on windows is enabled (everything then every single context menu strip, etc would be made dark then).

Plus I even know how to darkify the system menu strip too on the forms (however needs handles after first converting the original ones to a ContextMenuStrip to the winforms library with an dark mode renderer that renders as dark only when dark is set) and then set that strip as the system menu strip.

So yes, it's useful for things like System.Windows.Forms.Form or any windows forms control (including ContextMenuStrips).

However I do not know if the Windows Forms or WPF teams would welcome the use of undocumented windows apis being used inside of those libraries despite it now being easy to be able to do.

AArnott commented 3 years ago

I do not know if the Windows Forms or WPF teams would welcome the use of undocumented windows apis being used inside of those libraries despite it now being easy to be able to do.

I'm sure they wouldn't, on legal grounds. Microsoft products outside Windows cannot call into undocumented Windows APIs.

SafeHandle is never going to help with safety here because it doesn't prevent the object from being concurrently collected

Why do you say that? Who is going to finalize a SafeHandle while it's in a p/invoke call? That's what its addref/releaseref methods are for. SafeHandle should never close the native handle while it's in use. Even if you explicitly Dispose of it, I think it waits till the last ReleaseRef call before closing the native handle.

HandleRef keeps you from forgetting to do GC.KeepAlive(form);

Well, form is not a native handle, and we would not generate a HandleRef or a SafeHandle for a Form class so I'm not sure how that's relevant.

for scenarios where you do not have control of the code that created the handle, such as System.Windows.Forms.Form.

Form doesn't derive from HandleRef, so I'm again not sure what you're talking about.

If WinForms has public APIs that produce HandleRef for you to use and you want to use that in CsWin32-generated APIs, then that's a more solid case. If those APIs exist, can you produce examples?

jnm2 commented 3 years ago

@AArnott I must have not been communicating clearly. I'm not sure how to reply to most of your responses.

Here's the situation that HandleRef is useful for, and there's simply no way that I can ever use SafeHandle to help prevent System.Windows.Forms.Form from being finalized and freeing its handle. Keep in mind that Form.Handle returns IntPtr and Form contains a finalizer that frees that handle.

void DoSomething(Form form)
{
    // Danger! The GC might finalize 'form' during this call, freeing the handle.
    User32.EnumChildWindows(form.Handle, ..., ...);
}

Seeing HandleRef and not being able to pass just form.Handle pressures you towards success by not forgetting to keep 'form' alive.

void DoSomething(Form form)
{
    User32.EnumChildWindows(new HandleRef(form, form.Handle), ..., ...);
}

This is also safe, but I hardly ever remember to add the GC.KeepAlive call. I think this is what @AraHaan is asking when they say either make it a HandleRef overload so you can't forget about this problem, or add in the XML docs that you should add GC.KeepAlive if the handle is owned by a managed object.

void DoSomething(Form form)
{
    User32.EnumChildWindows(form.Handle, ..., ...);
    GC.KeepAlive(form);
}

SafeHandle makes no sense here because the Form object that you may or may not own has its own finalizer which frees its own IntPtr handle, which you decidedly don't own.

void DoSomething(Form form)
{
    // Nothing is stopping the GC from concurrently finalizing 'form' and freeing the handle during this call
    User32.EnumChildWindows(new SomeKindOfSafeHandle(form.Handle), ..., ...);
}
jnm2 commented 3 years ago

You could say "the problem is clearly that Windows Forms and similar libraries should be using SafeHandle," but that doesn't solve the question of what to do with today's .NET Core 3.0–.NET 6's Windows Forms, or what to do with .NET Framework's Windows Forms which will probably never deprecate its IntPtr handle property even if .NET 10 does. The problem is so systemic that IWin32Window has IntPtr Handle { get; }. I almost can't envision this being changed as of .NET 10.

AArnott commented 3 years ago

I must have not been communicating clearly. I'm not sure how to reply to most of your responses.

That may be my fault. I've never use HandleRef before so I am stumbling in the dark a bit.

And thank you for your last couple comments. I feel like I understand the scenario much better now.

either make it a HandleRef overload so you can't forget about this problem, or add in the XML docs that you should add GC.KeepAlive if the handle is owned by a managed object

Either of these options sound reasonable. Given this seems to be a problem specifically with WinForms, if we offer HandleRef overloads I think it will only be under a non-default option since it appeals only to a subset of the crowd. Adding the xml doc comment seems reasonable too, although since we already use the docs from docs.microsoft.com I'm not sure where we would add our own docs. At the bottom of the param doc, after the "this has been truncated" notice?

jnm2 commented 3 years ago

Okay, cool! I'm glad that helped. I think making HandleRef opt-in is very reasonable since it's only there to cope with non-SafeHandle finalizers, and Microsoft docs were finally updated last year to start recommending SafeHandle instead of freeing handles directly in handwritten finalizers.

At the bottom of the docs seems fine. This is almost better suited to be reported as a warning by the source generator than to be in docs though.

AArnott commented 3 years ago

reported as a warning by the source generator than to be in docs though.

That's an interesting idea. Basically anytime an object.Member is passed in as an argument to a method that takes a HANDLE-style typedef, emit a warning if a GC.KeepAlive(object) does not appear sometime after that call in the method. Is that what you were thinking?

jnm2 commented 3 years ago

Yes. Now that I'm thinking about it, you'd probably need to expose an analyzer because the source generator probably can't itself be the one to register for IInvocationOperation/IArgumentOperations etc to check and provide warnings for all call sites. (Or even if it could, you wouldn't want to delay the source generator by eagerly demanding parsing and semantics to be fully generated for the whole project. Though maybe they are already available before any source is generated?)

jnm2 commented 3 years ago

Oh, also, separate analyzer would be analyzing a semantic model of the compilation that already has the generated source, which probably makes everything easier than dealing with missing types and members.

KalleOlaviNiemitalo commented 4 months ago

Keep in mind that Form.Handle returns IntPtr and Form contains a finalizer that frees that handle.

I don't see such a finalizer. There is Form.Dispose(bool), which indirectly calls Control.Dispose(bool), but that calls DestroyHandle only if the disposing parameter is true.

Open forms cannot be finalized anyway, as they are rooted from the System.Windows.Forms.Application.OpenForms collection.