Prevent NULL HWND's from being parented under SystemResources listener windows #2100
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…uildOrReparentWindow`. 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.
ghost
requested a review
from 
ghost
added
the
vatsan-madhavan
added
issue-type-netfx-port
rladuca
approved these changes
Oct 24, 2019
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #2089
When an
HwndHostreceivesSourceChangedevent, it goes throughBuildOrReparentWindow. When the hosted window is invisible, it is usually reparented under a temporary windows maintained by WPF in theSystemResourcesclass, 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
NULLHWND'sare attempted to be parented toSystemResourcesmanaged temporary windows. This bug goes back quite a while (.NET 4.5 likely). WPF seems to ignore the return value fromkernel32!SetParentand 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!SetParenton aNULLHWND, 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 theDPI_AWARENESS_CONTEXTvalue 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 inSystemResourcesandHwndHostare defensive improvements.SystemResources.EnsureResourceChangeListener(HwndDpiInfo)can attempt to create a parking-window corresponding toDPI_AWARENESS_CONTEXT_VALUEthat is invalid/meaningless. This should not be allowed. A few additional checks are added to ensure this. Further,GetDpiAwarenessCompatibleNotificationWindowis augmented to be more defensive.Also, variant of
EnsureResourceChangeListeneris dead code - it is being removed.If for some unknown reason
SystemResources.GetDpiAwarenessCompatibleNotificationWindowfails and returnsnulltoHwndHost.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.