SafeHandle marshalling improvements in the source-generated marshalling

[jkoritzinsky]

From #69608:

4. SafeHandle disposal. When marshaling out a SafeHandle, the generator emits code like this:

            __retVal = new global::Microsoft.Win32.SafeHandles.SafeFindHandle();
            try
            {
                //
                // Pin
                //
                fixed (void* __lpFileName_gen_native__pinned = lpFileName)
                {
                    ushort* __lpFileName_gen_native = (ushort*)__lpFileName_gen_native__pinned;
                    fixed (global::Interop.Kernel32.WIN32_FIND_DATA* __lpFindFileData_gen_native = &lpFindFileData)
                    {
                        {
                            System.Runtime.InteropServices.Marshal.SetLastSystemError(0);
                            __retVal_gen_native = __PInvoke__(__lpFileName_gen_native, fInfoLevelId, __lpFindFileData_gen_native, fSearchOp, lpSearchFilter, dwAdditionalFlags);
                            __lastError = System.Runtime.InteropServices.Marshal.GetLastSystemError();
                        }
                    }
                }

                __invokeSucceeded = true;
            }
            finally
            {
                if (__invokeSucceeded)
                {
                    //
                    // GuaranteedUnmarshal
                    //
                    System.Runtime.InteropServices.Marshal.InitHandle(__retVal, __retVal_gen_native);
                }
            }

It's using a try/finally and __invokeSucceeded, so it's trying to accomodate something in that try region throwing. But if something there does throw, the allocated SafeHandle won't be disposed, which will in turn leave it for finalization. Should there be a catch block?

catch
{
    __retVal.Dispose();
    throw;
}

or potentially instead a catch with an exception filter that does the disposal and returns false to avoid interrupting the unwind?

In .NET 8, we'll look at moving the SafeHandle marshalling into a custom marshaller type. When we do so, we'll address this feedback.

Note for future self: This would be a good use case for the OnInvoked custom marshaller method that isn't a GC.KeepAlive usage.

[ghost]

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

---

From #69608:

4. SafeHandle disposal. When marshaling out a SafeHandle, the generator emits code like this:

            __retVal = new global::Microsoft.Win32.SafeHandles.SafeFindHandle();
            try
            {
                //
                // Pin
                //
                fixed (void* __lpFileName_gen_native__pinned = lpFileName)
                {
                    ushort* __lpFileName_gen_native = (ushort*)__lpFileName_gen_native__pinned;
                    fixed (global::Interop.Kernel32.WIN32_FIND_DATA* __lpFindFileData_gen_native = &lpFindFileData)
                    {
                        {
                            System.Runtime.InteropServices.Marshal.SetLastSystemError(0);
                            __retVal_gen_native = __PInvoke__(__lpFileName_gen_native, fInfoLevelId, __lpFindFileData_gen_native, fSearchOp, lpSearchFilter, dwAdditionalFlags);
                            __lastError = System.Runtime.InteropServices.Marshal.GetLastSystemError();
                        }
                    }
                }

                __invokeSucceeded = true;
            }
            finally
            {
                if (__invokeSucceeded)
                {
                    //
                    // GuaranteedUnmarshal
                    //
                    System.Runtime.InteropServices.Marshal.InitHandle(__retVal, __retVal_gen_native);
                }
            }

It's using a try/finally and __invokeSucceeded, so it's trying to accomodate something in that try region throwing. But if something there does throw, the allocated SafeHandle won't be disposed, which will in turn leave it for finalization. Should there be a catch block?

catch
{
    __retVal.Dispose();
    throw;
}

or potentially instead a catch with an exception filter that does the disposal and returns false to avoid interrupting the unwind?

In .NET 8, we'll look at moving the SafeHandle marshalling into a custom marshaller type. When we do so, we'll address this feedback.

Note for future self: This would be a good use case for the OnInvoked custom marshaller method that isn't a GC.KeepAlive usage.

Author:	jkoritzinsky
Assignees:	-
Labels:	area-System.Runtime.InteropServices
Milestone:	8.0.0

[AaronRobinsonMSFT]

/cc @stephentoub

[jkoritzinsky]

We completed this work with #85419