[release/3.1] Prevent NULL HWND's from being parented under SystemResources listener windows #2101
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 
vatsan-madhavan
added
Bug
rladuca
approved these changes
Oct 24, 2019
vatsan-madhavan
added
ask-mode
* NO MERGE *
vatsan-madhavan
added
auto_merge
|
Hello @vatsan-madhavan! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
locked as resolved and limited conversation to collaborators
This pull request was closed.
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
.NET 5 PR: #2100
Description (Summary)
When
HwndHosthostedHWND's need to be "parked" while theHwndHostis not being actively shown, WPF will reparent suchHWND's under temporary message-only windows maintained inside theSystemResourcesclass.There is a bug that causes null
HWND's to be parented under such message-only windows. When this happens in High-DPI applications that use mixed-mode DPI capabilities introduced by WPF in .NET 4.8 (parent and child windows with differentDPI_AWARENESS_CONTEXTvalues), a crash ensues.Customer Impact
This is a fix for a crash affecting several applications including Visual Studio, and Azure Information Protection Add-in for Office
This was fixed recently in .NET 4.8, and is being forwarded ported to .NET Core for consistency.
Regresssion
Not a regression in .NET Core, but this was a regression introduced by .NET 4.8.
Risk
The fix is small and well understood, and has been tested well. The .NET Framework version of this fix has been validated by Visual Studio (the codebases are identical in this area and .NET Framework testing is a reliable proxy for this change in .NET Core).
Details
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.