[release/6.0] Fix race conditions in SystemEvents shutdown logic (#66230)

* Fix race conditions in SystemEvents shutdown logic (#62773)

* Fix race conditions in SystemEvents shutdown logic

When the application is terminated through Restart Manager the event broadcasting window will get the `WM_CLOSE` message. The message gets handled by passing it to `DefWndProc` which calls `DestroyWindow` on the window itself thus making the window handle invalid. The `Shutdown` method expects the window handle to be valid to post `WM_QUIT` message to terminate the thread running the message loop but that's no longer possible under these conditions.

Additionally there's second race condition with the `s_eventThreadTerminated` event that is created during shutdown and set conditionally. A race condition between the threads could cause it to be created when the window message thread is already shutting down and thus it would never be set. Waiting for it in the `Shutdown` method would be cause a deadlock. This thread is also completely unnecessary since a `Join` is performed on the thread itself.

The fix has several changes that act together:
- `s_eventThreadTerminated` event is removed completely in favor of only relying on `Thread.Join`
- `WM_DESTROY` message is detected (which happens as a result of WM_CLOSE calling `DefWndProc` which in turn calls `DestroyWindow`) and handled by shutting down the message loop thread
- The message loop itself is rewritten to use standard `GetMessageW` loop. The reasoning on why it was not used seems not to be valid anymore since AppDomain shutdowns are performed differently

* Add unit test.

* Add braces

* Add package authoring for servicing

Co-authored-by: Filip Navara <filip.navara@gmail.com>

7 files changed, 75 insertions, 40 deletions

diff --git a/src/libraries/Common/src/Interop/Windows/User32/Interop.Constants.cs b/src/libraries/Common/src/Interop/Windows/User32/Interop.Constants.cs
index 7a1574155a7..e1a9bc4cce2 100644
--- a/src/libraries/Common/src/Interop/Windows/User32/Interop.Constants.cs
+++ b/src/libraries/Common/src/Interop/Windows/User32/Interop.Constants.cs
@@ -198,6 +198,7 @@ internal static partial class Interop
 
         public const int WAIT_TIMEOUT = 0x00000102;
 
+        public const int WM_DESTROY = 0x0002;
         public const int WM_CLOSE = 0x0010;
         public const int WM_QUERYENDSESSION = 0x0011;
         public const int WM_QUIT = 0x0012;
diff --git a/src/libraries/Common/src/Interop/Windows/User32/Interop.PeekMessage.cs b/src/libraries/Common/src/Interop/Windows/User32/Interop.GetMessage.cs
index 45710968dba..3f4a12dfa66 100644
--- a/src/libraries/Common/src/Interop/Windows/User32/Interop.PeekMessage.cs
+++ b/src/libraries/Common/src/Interop/Windows/User32/Interop.GetMessage.cs
@@ -9,6 +9,6 @@ internal static partial class Interop
     internal static partial class User32
     {
         [DllImport(Libraries.User32, CharSet = CharSet.Unicode, ExactSpelling = true)]
-        public static extern bool PeekMessageW([In, Out] ref MSG msg, IntPtr hwnd, int msgMin, int msgMax, int remove);
+        public static extern int GetMessageW(ref MSG msg, IntPtr hwnd, int msgMin, int msgMax);
     }
 }
diff --git a/src/libraries/Common/src/Interop/Windows/User32/Interop.PostQuitMessage.cs b/src/libraries/Common/src/Interop/Windows/User32/Interop.PostQuitMessage.cs
new file mode 100644
index 00000000000..b7595f9dcfb
--- /dev/null
+++ b/src/libraries/Common/src/Interop/Windows/User32/Interop.PostQuitMessage.cs
@@ -0,0 +1,14 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Runtime.InteropServices;
+
+internal static partial class Interop
+{
+    internal static partial class User32
+    {
+        [DllImport(Libraries.User32, CharSet = CharSet.Unicode, ExactSpelling = true)]
+        public static extern void PostQuitMessage(int exitCode);
+    }
+}
diff --git a/src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft.Win32.SystemEvents.csproj b/src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft.Win32.SystemEvents.csproj
index 068e9ed7954..f529ae32889 100644
--- a/src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft.Win32.SystemEvents.csproj
+++ b/src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft.Win32.SystemEvents.csproj
@@ -5,6 +5,8 @@
 
 Commonly Used Types:
 Microsoft.Win32.SystemEvents</PackageDescription>
+    <GeneratePackageOnBuild>true</GeneratePackageOnBuild>
+    <ServicingVersion>1</ServicingVersion>
     <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
     <Nullable>enable</Nullable>
     <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent);netcoreapp3.1-windows;netcoreapp3.1;netstandard2.0;net461</TargetFrameworks>
@@ -35,6 +37,8 @@ Microsoft.Win32.SystemEvents</PackageDescription>
              Link="Common\Interop\Windows\User32\Interop.DispatchMessage.cs" />
     <Compile Include="$(CommonPath)Interop\Windows\User32\Interop.GetClassInfo.cs"
              Link="Common\Interop\Windows\User32\Interop.GetClassInfo.cs" />
+    <Compile Include="$(CommonPath)Interop\Windows\User32\Interop.GetMessage.cs"
+             Link="Common\Interop\Windows\User32\Interop.GetMessage.cs" />
     <Compile Include="$(CommonPath)Interop\Windows\User32\Interop.GetProcessWindowStation.cs"
              Link="Common\Interop\Windows\User32\Interop.GetProcessWindowStation.cs" />
     <Compile Include="$(CommonPath)Interop\Windows\User32\Interop.GetUserObjectInformation.cs"
@@ -49,10 +53,10 @@ Microsoft.Win32.SystemEvents</PackageDescription>
              Link="Common\Interop\Windows\User32\Interop.MSG.cs" />
     <Compile Include="$(CommonPath)Interop\Windows\User32\Interop.MsgWaitForMultipleObjectsEx.cs"
              Link="Common\Interop\Windows\User32\Interop.MsgWaitForMultipleObjectsEx.cs" />
-    <Compile Include="$(CommonPath)Interop\Windows\User32\Interop.PeekMessage.cs"
-             Link="Common\Interop\Windows\User32\Interop.PeekMessage.cs" />
     <Compile Include="$(CommonPath)Interop\Windows\User32\Interop.PostMessage.cs"
              Link="Common\Interop\Windows\User32\Interop.PostMessage.cs" />
+    <Compile Include="$(CommonPath)Interop\Windows\User32\Interop.PostQuitMessage.cs"
+             Link="Common\Interop\Windows\User32\Interop.PostQuitMessage.cs" />
     <Compile Include="$(CommonPath)Interop\Windows\User32\Interop.RegisterClass.cs"
              Link="Common\Interop\Windows\User32\Interop.RegisterClass.cs" />
     <Compile Include="$(CommonPath)Interop\Windows\User32\Interop.RegisterWindowMessage.cs"
diff --git a/src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft/Win32/SystemEvents.cs b/src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft/Win32/SystemEvents.cs
index 2192c517f81..6e3fc69ef48 100644
--- a/src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft/Win32/SystemEvents.cs
+++ b/src/libraries/Microsoft.Win32.SystemEvents/src/Microsoft/Win32/SystemEvents.cs
@@ -35,7 +35,6 @@ namespace Microsoft.Win32
         // cross-thread marshaling
         private static volatile Queue<Delegate>? s_threadCallbackList; // list of Delegates
         private static volatile int s_threadCallbackMessage;
-        private static volatile ManualResetEvent? s_eventThreadTerminated;
 
         // Per-instance data that is isolated to the window thread.
         private volatile IntPtr _windowHandle;
@@ -1071,7 +1070,7 @@ namespace Microsoft.Win32
 
         private static void Shutdown()
         {
-            if (s_systemEvents != null && s_systemEvents._windowHandle != IntPtr.Zero)
+            if (s_systemEvents != null)
             {
                 lock (s_procLockObject)
                 {
@@ -1080,17 +1079,22 @@ namespace Microsoft.Win32
                         // If we are using system events from another thread, request that it terminate
                         if (s_windowThread != null)
                         {
-                            s_eventThreadTerminated = new ManualResetEvent(false);
-
 #if DEBUG
                             int pid;
                             int thread = Interop.User32.GetWindowThreadProcessId(new HandleRef(s_systemEvents, s_systemEvents._windowHandle), out pid);
                             Debug.Assert(thread != Interop.Kernel32.GetCurrentThreadId(), "Don't call Shutdown on the system events thread");
 #endif
-                            Interop.User32.PostMessageW(new HandleRef(s_systemEvents, s_systemEvents._windowHandle), Interop.User32.WM_QUIT, IntPtr.Zero, IntPtr.Zero);
 
-                            s_eventThreadTerminated.WaitOne();
-                            s_windowThread.Join(); // avoids an AppDomainUnloaded exception on our background thread.
+                            // The handle could be valid, Zero or invalid depending on the state of the thread
+                            // that is processing the messages. We optimistically expect it to be valid to
+                            // notify the thread to shutdown. The Zero or invalid values should be present
+                            // only when the thread is already shutting down due to external factors.
+                            if (s_systemEvents._windowHandle != IntPtr.Zero)
+                            {
+                                Interop.User32.PostMessageW(new HandleRef(s_systemEvents, s_systemEvents._windowHandle), Interop.User32.WM_QUIT, IntPtr.Zero, IntPtr.Zero);
+                            }
+
+                            s_windowThread.Join();
                         }
                         else
                         {
@@ -1218,6 +1222,11 @@ namespace Microsoft.Win32
                     OnTimerElapsed(wParam);
                     break;
 
+                case Interop.User32.WM_DESTROY:
+                    Interop.User32.PostQuitMessage(0);
+                    _windowHandle = IntPtr.Zero;
+                    break;
+
                 default:
                     // If we received a thread execute message, then execute it.
                     if (msg == s_threadCallbackMessage && msg != 0)
@@ -1248,33 +1257,10 @@ namespace Microsoft.Win32
                 {
                     Interop.User32.MSG msg = default(Interop.User32.MSG);
 
-                    bool keepRunning = true;
-
-                    // Blocking on a GetMessage() call prevents the EE from being able to unwind
-                    // this thread properly (e.g. during AppDomainUnload). So, we use PeekMessage()
-                    // and sleep so we always block in managed code instead.
-                    while (keepRunning)
+                    while (Interop.User32.GetMessageW(ref msg, _windowHandle, 0, 0) > 0)
                     {
-                        int ret = Interop.User32.MsgWaitForMultipleObjectsEx(0, IntPtr.Zero, 100, Interop.User32.QS_ALLINPUT, Interop.User32.MWMO_INPUTAVAILABLE);
-
-                        if (ret == Interop.User32.WAIT_TIMEOUT)
-                        {
-                            Thread.Sleep(1);
-                        }
-                        else
-                        {
-                            while (Interop.User32.PeekMessageW(ref msg, IntPtr.Zero, 0, 0, Interop.User32.PM_REMOVE))
-                            {
-                                if (msg.message == Interop.User32.WM_QUIT)
-                                {
-                                    keepRunning = false;
-                                    break;
-                                }
-
-                                Interop.User32.TranslateMessage(ref msg);
-                                Interop.User32.DispatchMessageW(ref msg);
-                            }
-                        }
+                        Interop.User32.TranslateMessage(ref msg);
+                        Interop.User32.DispatchMessageW(ref msg);
                     }
                 }
 
@@ -1293,10 +1279,6 @@ namespace Microsoft.Win32
             }
 
             Dispose();
-            if (s_eventThreadTerminated != null)
-            {
-                s_eventThreadTerminated.Set();
-            }
         }
 
         // A class that helps fire events on the right thread.
diff --git a/src/libraries/Microsoft.Win32.SystemEvents/tests/Microsoft.Win32.SystemEvents.Tests.csproj b/src/libraries/Microsoft.Win32.SystemEvents/tests/Microsoft.Win32.SystemEvents.Tests.csproj
index 83d774de464..1341ae4f701 100644
--- a/src/libraries/Microsoft.Win32.SystemEvents/tests/Microsoft.Win32.SystemEvents.Tests.csproj
+++ b/src/libraries/Microsoft.Win32.SystemEvents/tests/Microsoft.Win32.SystemEvents.Tests.csproj
@@ -23,6 +23,7 @@
     <Compile Include="SystemEvents.SessionSwitch.cs" />
     <Compile Include="SystemEvents.PowerMode.cs" />
     <Compile Include="SystemEvents.TimeChanged.cs" />
+    <Compile Include="ShutdownTest.cs" />
     <Compile Include="SystemEventsTest.cs" />
     <Compile Include="SystemEvents.DisplaySettings.cs" />
     <Compile Include="SystemEvents.CreateTimer.cs" />
diff --git a/src/libraries/Microsoft.Win32.SystemEvents/tests/ShutdownTest.cs b/src/libraries/Microsoft.Win32.SystemEvents/tests/ShutdownTest.cs
new file mode 100644
index 00000000000..d8012da78db
--- /dev/null
+++ b/src/libraries/Microsoft.Win32.SystemEvents/tests/ShutdownTest.cs
@@ -0,0 +1,33 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Threading;
+using Microsoft.DotNet.RemoteExecutor;
+using Xunit;
+using static Interop;
+
+namespace Microsoft.Win32.SystemEventsTests
+{
+    public abstract class ShutdownTest : SystemEventsTest
+    {
+        [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindowsNanoNorServerCore))]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
+        public void ShutdownThroughRestartManager()
+        {
+            RemoteExecutor.Invoke(() =>
+            {
+                // Register any event to ensure that SystemEvents get initialized
+                SystemEvents.TimeChanged += (o, e) => { };
+
+                // Fake Restart Manager behavior by sending external WM_CLOSE message
+                SendMessage(Interop.User32.WM_CLOSE, IntPtr.Zero, IntPtr.Zero);
+
+                // Emulate calling the Shutdown event
+                var shutdownMethod = typeof(SystemEvents).GetMethod("Shutdown", System.Reflection.BindingFlags.Static | System.Reflection.BindingFlags.NonPublic, null, new Type[0], null);
+                Assert.NotNull(shutdownMethod);
+                shutdownMethod.Invoke(null, null);
+            }).Dispose();
+        }
+    }
+}