Fix regression in ParkHandle method from .NET framework #4796

dreddy-work · 2021-04-15T21:11:01Z

Seems this changes was missed from Day 1 and a regression from .NET Framework.
Ex: https://referencesource.microsoft.com/#System.Windows.Forms/winforms/Managed/System/WinForms/Control.cs,8435

Without these changes, on PMv2 applications, We might be ending up with wrong DpiAwareness than the requested for windows being created and some times, might end up failing to parent because of inconsistence DpiAwareness between parent and child.

#2262 addressed for catching the failure in 'SetParent' but applications may be still creating windows with incorrect DpiAwareness.

Need to check on how to add a Unit test.

Microsoft Reviewers: Open in CodeFlow

…ramework. Without these changes, on PMv2 applications, We might be ending up with wrong Dpiawareness than the requested for windows and some times, might endup failing to parent because of inconsitance DpIcontexts. #2262 addressed for catching the failure but still applications may be still creating wrong DPIawareness windows.

…eddy/FixParkingWindowRegression

RussKie

A little more work needed.

Please rebase instead of tracking-merges

src/System.Windows.Forms/tests/UnitTests/HDPI/ParkingWindowTests.cs

dreddy-work · 2021-05-20T03:30:42Z

Tests are failing random but there is a genuine issue. Will be investigating.

RussKie · 2021-05-20T14:16:55Z

    System.Windows.Forms.Tests.AccessibleObjects.ErrorProviderAccessibleObjectTests.ErrorProvider_NameDoesntEqualControlTypeOrChildName [FAIL]
      System.ComponentModel.Win32Exception : Failed to set Win32 parent window of the Control.
      Stack Trace:
        /_/src/System.Windows.Forms/src/System/Windows/Forms/Control.cs(10810,0): at System.Windows.Forms.Control.SetParentHandle(IntPtr value)
        /_/src/System.Windows.Forms/src/System/Windows/Forms/Control.ControlCollection.cs(121,0): at System.Windows.Forms.Control.ControlCollection.Add(Control value)
        /_/src/System.Windows.Forms/src/System/Windows/Forms/Form.ControlCollection.cs(58,0): at System.Windows.Forms.Form.ControlCollection.Add(Control value)
        /_/src/System.Windows.Forms/tests/UnitTests/AccessibleObjects/ErrorProviderAccessibleObjectTests.cs(36,0): at System.Windows.Forms.Tests.AccessibleObjects.ErrorProviderAccessibleObjectTests..ctor()
           at System.RuntimeType.CreateInstanceDefaultCtor(Boolean publicOnly, Boolean wrapExceptions) in System.Private.CoreLib.dll:token 0x6000644+0x66

…n CI build and that too after adding new tests that were setting processwide dpi awareness, I am guessing filaure is because of incorrect DPIawareness between the threads while running tests. Replacing process wide DPI awareness with thread.

dreddy-work · 2021-05-20T19:12:28Z

Tests are failing random but there is a genuine issue. Will be investigating.

Given that these failures are reproduced only in PR build and that too after adding new tests for Parkingwindow that were setting process wide dpi awareness, I am guessing failure is because of incorrect DPI awareness of the threads within the test case execution. Replacing process wide DPI awareness with thread and resetting back after finishing the test. This helped resolve test issue. This change is critical for control/handle creation and impact all DPI awareness modes. This is high risk change and would need to be validated thoroughly in this preview.

Tanya-Solyanik

The port itself looks good.
What makes you say that the change is risky? It looks like a direct port from the FX, no?

Could yo please explain what the tests are doing?

src/System.Windows.Forms/src/System/Windows/Forms/NativeWindow.cs

...em.Windows.Forms/tests/UnitTests/HDPI/System/Windows/Forms/Application.ParkingWindowTests.cs

dreddy-work · 2021-05-21T06:21:24Z

What makes you say that the change is risky? It looks like a direct port from the FX, no?

It is risky because it was impacting all applications irrespective of DPI mode. Yes it does match with Framework but PMV2 mode usage in framework should be very less comparatively.

dreddy-work · 2021-05-21T06:23:16Z

Could yo please explain what the tests are doing?

Tests were intended to validate that we indeed creating parking windows with expected DPI awareness on them. Setting the scope for control creation and expect the parking handle creation in same DPI awareness when control Handle is created.

dreddy-work · 2021-05-21T16:47:07Z

/azp run

azure-pipelines · 2021-05-21T16:47:24Z

Azure Pipelines successfully started running 2 pipeline(s).

Tanya-Solyanik

I seem to remember that SetThreadDpiAwarenessContext is a no-op in a SA or unaware process as well, had you tested that this API works better than scope does.

Tests were intended to validate that we indeed creating parking windows with expected DPI awareness on them. Setting the scope for control creation and expect the parking handle creation in same DPI awareness when control Handle is created.

I don't think that each fact is a new Application, so parking windows created in any other test would persist through the test run, so you are not necessarily verifying creation of this window. These tests verify that appropriate parking window gets associates with each context. I think the first 2 tests can be merged into theory.

its restoring to original value before we set to PMV2. API returns the value it is replacing
I see it now, :) I missed it the first time.

Tanya-Solyanik · 2021-05-22T01:47:41Z

src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/Application.ParkingWindowTests.cs

+                    Assert.NotNull(parkingWindow);
+
+                    IntPtr dpiContext = User32.GetWindowDpiAwarenessContext(parkingWindow.Handle);
+                    Assert.True(User32.AreDpiAwarenessContextsEqual(User32.DPI_AWARENESS_CONTEXT.UNAWARE, dpiContext));


AssertExtention that compares DPI awareness contexts would be helpful here and in the future. You can factor out this line into a helper method.

Tanya-Solyanik · 2021-05-22T01:51:02Z

src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/Application.ParkingWindowTests.cs

+        }
+
+        [WinFormsFact]
+        public void ParkingWindow_PermonitorV2()


Suggestion: ParkingWindow_PerMonitorV2

dreddy-work · 2021-05-22T03:20:25Z

I seem to remember that SetThreadDpiAwarenessContext is a no-op in a SA or unaware process as well, had you tested that this API works better than scope does.

Tested. It does set to the context passed.

I don't think that each fact is a new Application, so parking windows created in any other test would persist through the test run, so you are not necessarily verifying creation of this window. These tests verify that appropriate parking window gets associates with each context. I think the first 2 tests can be merged into theory.

Agree as our process is already running in UNAWARE and unaware parking window might have already been there.

RussKie · 2021-05-23T09:14:56Z

Tests that alter/affect global singletons (e.g. Application) must be done as MAUI tests or run under RemoteExecutor (subject to dotnet/arcade#7426)

dreddy-work requested a review from a team as a code owner April 15, 2021 21:11

ghost assigned dreddy-work Apr 15, 2021

dreddy-work added the area-HDPI-SA Issues related to high DPI SystemAware mode label Apr 15, 2021

RussKie added the 🚫 * NO-MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 19, 2021

RussKie changed the title ~~[Do not merge] Fix regression in ParkHandle method from .NET framework~~ Fix regression in ParkHandle method from .NET framework Apr 19, 2021

RussKie marked this pull request as draft April 19, 2021 12:10

dreddy-work marked this pull request as ready for review May 19, 2021 16:13

dreddy-work added 3 commits May 19, 2021 10:35

Merge branch 'main' of https://github.com/dotnet/winforms into dev/dr…

ed13c84

…eddy/FixParkingWindowRegression

Merge branch 'main' of https://github.com/dotnet/winforms into dev/dr…

2480252

…eddy/FixParkingWindowRegression

Adding tests around Parking window creation in various contexts.

dbe2f06

dreddy-work removed the 🚫 * NO-MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label May 19, 2021

Fixing build failure by analyzer.

67fb2f4

RussKie requested changes May 19, 2021

View reviewed changes

ghost added 📭 waiting-author-feedback The team requires more information from the author and removed 📭 waiting-author-feedback The team requires more information from the author labels May 19, 2021

Review feedback.

d4e5778

dreddy-work requested a review from RussKie May 20, 2021 00:30

RussKie reviewed May 20, 2021

View reviewed changes

src/System.Windows.Forms/tests/UnitTests/HDPI/ParkingWindowTests.cs Outdated Show resolved Hide resolved

RussKie reviewed May 20, 2021

View reviewed changes

src/System.Windows.Forms/tests/UnitTests/HDPI/ParkingWindowTests.cs Outdated Show resolved Hide resolved

RussKie reviewed May 20, 2021

View reviewed changes

src/System.Windows.Forms/tests/UnitTests/HDPI/ParkingWindowTests.cs Outdated Show resolved Hide resolved

RussKie reviewed May 20, 2021

View reviewed changes

src/System.Windows.Forms/tests/UnitTests/HDPI/ParkingWindowTests.cs Outdated Show resolved Hide resolved

RussKie added the 📭 waiting-author-feedback The team requires more information from the author label May 20, 2021

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label May 20, 2021

RussKie added the 📭 waiting-author-feedback The team requires more information from the author label May 20, 2021

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label May 20, 2021

dreddy-work added 2 commits May 20, 2021 11:15

reformatting tests.

f95f4da

dreddy-work requested review from RussKie and Tanya-Solyanik May 20, 2021 19:12

RussKie previously approved these changes May 20, 2021

View reviewed changes

Tanya-Solyanik reviewed May 21, 2021

View reviewed changes

Review suggestions.

fa48427

dreddy-work dismissed RussKie’s stale review via fa48427 May 21, 2021 07:31

dreddy-work requested a review from RussKie May 21, 2021 07:33

Running tests only on Windows 10

ea3802e

dreddy-work merged commit 137b968 into main May 21, 2021

ghost added this to the 6.0 Preview6 milestone May 21, 2021

dreddy-work deleted the dev/dreddy/FixParkingWindowRegression branch May 21, 2021 17:10

Tanya-Solyanik reviewed May 22, 2021

View reviewed changes

dreddy-work mentioned this pull request Nov 5, 2021

Create new .NET 6.0 pages dotnet/docs-desktop#1199

Merged

ghost locked as resolved and limited conversation to collaborators Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix regression in ParkHandle method from .NET framework #4796

Fix regression in ParkHandle method from .NET framework #4796

dreddy-work commented Apr 15, 2021 •

edited by ghost

Loading

RussKie left a comment

dreddy-work commented May 20, 2021

RussKie commented May 20, 2021

dreddy-work commented May 20, 2021 •

edited

Loading

Tanya-Solyanik left a comment

dreddy-work commented May 21, 2021 •

edited

Loading

dreddy-work commented May 21, 2021

dreddy-work commented May 21, 2021

azure-pipelines bot commented May 21, 2021

Tanya-Solyanik left a comment

Tanya-Solyanik May 22, 2021

Tanya-Solyanik May 22, 2021

dreddy-work commented May 22, 2021

RussKie commented May 23, 2021

Fix regression in ParkHandle method from .NET framework #4796

Fix regression in ParkHandle method from .NET framework #4796

Conversation

dreddy-work commented Apr 15, 2021 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

RussKie left a comment

Choose a reason for hiding this comment

dreddy-work commented May 20, 2021

RussKie commented May 20, 2021

dreddy-work commented May 20, 2021 • edited Loading

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

dreddy-work commented May 21, 2021 • edited Loading

dreddy-work commented May 21, 2021

dreddy-work commented May 21, 2021

azure-pipelines bot commented May 21, 2021

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Tanya-Solyanik May 22, 2021

Choose a reason for hiding this comment

Tanya-Solyanik May 22, 2021

Choose a reason for hiding this comment

dreddy-work commented May 22, 2021

RussKie commented May 23, 2021

dreddy-work commented Apr 15, 2021 •

edited by ghost

Loading

dreddy-work commented May 20, 2021 •

edited

Loading

dreddy-work commented May 21, 2021 •

edited

Loading