Check for the return value of native SetParent calls in Control class

[chenxinyanc]

Fixes #2163

Proposed changes

• Check for the return value and GetLastError in RecreateHandleCore and SetParentHandle methods.
• Did not add the UT for this yet, because when Control.CreateHandle calls Application.ParkHandle, it does not specify the DPI awareness (DPI_AWARENESS_CONTEXT_UNSPECIFIED), so ParkHandle will choose a parking window for UNSPECIFIED awareness context. When constructing the parking window for UNSPECIFIED context, the DPI awareness of current thread will be used (e.g. PerMonitorV2). This will causes the subsequently parked window uses the very same DPI awareness setting, regardless of their owning thread's current DPI awareness at the point of control instance / handle construction.
  • I'm not sure whether it's by design or not, but perhaps we need to specify the DPI awareness explicitly when calling ParkHandle in CreateHandle method.
  • Nevertheless, I've tested the behavior with https://github.com/chenxinyanc/WinFormSetParentTest and it throws Win32Exception accordingly.

Customer Impact

Regression?

• Yes / No

Risk

Screenshots

Before

After

Test methodology

Accessibility testing

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

[dnfclas]

All CLA requirements met.

[codecov]

Codecov Report

Merging #2262 into master will decrease coverage by 0.01755%.
The diff coverage is 40%.

@@                 Coverage Diff                 @@
##              master       #2262         +/-   ##
===================================================
- Coverage   29.45241%   29.43485%   -0.01756%     
===================================================
  Files           1005         945         -60     
  Lines         259629      266565       +6936     
  Branches       36855       37943       +1088     
===================================================
+ Hits           76467       78463       +1996     
- Misses        178224      182909       +4685     
- Partials        4938        5193        +255

Flag	Coverage Δ
#Debug	29.43485% <40%> (-0.01756%)	⬇️
#production	29.43485% <40%> (-0.01756%)	⬇️
#test	100% <ø> (ø)	⬆️

[RussKie]

You must sign CLA before we can accept the submission.
Few stylistic issues

[RussKie]

@hughbe look like we overlooked an overload public static IntPtr SetParent(IHandle hWndChild, IHandle hWndChildNewParent)

[chenxinyanc]

The test seems to get stuck on CI pipeline for 57min… Weird.

[RussKie]

@JeremyKuhne could you please review as well?

[JeremyKuhne]

Since we're dependent on recreationSuccessful this try ... finally can move to wrap the block above that I suggested to move. It would look something like this:

// Do the main work of recreating the handle
DestroyHandle();
CreateHandle();
SetState(States.Recreate, false);

try
{
    // Inform children that their parent's handle has recreated.

    // ...
}
finally
{
    // If there is a parent window that isn't a WinForms window we need to reparent manually.
    // If the parent was a WinForms Control, CreateControl will have taken care of this.
    if (parentHandle.Handle != IntPtr.Zero

    //  ...

[chenxinyanc]

I'm moving re-parenting Win32 window part outside the finally block, as we should not expect to recover anything / keep anything consistent from ThreadAbortException in the main thread, and this can reduce 1 level of nesting try block.

However, I'm keeping the CreateHandle and DestroyHandle part inside try block, as we can still try our best to keep the states (States.Created, States.Recreate, Parent value of the controls in controlSnapshot) consistent when there is an Exception.

[JeremyKuhne]

Note that there are no ThreadAbortException throws in Core. (@RussKie, not sure if you knew this.)

To simplify the code for these flags it would be worth looking at creating ref struct disposables that set and clear specified flags. Pattern dispose and the new syntax for using can help us reduce nesting and cognitive overload on some of our try .. finally blocks. (And it is efficient when using structs like this as there are no heap allocations.)

[JeremyKuhne]

Looks good!

[RussKie]

This change appears to have caused a regression reported in #6093