[PATCH v2 1/2] strmbase: Do not notify the parent of the window's destruction.
Fixes a regression introduced by 3b5198c8283d891095612c1001edb5e5788d6059. Media Player Classic deadlocks when the window is destroyed, because DestroyWindow will notify the parent, which is on a different thread and waiting on a signal from the filter's thread. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- Tests (next patch) also show that Windows does not notify the parent either, despite not having the style set prior to destruction (without this patch, the test would fail on Wine). dlls/strmbase/window.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/dlls/strmbase/window.c b/dlls/strmbase/window.c index 07a12ec..d646f7e 100644 --- a/dlls/strmbase/window.c +++ b/dlls/strmbase/window.c @@ -77,6 +77,13 @@ static LRESULT CALLBACK WndProcW(HWND hwnd, UINT message, WPARAM wparam, LPARAM This->Width = LOWORD(lparam); This->Height = HIWORD(lparam); + break; + case WM_CLOSE: + /* Some applications like Media Player Classic deadlock when the parent, + which is on a different thread, is notified before the destruction. + Windows also doesn't notify it, despite not having the style prior. */ + SetWindowLongW(hwnd, GWL_EXSTYLE, GetWindowLongW(hwnd, GWL_EXSTYLE) | WS_EX_NOPARENTNOTIFY); + break; } return DefWindowProcW(hwnd, message, wparam, lparam); -- 2.21.0
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/quartz/tests/filtergraph.c | 44 ++++++++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index ca45031..95b2082 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -4232,16 +4232,39 @@ static HWND get_renderer_hwnd(IFilterGraph2 *graph) return hwnd; } +static LRESULT CALLBACK parent_wndproc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) +{ + switch (msg) + { + case WM_PARENTNOTIFY: + if (LOWORD(wParam) == WM_DESTROY) + ok(0, "Received WM_PARENTNOTIFY with WM_DESTROY.\n"); + break; + } + return DefWindowProcA(hwnd, msg, wParam, lParam); +} + static void test_window_threading(void) { WCHAR *filename = load_resource(avifile); IFilterGraph2 *graph = create_graph(); + IVideoWindow *videownd; + WNDCLASSA cls = { 0 }; + HWND hwnd, parent; HRESULT hr; DWORD tid; ULONG ref; - HWND hwnd; BOOL ret; + cls.lpfnWndProc = parent_wndproc; + cls.hInstance = GetModuleHandleA(NULL); + cls.hCursor = LoadCursorA(0, (const char*)IDC_ARROW); + cls.lpszClassName = "TestParent"; + RegisterClassA(&cls); + + parent = CreateWindowExA(0, "TestParent", NULL, WS_OVERLAPPEDWINDOW, 50, 50, 150, 150, NULL, NULL, cls.hInstance, NULL); + ok(parent != NULL, "Failed to create parent window.\n"); + hr = IFilterGraph2_RenderFile(graph, filename, NULL); if (FAILED(hr)) { @@ -4259,6 +4282,14 @@ static void test_window_threading(void) /* The thread should be processing messages, or this will hang. */ SendMessageA(hwnd, WM_NULL, 0, 0); + + /* The WM_DESTROY notification is not sent to the owner, even if the style is off */ + hr = IFilterGraph2_QueryInterface(graph, &IID_IVideoWindow, (void**)&videownd); + ok(hr == S_OK, "Got hr %#x.\n", hr); + hr = IVideoWindow_put_Owner(videownd, (OAHWND)parent); + ok(hr == S_OK, "Got hr %#x.\n", hr); + IVideoWindow_Release(videownd); + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); } else skip("Could not find renderer window.\n"); @@ -4277,6 +4308,14 @@ static void test_window_threading(void) { tid = GetWindowThreadProcessId(hwnd, NULL); ok(tid == GetCurrentThreadId(), "Window should be created on main thread.\n"); + + /* The WM_DESTROY notification is not sent to the owner, even if the style is off */ + hr = IFilterGraph2_QueryInterface(graph, &IID_IVideoWindow, (void**)&videownd); + ok(hr == S_OK, "Got hr %#x.\n", hr); + hr = IVideoWindow_put_Owner(videownd, (OAHWND)parent); + ok(hr == S_OK, "Got hr %#x.\n", hr); + IVideoWindow_Release(videownd); + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); } else skip("Could not find renderer window.\n"); @@ -4285,6 +4324,9 @@ static void test_window_threading(void) ok(!ref, "Got outstanding refcount %d.\n", ref); ret = DeleteFileW(filename); ok(ret, "Failed to delete file, error %u.\n", GetLastError()); + + DestroyWindow(parent); + UnregisterClassA("TestParent", cls.hInstance); } START_TEST(filtergraph) -- 2.21.0
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=63097 Your paranoid android. === w1064v1809_he (32 bit report) === quartz: filtergraph.c:527: Test failed: didn't get EOS filtergraph.c:532: Test failed: expected 2ccdba, got 0
On 10/01/2020 14:39, Marvin wrote:
Hi,
While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check?
Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=63097
Your paranoid android.
=== w1064v1809_he (32 bit report) ===
quartz: filtergraph.c:527: Test failed: didn't get EOS filtergraph.c:532: Test failed: expected 2ccdba, got 0
Seems like a totally unrelated error that happens randomly on the testbot.
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
+ /* The WM_DESTROY notification is not sent to the owner, even if the style is off */ + hr = IFilterGraph2_QueryInterface(graph, &IID_IVideoWindow, (void**)&videownd); + ok(hr == S_OK, "Got hr %#x.\n", hr); + hr = IVideoWindow_put_Owner(videownd, (OAHWND)parent); + ok(hr == S_OK, "Got hr %#x.\n", hr); + IVideoWindow_Release(videownd); + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); } else skip("Could not find renderer window.\n"); @@ -4285,6 +4324,9 @@ static void test_window_threading(void) ok(!ref, "Got outstanding refcount %d.\n", ref); ret = DeleteFileW(filename); ok(ret, "Failed to delete file, error %u.\n", GetLastError()); + + DestroyWindow(parent); + UnregisterClassA("TestParent", cls.hInstance); }
It would be also helpful to test if the window gets subclassed, setting WS_EX_NOPARENTNOTIFY on WM_CLOSE doesn't look like a plausible explanation, it's rather a hack that seems to work. Probably it's better to put some effort to figuring out what is going on and how this works on Windows. -- Dmitry.
On 10/01/2020 17:37, Dmitry Timoshkov wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
+ /* The WM_DESTROY notification is not sent to the owner, even if the style is off */ + hr = IFilterGraph2_QueryInterface(graph, &IID_IVideoWindow, (void**)&videownd); + ok(hr == S_OK, "Got hr %#x.\n", hr); + hr = IVideoWindow_put_Owner(videownd, (OAHWND)parent); + ok(hr == S_OK, "Got hr %#x.\n", hr); + IVideoWindow_Release(videownd); + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); } else skip("Could not find renderer window.\n"); @@ -4285,6 +4324,9 @@ static void test_window_threading(void) ok(!ref, "Got outstanding refcount %d.\n", ref); ret = DeleteFileW(filename); ok(ret, "Failed to delete file, error %u.\n", GetLastError()); + + DestroyWindow(parent); + UnregisterClassA("TestParent", cls.hInstance); }
It would be also helpful to test if the window gets subclassed, setting WS_EX_NOPARENTNOTIFY on WM_CLOSE doesn't look like a plausible explanation, it's rather a hack that seems to work. Probably it's better to put some effort to figuring out what is going on and how this works on Windows.
Yeah I'll do a bit more investigations on this with subclassing the video window and checking it under Windows, but that's not going to end up in wine tests since it's too much of a hack / implementation detail. However, it will help with understanding the picture a bit better. FWIW I'll probably go with unparenting the window on WM_CLOSE instead. Does that seem more reasonable / less of a hack? It's not strictly about WM_CLOSE itself, it just has to happen before DestroyWindow gets called. Thanks, Gabriel
participants (3)
-
Dmitry Timoshkov -
Gabriel Ivăncescu -
Marvin