Skip to content
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

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

Open
AraHaan opened this issue Mar 17, 2021 · 12 comments
Labels
discussion needed We need more feedback or responses from ABI experts

Comments

@AraHaan
Copy link
Contributor

AraHaan commented Mar 17, 2021

Currently in CsWin32 no overloads taking in HandleRefs are generated.

The use cases:

  • HandleRef is not only used by user applications (for referencing HWND handles to (most commonly) gui controls to pass them into p/invoke methods to the operating system apis).
  • They are also used in the runtime itself (they are used a lot in the dotnet organization on github).
  • user made libraries may also choose to use HandeRef for p/invokes to handles they do not own themselves to keep the GC from destroying them in mid call to the p/invoke.

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.

@AraHaan AraHaan changed the title reconsider taking HandeRef as input parameters when the parameter must be a handle of some sort. Reconsider taking HandeRef as input parameters when the parameter must be a handle of some sort. Mar 17, 2021
@AArnott
Copy link
Member

AArnott commented Mar 17, 2021

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?

@AArnott AArnott added the discussion needed We need more feedback or responses from ABI experts label Mar 17, 2021
@jnm2
Copy link
Contributor

jnm2 commented Mar 17, 2021

@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
Copy link
Contributor Author

AraHaan commented Mar 17, 2021

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
Copy link
Member

AArnott commented Mar 17, 2021

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
Copy link
Contributor

jnm2 commented Mar 17, 2021

@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
Copy link
Contributor

jnm2 commented Mar 17, 2021

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
Copy link
Member

AArnott commented Mar 17, 2021

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
Copy link
Contributor

jnm2 commented Mar 17, 2021

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
Copy link
Member

AArnott commented Mar 17, 2021

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
Copy link
Contributor

jnm2 commented Mar 17, 2021

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
Copy link
Contributor

jnm2 commented Mar 17, 2021

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.

AArnott added a commit that referenced this issue Jun 12, 2023
* Crank up dependabot

* Bump Microsoft.NET.Test.Sdk from 17.5.0 to 17.6.0

Bumps [Microsoft.NET.Test.Sdk](https://github.com/microsoft/vstest) from 17.5.0 to 17.6.0.
- [Release notes](https://github.com/microsoft/vstest/releases)
- [Changelog](https://github.com/microsoft/vstest/blob/main/docs/releases.md)
- [Commits](microsoft/vstest@v17.5.0...v17.6.0)

---
updated-dependencies:
- dependency-name: Microsoft.NET.Test.Sdk
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Andrew Arnott <andrewarnott@live.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@KalleOlaviNiemitalo
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed We need more feedback or responses from ABI experts
Projects
None yet
Development

No branches or pull requests

4 participants