[PATCH 0/1] MR7968: d3d9/tests: Replace incorrect "broken(1)" condition with flaky.
The test fails randomly on Windows 10 too, as shown in: https://testbot.winehq.org/JobDetails.pl?Key=158165 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58196 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7968
From: Rémi Bernon <rbernon(a)codeweavers.com> The test fails randomly on Windows 10 too, as shown in: https://testbot.winehq.org/JobDetails.pl?Key=158165 Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58196 --- dlls/d3d9/tests/device.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dlls/d3d9/tests/device.c b/dlls/d3d9/tests/device.c index a776c112c39..c1679ed0acc 100644 --- a/dlls/d3d9/tests/device.c +++ b/dlls/d3d9/tests/device.c @@ -4464,8 +4464,7 @@ static void test_wndproc(void) ok(!expect_messages->message, "Expected message %#x for window %#x, but didn't receive it, i=%u.\n", expect_messages->message, expect_messages->window, i); /* About 1 in 8 test runs receives WM_WINDOWPOSCHANGED on Vista. */ - ok(!windowposchanged_received || broken(1), - "Received WM_WINDOWPOSCHANGED but did not expect it, i=%u.\n", i); + flaky ok(!windowposchanged_received, "Received WM_WINDOWPOSCHANGED but did not expect it, i=%u.\n", i); expect_messages = NULL; /* On Windows 10 style change messages are delivered both on reset and -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7968
Wait, doesn't this make it flaky on Wine too? Is that the motivation for this change? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7968#note_102591
Wait, doesn't this make it flaky on Wine too? Is that the motivation for this change?
Ah, now that I see the bug report that is indeed the motivation. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7968#note_102595
The problem with "flaky", especially in a situation like this where it's being apparently abused for a test that isn't flaky at all but actually fails every time, is that it makes the test meaningless. That might be okay, especially if Windows doesn't consistently pass either, but in that case we should just remove the line entirely. Stefan, is this test important at all? I assume that WM_WINDOWPOSCHANGED is important enough that applications care about it, since we bother to explicitly test for it—if so, can we figure out how to rewrite the test to closely enough match the application behaviour so that it consistently passes? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7968#note_102625
The following is from years of memory, so it might not be accurate. It's been more than a decade since I added the tests. TL;DR: Remove or if(0) the test. The reason why this test tests the *absence* of WM_WINDOWPOSCHANGING and related messages here is that some games call Device::Reset from their WM_WINDOWPOSCHANGED handler. Due to d3d9's filtering of d3d9-caused messages, this does not lead to an endless recursion. The concept was introduced in c24e48d937cfd0000a22af555a920ea7fab71d7b for a game that crashed during device creation because it reset the not yet created device on the fullscreen mode switch + window resize. This particular line is checking that the same applies on focus loss / regain. I don't know if there's any application that would otherwise run into an endless loop during focus loss. I couldn't find out which game was affected because the commit doesn't say, no bug report is I think (but again, memory from my last time I tried to make those tests more reliable) the test fails because there are other reasons why WM_WINDOWPOSCHANGING can be sent, and because they are outside of d3d9's (or wined3d's) window operations, they are not filtered and the test can't tell them apart. A lot of the flush_events() calls are there to prevent external events from interfering, but it isn't wholly reliable everywhere. There is no way to ask d3d9 if device creation/focus loss/focus regain/device reset/device destruction is ongoing from inside our message handler and a "I am calling d3d now" flag in the test is racy vs external events. Another aspect specific to WM_WINDOWPOSCHANGED is that some window operations are no-ops and only send a WM_WINDOWPOSCHANGING, but not a WM_WINDOWPOSCHANGED message. However, what is a no-op and what is not depends on the window manager and I remember that kwin - which is otherwise very well behaved - decided to move or resize windows in some situations and caused a WM_WINDOWPOSCHANGED message to be sent anyhow. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7968#note_102828
Thanks. In that case it seems like the prudent thing to do would in fact be to call Reset() from WM_WINDOWPOSCHANGED, as the application does, and probably also test that we did so at most twice? I imagine we might have been afraid to do so lest a failing test result in a hang or crash rather than a simple "test failed" message, but I at least am of the school of thought that a failing test is a failing test, regardless of the mode of failure. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7968#note_102834
On Thu May 8 16:51:45 2025 +0000, Elizabeth Figura wrote:
Thanks. In that case it seems like the prudent thing to do would in fact be to call Reset() from WM_WINDOWPOSCHANGED, as the application does, and probably also test that we did so at most twice? I imagine we might have been afraid to do so lest a failing test result in a hang or crash rather than a simple "test failed" message, but I at least am of the school of thought that a failing test is a failing test, regardless of the mode of failure. The recursive reset() is just one of the ways some applications depend on the message sequence. It wouldn't be a good replacement for the existing tests, although it would certainly be worth adding to explicitly test one of the ways a known [0] application fails.
Some of the other pickiness I came across while implementing the alt-tab handling: Again this is from memory, I don't remember any more which application did what, but maybe we can figure it out from the archives: * Some application insists that WM_SYSCOMMAND(SC_RESTORE) is delivered after all the focus gain, screen res change, window size change messages. WM_SYSCOMMAND is actually the first thing that gets sent on task bar click, and only because the rest of the stuff gets sent by d3d9's WM_SYSCOMMAND handler is it the last one delivered to the application * Some application (I think Command & Conquer 3) sets the focus window to something else and then back to the main application while not processing messages on the window's thread. user32 is cancelling out the two WM_ACTIVATEAPP messages * Some other game (I think Warcraft 3) hides the device window with ShowWindow(SW_HIDE) and crashes when a WM_WINDOWPOSCHANGING message arrives. That's where the weirdo special !WS_VISIBLE checks come from. It has focus window != device window. * Deus Ex (ddraw) destroys the ddraw device in WM_KILLFOCUS and expects it to work. wined3d and ddraw are therefore very careful not to access device->xxx after a certain point. Fullscreen focus switching on Windows always had a reputation of being fragile, and sadly Wine and the many, many window managers it runs on is no different. But the picky (and fragile) tests were built over time for good reason. [0]: I don't know which application prompted the original test_wndproc(). Henri did that work and I couldn't figure out from the archives and bug tracker which application motivated that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7968#note_102837
On Thu May 8 16:51:45 2025 +0000, Stefan Dösinger wrote:
The recursive reset() is just one of the ways some applications depend on the message sequence. It wouldn't be a good replacement for the existing tests, although it would certainly be worth adding to explicitly test one of the ways a known [0] application fails. Some of the other pickiness I came across while implementing the alt-tab handling: Again this is from memory, I don't remember any more which application did what, but maybe we can figure it out from the archives: * Some application insists that WM_SYSCOMMAND(SC_RESTORE) is delivered after all the focus gain, screen res change, window size change messages. WM_SYSCOMMAND is actually the first thing that gets sent on task bar click, and only because the rest of the stuff gets sent by d3d9's WM_SYSCOMMAND handler is it the last one delivered to the application * Some application (I think Command & Conquer 3) sets the focus window to something else and then back to the main application while not processing messages on the window's thread. user32 is cancelling out the two WM_ACTIVATEAPP messages * Some other game (I think Warcraft 3) hides the device window with ShowWindow(SW_HIDE) and crashes when a WM_WINDOWPOSCHANGING message arrives. That's where the weirdo special !WS_VISIBLE checks come from. It has focus window != device window. * Deus Ex (ddraw) destroys the ddraw device in WM_KILLFOCUS and expects it to work. wined3d and ddraw are therefore very careful not to access device->xxx after a certain point. Fullscreen focus switching on Windows always had a reputation of being fragile, and sadly Wine and the many, many window managers it runs on is no different. But the picky (and fragile) tests were built over time for good reason. [0]: I don't know which application prompted the original test_wndproc(). Henri did that work and I couldn't figure out from the archives and bug tracker which application motivated that. Yeah, I'm not proposing to replace the entire tests, just that we should replace that specific test (was WM_WINDOWPOSCHANGED received) with a more direct test (does Reset() recurse forever).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7968#note_102850
On Thu May 8 20:01:02 2025 +0000, Elizabeth Figura wrote:
Yeah, I'm not proposing to replace the entire tests, just that we should replace that specific test (was WM_WINDOWPOSCHANGED received) with a more direct test (does Reset() recurse forever). I had another look at this. The nature of the WM_WINDOWPOSCHANGED messages differ.
On Wine (with kwin as window manager at least), the WM_WINDOWPOSCHANGED messages occur during the flush_events() after SetForegroundWindow returns. So they happen after d3d's message handling. Calling Reset() from the window handler in this situation will break follow-up tests. We could adjust those tests to take a possible call of Reset() into account, but it would take away part of their meaning - in particular, that after regaining focus in d3d9 (non-ex) the application has to call Reset and gets D3DERR_DEVICENOTRESET otherwise. That fact is more important than what kind of message filtering does or does not happen during focus regain. On Windows, the messages generated seem to be a random side effect of SetForegroundWindow itself. It also happens in the D3DCREATE_NOWINDOWCHANGES, although less often than in the case where d3d9 manipulates the device window. Flags are 0x1803 (SWP_NOCLIENTMOVE | SWP_NOCLIENTSIZE | SWP_NOSIZE | SWP_NOMOVE), but absent SWP_NOZORDER | SWP_NOACTIVATE, suggesting to me that the Z order was changed. See https://testbot.winehq.org/JobDetails.pl?Key=158396#k302 w8adm, w864 where it didn't happen and w8 for a case where it happened on i=1. I'll ponder this a bit, run a few more experiments and see. The two options I see are removing the test, or moving the WINDOWPOSCHANGED part in between the SetForegroundWindow and flush_events() + making the broken() on Windows stricter - test WINDOWPOS.flags for NOSIZE | NOMOVE or somesuch. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7968#note_104374
This merge request was closed by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7968
participants (3)
-
Elizabeth Figura (＠zfigura) -
Rémi Bernon
-
Stefan Dösinger