HandleRef overloads for all handles

[jnm2]

This project is organized with freaky accuracy to how I've been doing it for years and I was very glad to find this much more complete library.

The only stumbling block is that methods like IsWindow take IntPtr when they should be taking either a HandleRef or a WindowSafeHandle. The danger of background GC freeing handles on a background thread concurrently with the native call's internal execution is severe enough that even with HandleRef overloads, I consider the presence of the IntPtr overloads a danger. Someone will use form.Handle without thinking about it. The verbosity of writing new HandleRef(form, form.Handle) and new HandleRef(null, handleOfUnmanagedObject) is well worth it given the difficulty of tracking down the heisenbug that is created when you don't guard against GC freeing.

Since the major version is still zero, how open would you be to a breaking change?

[AArnott]

Thanks for your feedback. Yes, we're still taking breaking changes when there is sufficient justification. Your case may qualify.

Can you help me understand the danger a bit more? In general our policy is that we use SafeHandle-derived types when the process owns the handle, and IntPtr when it doesn't, since the process will never own the lifetime and thus a SafeHandle's disposal semantics don't matter.

[jnm2]

Awesome! For example, when the handle is managed by a System.Windows.Forms.Control.

var form = new System.Windows.Forms.Form();
User32.GetDC(form.Handle);

If form is not mentioned below the GetDC statement, a background thread may cause a GC which finalizes the form, freeing the handle while GetDC is still in native code. The evaluation stack in IL is the same as if we had done this:

var form = new System.Windows.Forms.Form();
var tmp = form.Handle;
// No more references to `form` after this point, so it is available for garbage collection
// As soon as Form.get_Handle returns, passing the handle to native code is unsafe.
User32.GetDC(tmp);

The workaround is to put GC.KeepAlive(form); after the last native call, but there's nothing to remind you to do that. HandleRef works because the pinvoke marshaler creates its own GC.KeepAlive on handleRef.Wrapper until native code has returned, and because it's hard to overlook.

[jnm2]

This applies any time there is the potential for a managed object to free a handle which it exposes as an IntPtr rather than a SafeHandle, as Windows Forms does.

The same thing applies to System.Drawing.Icon.Handle. Any native method which takes an hIcon should provide a HandleRef overload in case someone is using the BCL Icon class.

Things I see right off on http://referencesource.microsoft.com:

System.Drawing.Icon.Handle

System.Windows.Forms.Control.Handle System.Windows.Forms.Cursor.Handle System.Windows.Forms.ImageList.Handle System.Windows.Forms.InputLanguage.Handle System.Windows.Forms.IWin32Window.Handle (not uncommon to pass this interface around) System.Windows.Forms.Menu.Handle System.Windows.Forms.NativeWindow.Handle System.Windows.Forms.StatusBar.ControlToolTip.Handle System.Windows.Forms.ToolTip.Handle System.Windows.Forms.TreeNode.Handle System.Windows.Forms.VisualStyleRenderer.Handle

System.ServiceModel.Activation.NamedPipeDuplicateContext.Handle

System.DirectoryServices.SearchResultCollection.Handle

System.Windows.Interop.IWin32Window.Handle System.Windows.Interop.HwndSource.Handle System.Windows.Interop.HwndHost.Handle System.Windows.Interop.WindowInteropHelper.Handle

[jnm2]

There's also the potential for there to be a third-party library that provides a completely separate object model which messes up and exposes IntPtrs instead of SafeHandles, over native handle types not represented in the list of native handle types which the BCL exposes as bare IntPtrs.

How important these all are I'm not sure. How likely is it that people will access the Handle properties in order to use your library? If you want to limit the scope, I would say HWND and HICON for sure. Beyond that I don't know if the other handles are used that often. What's the harm, though?

[jnm2]

Another thought. If you remove the IntPtr overloads, it becomes painful to use user32 APIs for users solely using your libraries and not Windows Forms. That's where SafeWindowHandle overloads in addition to the HandleRef would be nice when replacing IntPtr overloads.

CreateWindow etc should be returning a SafeHandle anyway because you're guaranteed to be responsible for freeing it. That's a separate problem which would be addressed by a change like this.

[vbfox]

I understand that HandleRef can be practical when dealing with old APIs like winforms that don't uses/exposes SafeHandles but I'm not sure that we should add it everywhere, it has only been added back to netstandard in 2.0 for compatibility reasons and is marked as deprecated in https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.handleref?view=netstandard-2.0 :

Starting with the .NET Framework 2.0, the functionality of the HandleRef class has been replaced by the SafeHandle class and its derived classes, as well as by the CriticalHandle class.

(If we are missing safe handles that's another question, we should have them)

[jnm2]

Deprecated might be a bit strong. There's been no plan to obsolete it. That's a message the Windows Forms API designers can act on, not p/invoke writers who are forced to interop with Windows Forms.

I could live with the decision to not support interop with Windows Forms though I don't think that's optimal. Perhaps I could develop an analyzer that moves GC.KeepAlive to after any pinvoke call that uses an IntPtr .Handle.