From 86347e95a8a7b70bc6da1fde0538b2a9ec34a67f Mon Sep 17 00:00:00 2001 From: Vatsan Madhavan Date: Wed, 23 Oct 2019 15:37:09 -0700 Subject: [PATCH] When an `HwndHost `receives `SourceChanged `event, it goes through `BuildOrReparentWindow`. When the hosted window is invisible, it is usually reparented under a temporary windows maintained by WPF in the `SystemResources `class, until later on the window can be rebuilt and parented back to a valid parent. There is a latent bug in this logic where in `NULL ` `HWND's `are attempted to be parented to `SystemResources `managed temporary windows. This bug goes back quite a while (.NET 4.5 likely). WPF seems to ignore the return value from `kernel32!SetParent` and not deal with this failure. This has not been a crashing failure until now. Starting .NET 4.8, there have been some changes to this codepath that has resulted in the current bug becoming a crash. In addition to calling `kernel32!SetParent` on a `NULL` `HWND`, WPF attempts to obtain a DPI-specific parking-window. This process of querying a DPI-specific parking window fails because WPF is unable to use the `DPI_AWARENESS_CONTEXT` value returned by the system for `(HWND)nullptr`. The only necessary part of this fix is in `HwndHost`: WPF should not attempt to reparent the hosted window under a parking-window if the hosted window is `(HWND)nullptr`. This only requires a simple check : `else if (_hwnd.Handle != IntPtr.Zero)`). All other changes in `SystemResources` and `HwndHost` are defensive improvements. `SystemResources.EnsureResourceChangeListener(HwndDpiInfo)` can attempt to create a parking-window corresponding to `DPI_AWARENESS_CONTEXT_VALUE` that is invalid/meaningless. This should not be allowed. A few additional checks are added to ensure this. Further, `GetDpiAwarenessCompatibleNotificationWindow` is augmented to be more defensive. Also, variant of `EnsureResourceChangeListener` is dead code - it is being removed. If for some unknown reason `SystemResources.GetDpiAwarenessCompatibleNotificationWindow` fails and returns `null` to `HwndHost.BuildOrReparentWindow`, WPF will fail to reparent the hosted window, and it will be 'lost'. This seems very unlikely - I have added a Trace to ensure that we can debug this situation if it does occur. --- .../System/Windows/Interop/HwndHost.cs | 29 ++++++----- .../System/Windows/SystemResources.cs | 48 ++++++++----------- 2 files changed, 37 insertions(+), 40 deletions(-) diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Interop/HwndHost.cs b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Interop/HwndHost.cs index 15e744e420d..54c17063a61 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Interop/HwndHost.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Interop/HwndHost.cs @@ -976,23 +976,28 @@ private void BuildOrReparentWindow() UnsafeNativeMethods.SetParent(_hwnd, new HandleRef(null,hwndParent)); } } - else + else if (_hwnd.Handle != IntPtr.Zero) { // Reparent the window to notification-only window provided by SystemResources // This keeps the child window around, but it is not visible. We can reparent the // window later when a new parent is available var hwnd = SystemResources.GetDpiAwarenessCompatibleNotificationWindow(_hwnd); - UnsafeNativeMethods.SetParent(_hwnd, new HandleRef(null, hwnd.Handle)); - // ...But we have a potential problem: If the SystemResources listener window gets - // destroyed ahead of the call to HwndHost.OnDispatcherShutdown(), the HwndHost's window - // will be destroyed too, before the "logical" Dispose has had a chance to do proper - // shutdown. This turns out to be very significant for WebBrowser/ActiveXHost, which shuts - // down the hosted control through the COM interfaces, and the control destroys its - // window internally. Evidently, the WebOC fails to do full, proper cleanup if its - // window is destroyed unexpectedly. - // To avoid this situation, we make sure SystemResources responds to the Dispatcher - // shutdown event after this HwndHost. - SystemResources.DelayHwndShutdown(); + Debug.Assert(hwnd != null); + Trace.WriteLineIf(hwnd == null, $"- Warning - Notification Window is null\n{new System.Diagnostics.StackTrace(true).ToString()}"); + if (hwnd != null) + { + UnsafeNativeMethods.SetParent(_hwnd, new HandleRef(null, hwnd.Handle)); + // ...But we have a potential problem: If the SystemResources listener window gets + // destroyed ahead of the call to HwndHost.OnDispatcherShutdown(), the HwndHost's window + // will be destroyed too, before the "logical" Dispose has had a chance to do proper + // shutdown. This turns out to be very significant for WebBrowser/ActiveXHost, which shuts + // down the hosted control through the COM interfaces, and the control destroys its + // window internally. Evidently, the WebOC fails to do full, proper cleanup if its + // window is destroyed unexpectedly. + // To avoid this situation, we make sure SystemResources responds to the Dispatcher + // shutdown event after this HwndHost. + SystemResources.DelayHwndShutdown(); + } } } finally diff --git a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/SystemResources.cs b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/SystemResources.cs index 4817baac88c..1bc74098942 100644 --- a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/SystemResources.cs +++ b/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/SystemResources.cs @@ -1023,7 +1023,7 @@ internal static void OnThemeChanged() /// This is the default HWND used to listen for theme-change messages. /// /// When is true, additional notification windows are created - /// on-demand by or + /// on-demand by /// as the need arises. For e.g., when calls into , /// we would look for a notify-window that matches both (a) DPI Awareness Context and (b) DPI Scale factor of the foreign window from HwndHost to return. If none is found, /// we would create one and add it to our list in and return the newly created notify-window. @@ -1054,27 +1054,6 @@ private static void EnsureResourceChangeListener() } } - /// - /// Ensures that the notify-window corresponding to a given has been - /// created. - /// - /// DPI Awareness Context for which notify-window has to be ensured - private static void EnsureResourceChangeListener(DpiAwarenessContextValue dpiContextValue) - { - EnsureResourceChangeListener(); - - // Test if _hwndNotify has a key that contains dpiContextValue - otherwise create and add a notify-window with - // this DPI Awareness Context - if (_hwndNotify.Keys.FirstOrDefault((hwndDpiContext) => hwndDpiContext.DpiAwarenessContextValue == dpiContextValue) == null) - { - var hwndDpiInfo = CreateResourceChangeListenerWindow(dpiContextValue); - if (!_dpiAwarenessContextAndDpis.Contains(hwndDpiInfo)) - { - _dpiAwarenessContextAndDpis.Add(hwndDpiInfo); - } - } - } - /// /// Ensures that a notify-window corresponding to a given HwndDpiInfo(=DpiAwarenessContextValue + DpiScale) has been /// created. @@ -1085,9 +1064,16 @@ private static void EnsureResourceChangeListener(DpiAwarenessContextValue dpiCon /// whose characteristics we are trying to match, we are guaranteed that the DPI Scale factor of the newly created HWND /// would be identical to that of the reference HWND. /// - private static void EnsureResourceChangeListener(DpiUtil.HwndDpiInfo hwndDpiInfo) + private static bool EnsureResourceChangeListener(DpiUtil.HwndDpiInfo hwndDpiInfo) { EnsureResourceChangeListener(); + + // It's meaningless to ensure RCL for Invalid DACV + if (hwndDpiInfo.DpiAwarenessContextValue == DpiAwarenessContextValue.Invalid) + { + return false; + } + if (!_hwndNotify.ContainsKey(hwndDpiInfo)) { var hwndDpiInfoKey = @@ -1095,13 +1081,15 @@ private static void EnsureResourceChangeListener(DpiUtil.HwndDpiInfo hwndDpiInfo hwndDpiInfo.DpiAwarenessContextValue, hwndDpiInfo.ContainingMonitorScreenRect.left, hwndDpiInfo.ContainingMonitorScreenRect.top); - Debug.Assert(hwndDpiInfo == hwndDpiInfoKey); - if (!_dpiAwarenessContextAndDpis.Contains(hwndDpiInfo)) + if (hwndDpiInfoKey == hwndDpiInfo && // If hwndDpiInfoKey != hwndDpiInfo, something is wrong, abort. + !_dpiAwarenessContextAndDpis.Contains(hwndDpiInfo)) { _dpiAwarenessContextAndDpis.Add(hwndDpiInfo); } } + + return _hwndNotify.ContainsKey(hwndDpiInfo); } /// @@ -1113,7 +1101,7 @@ private static void EnsureResourceChangeListener(DpiUtil.HwndDpiInfo hwndDpiInfo /// y-coordinate position on the screen where the window is to be created /// /// Assumes that and have been initialized. This method - /// must only be called by or + /// must only be called by or /// /// of the newly created , which is also a new key added into private static DpiUtil.HwndDpiInfo CreateResourceChangeListenerWindow(DpiAwarenessContextValue dpiContextValue, int x = 0, int y = 0, [System.Runtime.CompilerServices.CallerMemberName] string callerName = "") @@ -1652,8 +1640,12 @@ internal static HwndWrapper GetDpiAwarenessCompatibleNotificationWindow(HandleRe DpiUtil.GetExtendedDpiInfoForWindow(hwnd.Handle, fallbackToNearestMonitorHeuristic: true) : new DpiUtil.HwndDpiInfo(processDpiAwarenessContextValue, GetDpiScaleForUnawareOrSystemAwareContext(processDpiAwarenessContextValue)); - EnsureResourceChangeListener(hwndDpiInfo); - return _hwndNotify[hwndDpiInfo].Value; + if (EnsureResourceChangeListener(hwndDpiInfo)) + { + return _hwndNotify[hwndDpiInfo].Value; + } + + return null; } ///