Convert more interop to use function pointers (#43514)

Author: jkotas

src/libraries/Common/src/Interop/Windows/IpHlpApi/Interop.NetworkInformation.cs

@@ -508,8 +508,6 @@ internal unsafe struct MibUdp6RowOwnerPid
             internal ReadOnlySpan<byte> localAddrAsSpan => MemoryMarshal.CreateSpan(ref localAddr[0], 16);
         }
 
-        internal delegate void StableUnicastIpAddressTableDelegate(IntPtr context, IntPtr table);
-
         [DllImport(Interop.Libraries.IpHlpApi)]
         internal static extern uint GetAdaptersAddresses(
             AddressFamily family,
@@ -563,10 +561,10 @@ internal static extern uint GetExtendedUdpTable(IntPtr pUdpTable, ref uint dwOut
         internal static extern uint CancelMibChangeNotify2(IntPtr notificationHandle);
 
         [DllImport(Interop.Libraries.IpHlpApi)]
-        internal static extern uint NotifyStableUnicastIpAddressTable(
+        internal static extern unsafe uint NotifyStableUnicastIpAddressTable(
             AddressFamily addressFamily,
             out SafeFreeMibTable table,
-            StableUnicastIpAddressTableDelegate callback,
+            delegate* unmanaged<IntPtr, IntPtr, void> callback,
             IntPtr context,
             out SafeCancelMibChangeNotify notificationHandle);
     }

src/libraries/Common/src/Interop/Windows/User32/Interop.EnumWindows.cs

@@ -8,9 +8,7 @@ internal partial class Interop
 {
     internal partial class User32
     {
-        internal delegate bool EnumThreadWindowsCallback(IntPtr hWnd, IntPtr lParam);
-
         [DllImport(Libraries.User32)]
-        public static extern bool EnumWindows(EnumThreadWindowsCallback callback, IntPtr extraData);
+        public static extern unsafe bool EnumWindows(delegate* unmanaged<IntPtr, IntPtr, Interop.BOOL> callback, IntPtr extraData);
     }
 }

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/ProcessManager.Win32.cs

@@ -18,48 +18,47 @@ internal static partial class ProcessManager
     {
         public static IntPtr GetMainWindowHandle(int processId)
         {
-            return new MainWindowFinder().FindMainWindow(processId);
+            return MainWindowFinder.FindMainWindow(processId);
         }
     }
 
-    internal sealed class MainWindowFinder
+    internal struct MainWindowFinder
     {
         private const int GW_OWNER = 4;
         private IntPtr _bestHandle;
         private int _processId;
 
-        public IntPtr FindMainWindow(int processId)
+        public static unsafe IntPtr FindMainWindow(int processId)
         {
-            _bestHandle = IntPtr.Zero;
-            _processId = processId;
+            MainWindowFinder instance;
 
-            Interop.User32.EnumWindows(EnumWindowsCallback, IntPtr.Zero);
+            instance._bestHandle = IntPtr.Zero;
+            instance._processId = processId;
 
-            return _bestHandle;
+            Interop.User32.EnumWindows(&EnumWindowsCallback, (IntPtr)(void*)&instance);
+
+            return instance._bestHandle;
         }
 
-        private bool IsMainWindow(IntPtr handle)
+        private static bool IsMainWindow(IntPtr handle)
         {
-            if (Interop.User32.GetWindow(handle, GW_OWNER) != IntPtr.Zero || !Interop.User32.IsWindowVisible(handle))
-                return false;
-
-            return true;
+            return (Interop.User32.GetWindow(handle, GW_OWNER) == IntPtr.Zero) && Interop.User32.IsWindowVisible(handle);
         }
 
-        private bool EnumWindowsCallback(IntPtr handle, IntPtr extraParameter)
+        [UnmanagedCallersOnly]
+        private static unsafe Interop.BOOL EnumWindowsCallback(IntPtr handle, IntPtr extraParameter)
         {
-            int processId;
+            MainWindowFinder* instance = (MainWindowFinder*)extraParameter;
+
+            int processId = 0; // Avoid uninitialized variable if the window got closed in the meantime
             Interop.User32.GetWindowThreadProcessId(handle, out processId);
 
-            if (processId == _processId)
+            if ((processId == instance->_processId) && IsMainWindow(handle))
             {
-                if (IsMainWindow(handle))
-                {
-                    _bestHandle = handle;
-                    return false;
-                }
+                instance->_bestHandle = handle;
+                return Interop.BOOL.FALSE;
             }
-            return true;
+            return Interop.BOOL.TRUE;
         }
     }
 

src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/TeredoHelper.cs

@@ -4,6 +4,7 @@
 using System.Collections.Generic;
 using System.ComponentModel;
 using System.Diagnostics;
+using System.Runtime.InteropServices;
 using System.Net.Sockets;
 using System.Threading;
 
@@ -22,22 +23,15 @@ namespace System.Net.NetworkInformation
     // CancelMibChangeNotify2 guarantees that, by the time it returns, all calls to the callback will be complete
     // and that no new calls to the callback will be issued.
     //
-    // The major concerns of the class are: 1) making sure none of the managed objects needed to handle a native
-    // callback are GC'd before the callback, and 2) making sure no native callbacks will try to call into an unloaded
-    // AppDomain.
 
     internal class TeredoHelper
     {
-        // Holds a list of all pending calls to NotifyStableUnicastIpAddressTable.  Also used as a lock to protect its
-        // contents.
-        private static readonly List<TeredoHelper> s_pendingNotifications = new List<TeredoHelper>();
         private readonly Action<object> _callback;
         private readonly object _state;
 
         private bool _runCallbackCalled;
 
-        // We explicitly keep a copy of this to prevent it from getting GC'd.
-        private readonly Interop.IpHlpApi.StableUnicastIpAddressTableDelegate _onStabilizedDelegate;
+        private GCHandle _gcHandle;
 
         // Used to cancel notification after receiving the first callback, or when the AppDomain is going down.
         private SafeCancelMibChangeNotify? _cancelHandle;
@@ -46,123 +40,85 @@ private TeredoHelper(Action<object> callback, object state)
         {
             _callback = callback;
             _state = state;
-            _onStabilizedDelegate = new Interop.IpHlpApi.StableUnicastIpAddressTableDelegate(OnStabilized);
-            _runCallbackCalled = false;
+
+            _gcHandle = GCHandle.Alloc(this);
         }
 
         // Returns true if the address table is already stable.  Otherwise, calls callback when it becomes stable.
         // 'Unsafe' because it does not flow ExecutionContext to the callback.
-        public static bool UnsafeNotifyStableUnicastIpAddressTable(Action<object> callback, object state)
+        public static unsafe bool UnsafeNotifyStableUnicastIpAddressTable(Action<object> callback, object state)
         {
-            if (callback == null)
-            {
-                NetEventSource.Fail(null, "UnsafeNotifyStableUnicastIpAddressTable called without specifying callback!");
-            }
+            Debug.Assert(callback != null);
 
-            TeredoHelper helper = new TeredoHelper(callback, state);
+            TeredoHelper? helper = new TeredoHelper(callback, state);
+            try
+            {
+                uint err = Interop.IpHlpApi.NotifyStableUnicastIpAddressTable(AddressFamily.Unspecified,
+                    out SafeFreeMibTable table, &OnStabilized, GCHandle.ToIntPtr(helper._gcHandle), out helper._cancelHandle);
 
-            uint err = Interop.IpHlpApi.ERROR_SUCCESS;
-            SafeFreeMibTable table;
+                table.Dispose();
 
-            lock (s_pendingNotifications)
-            {
-                // If OnAppDomainUnload gets the lock first, tell our caller that we'll finish async.  Their AppDomain
-                // is about to go down anyways.  If we do, hold the lock until we've added helper to the
-                // s_pendingNotifications list (if we're going to complete asynchronously).
-                if (Environment.HasShutdownStarted)
+                if (err == Interop.IpHlpApi.ERROR_IO_PENDING)
                 {
+                    Debug.Assert(helper._cancelHandle != null && !helper._cancelHandle.IsInvalid);
+
+                    // Suppress synchronous Dispose. Dispose will be called asynchronously by the callback.
+                    helper = null;
                     return false;
                 }
 
-                err = Interop.IpHlpApi.NotifyStableUnicastIpAddressTable(AddressFamily.Unspecified,
-                    out table, helper._onStabilizedDelegate, IntPtr.Zero, out helper._cancelHandle);
-
-                if (table != null)
+                if (err != Interop.IpHlpApi.ERROR_SUCCESS)
                 {
-                    table.Dispose();
+                    throw new Win32Exception((int)err);
                 }
 
-                if (err == Interop.IpHlpApi.ERROR_IO_PENDING)
-                {
-                    if (helper._cancelHandle.IsInvalid)
-                    {
-                        NetEventSource.Fail(null, "CancelHandle invalid despite returning ERROR_IO_PENDING");
-                    }
-
-                    // Async completion: add us to the s_pendingNotifications list so we'll be canceled in the
-                    // event of an AppDomain unload.
-                    s_pendingNotifications.Add(helper);
-                    return false;
-                }
+                return true;
             }
-
-            if (err != Interop.IpHlpApi.ERROR_SUCCESS)
+            finally
             {
-                throw new Win32Exception((int)err);
+                helper?.Dispose();
             }
-
-            return true;
         }
 
-        private void RunCallback(object? o)
+        private void Dispose()
         {
-            if (!_runCallbackCalled)
-            {
-                NetEventSource.Fail(null, "RunCallback called without setting runCallbackCalled!");
-            }
-
-            // If OnAppDomainUnload beats us to the lock, do nothing: the AppDomain is going down soon anyways.
-            // Otherwise, wait until the call to CancelMibChangeNotify2 is done before giving it up.
-            lock (s_pendingNotifications)
-            {
-                if (Environment.HasShutdownStarted)
-                {
-                    return;
-                }
+            _cancelHandle?.Dispose();
 
-#if DEBUG
-                bool successfullyRemoved = s_pendingNotifications.Remove(this);
-                if (!successfullyRemoved)
-                {
-                    NetEventSource.Fail(null, "RunCallback for a TeredoHelper which is not in s_pendingNotifications!");
-                }
-#else
-                s_pendingNotifications.Remove(this);
-#endif
-
-                if ((_cancelHandle == null || _cancelHandle.IsInvalid))
-                {
-                    NetEventSource.Fail(null, "Invalid cancelHandle in RunCallback");
-                }
-
-                _cancelHandle.Dispose();
-            }
-
-            _callback.Invoke(_state);
+            if (_gcHandle.IsAllocated)
+                _gcHandle.Free();
         }
 
         // This callback gets run on a native worker thread, which we don't want to allow arbitrary user code to
         // execute on (it will block AppDomain unload, for one).  Free the MibTable and delegate (exactly once)
         // to the managed ThreadPool for the rest of the processing.
-        //
-        // We can't use SafeHandle here for table because the marshaller doesn't support them in reverse p/invokes.
-        // We won't get an AppDomain unload here anyways, because OnAppDomainUnloaded will block until all of these
-        // callbacks are done.
-        private void OnStabilized(IntPtr context, IntPtr table)
+        [UnmanagedCallersOnly]
+        private static void OnStabilized(IntPtr context, IntPtr table)
         {
             Interop.IpHlpApi.FreeMibTable(table);
 
+            TeredoHelper helper = (TeredoHelper)GCHandle.FromIntPtr(context).Target!;
+
             // Lock the TeredoHelper instance to ensure that only the first call to OnStabilized will get to call
             // RunCallback.  This is the only place that TeredoHelpers get locked, as individual instances are not
             // exposed to higher layers, so there's no chance for deadlock.
-            if (!_runCallbackCalled)
+            if (!helper._runCallbackCalled)
             {
-                lock (this)
+                lock (helper)
                 {
-                    if (!_runCallbackCalled)
+                    if (!helper._runCallbackCalled)
                     {
-                        _runCallbackCalled = true;
-                        ThreadPool.QueueUserWorkItem(RunCallback, null);
+                        helper._runCallbackCalled = true;
+
+                        ThreadPool.QueueUserWorkItem(o =>
+                        {
+                            TeredoHelper helper = (TeredoHelper)o!;
+
+                            // We are intentionally not calling Dispose synchronously inside the OnStabilized callback.
+                            // According to MSDN, calling CancelMibChangeNotify2 inside the callback results into deadlock.
+                            helper.Dispose();
+
+                            helper._callback.Invoke(helper._state);
+                        }, helper);
                     }
                 }
             }