[PATCH 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. So while this may seem awkward, it's needed to replicate Windows behavior. 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 | 35 ++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index ca45031..726c54a 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -4232,16 +4232,38 @@ 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(); + 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)) { @@ -4256,6 +4278,10 @@ static void test_window_threading(void) { tid = GetWindowThreadProcessId(hwnd, NULL); ok(tid != GetCurrentThreadId(), "Window should have been created on a separate thread.\n"); + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); + + SetParent(hwnd, parent); + SetWindowLongW(hwnd, GWL_STYLE, GetWindowLongW(hwnd, GWL_STYLE) | WS_CHILD); /* The thread should be processing messages, or this will hang. */ SendMessageA(hwnd, WM_NULL, 0, 0); @@ -4277,6 +4303,10 @@ static void test_window_threading(void) { tid = GetWindowThreadProcessId(hwnd, NULL); ok(tid == GetCurrentThreadId(), "Window should be created on main thread.\n"); + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); + + SetParent(hwnd, parent); + SetWindowLongW(hwnd, GWL_STYLE, GetWindowLongW(hwnd, GWL_STYLE) | WS_CHILD); } else skip("Could not find renderer window.\n"); @@ -4285,6 +4315,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
On 1/9/20 7:13 AM, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/quartz/tests/filtergraph.c | 35 ++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index ca45031..726c54a 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -4232,16 +4232,38 @@ 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(); + 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)) { @@ -4256,6 +4278,10 @@ static void test_window_threading(void) { tid = GetWindowThreadProcessId(hwnd, NULL); ok(tid != GetCurrentThreadId(), "Window should have been created on a separate thread.\n"); + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); + + SetParent(hwnd, parent); + SetWindowLongW(hwnd, GWL_STYLE, GetWindowLongW(hwnd, GWL_STYLE) | WS_CHILD);
Does the application actually do this, or does it use IVideoWindow::put_Owner()? If it is the former, I'd like to see a comment to that effect.
/* The thread should be processing messages, or this will hang. */ SendMessageA(hwnd, WM_NULL, 0, 0); @@ -4277,6 +4303,10 @@ static void test_window_threading(void) { tid = GetWindowThreadProcessId(hwnd, NULL); ok(tid == GetCurrentThreadId(), "Window should be created on main thread.\n"); + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); + + SetParent(hwnd, parent); + SetWindowLongW(hwnd, GWL_STYLE, GetWindowLongW(hwnd, GWL_STYLE) | WS_CHILD); } else skip("Could not find renderer window.\n"); @@ -4285,6 +4315,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)
On 09/01/2020 16:46, Zebediah Figura wrote:
On 1/9/20 7:13 AM, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/quartz/tests/filtergraph.c | 35 ++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index ca45031..726c54a 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -4232,16 +4232,38 @@ 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(); + 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)) { @@ -4256,6 +4278,10 @@ static void test_window_threading(void) { tid = GetWindowThreadProcessId(hwnd, NULL); ok(tid != GetCurrentThreadId(), "Window should have been created on a separate thread.\n"); + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); + + SetParent(hwnd, parent); + SetWindowLongW(hwnd, GWL_STYLE, GetWindowLongW(hwnd, GWL_STYLE) | WS_CHILD);
Does the application actually do this, or does it use IVideoWindow::put_Owner()? If it is the former, I'd like to see a comment to that effect.
I actually don't know what the app does when it sets the parent. I wrote the test to see if Windows notifies the parent, and it seems that it doesn't (I also set the WS_CHILD just in case), despite not having the WS_EX_NOPARENTNOTIFY style in the first place. Wine would fail this test without the first patch, even without put_Owner. I'll investigate what the app does. However, while this patchset fixes that app, I thought a more generic solution would be preferable, since that's how Windows seems to work (unless you have a better idea). Also, by comment, you meant in the test file as C comment, right? Thanks, Gabriel
On 1/9/20 9:22 AM, Gabriel Ivăncescu wrote:
On 09/01/2020 16:46, Zebediah Figura wrote:
On 1/9/20 7:13 AM, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/quartz/tests/filtergraph.c | 35 ++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index ca45031..726c54a 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -4232,16 +4232,38 @@ 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(); + 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)) { @@ -4256,6 +4278,10 @@ static void test_window_threading(void) { tid = GetWindowThreadProcessId(hwnd, NULL); ok(tid != GetCurrentThreadId(), "Window should have been created on a separate thread.\n"); + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); + + SetParent(hwnd, parent); + SetWindowLongW(hwnd, GWL_STYLE, GetWindowLongW(hwnd, GWL_STYLE) | WS_CHILD);
Does the application actually do this, or does it use IVideoWindow::put_Owner()? If it is the former, I'd like to see a comment to that effect.
I actually don't know what the app does when it sets the parent. I wrote the test to see if Windows notifies the parent, and it seems that it doesn't (I also set the WS_CHILD just in case), despite not having the WS_EX_NOPARENTNOTIFY style in the first place. Wine would fail this test without the first patch, even without put_Owner.
I'll investigate what the app does. However, while this patchset fixes that app, I thought a more generic solution would be preferable, since that's how Windows seems to work (unless you have a better idea).
Manually fiddling with the window handle is not really something applications are supposed to do. I wouldn't necessarily expect anything about that to work. I don't know that it would cause a difference in this case, but it's better to test with public APIs.
Also, by comment, you meant in the test file as C comment, right?
Yes.
Thanks, Gabriel
On Thu, Jan 9, 2020 at 6:23 PM Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
On 09/01/2020 16:46, Zebediah Figura wrote:
On 1/9/20 7:13 AM, Gabriel Ivăncescu wrote:
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/quartz/tests/filtergraph.c | 35 ++++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/dlls/quartz/tests/filtergraph.c b/dlls/quartz/tests/filtergraph.c index ca45031..726c54a 100644 --- a/dlls/quartz/tests/filtergraph.c +++ b/dlls/quartz/tests/filtergraph.c @@ -4232,16 +4232,38 @@ 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(); + 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)) { @@ -4256,6 +4278,10 @@ static void test_window_threading(void) { tid = GetWindowThreadProcessId(hwnd, NULL); ok(tid != GetCurrentThreadId(), "Window should have been created on a separate thread.\n"); + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); + + SetParent(hwnd, parent); + SetWindowLongW(hwnd, GWL_STYLE, GetWindowLongW(hwnd, GWL_STYLE) | WS_CHILD);
Does the application actually do this, or does it use IVideoWindow::put_Owner()? If it is the former, I'd like to see a comment to that effect.
I actually don't know what the app does when it sets the parent. I wrote the test to see if Windows notifies the parent, and it seems that it doesn't (I also set the WS_CHILD just in case), despite not having the WS_EX_NOPARENTNOTIFY style in the first place. Wine would fail this test without the first patch, even without put_Owner.
I'll investigate what the app does. However, while this patchset fixes that app, I thought a more generic solution would be preferable, since that's how Windows seems to work (unless you have a better idea).
I don't understand how this window is set, just from this test alone. But, is it possible to subclass application window, and simply not forward certain messages? You can probably even test if it was subclassed when message returns.
Also, by comment, you meant in the test file as C comment, right?
Thanks, Gabriel
On 09/01/2020 17:38, Nikolay Sivov wrote:
On Thu, Jan 9, 2020 at 6:23 PM Gabriel Ivăncescu <gabrielopcode(a)gmail.com <mailto:gabrielopcode(a)gmail.com>> wrote:
On 09/01/2020 16:46, Zebediah Figura wrote: > On 1/9/20 7:13 AM, Gabriel Ivăncescu wrote: >> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com <mailto:gabrielopcode(a)gmail.com>> >> --- >> dlls/quartz/tests/filtergraph.c | 35 ++++++++++++++++++++++++++++++++- >> 1 file changed, 34 insertions(+), 1 deletion(-) >> >> diff --git a/dlls/quartz/tests/filtergraph.c >> b/dlls/quartz/tests/filtergraph.c >> index ca45031..726c54a 100644 >> --- a/dlls/quartz/tests/filtergraph.c >> +++ b/dlls/quartz/tests/filtergraph.c >> @@ -4232,16 +4232,38 @@ 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(); >> + 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)) >> { >> @@ -4256,6 +4278,10 @@ static void test_window_threading(void) >> { >> tid = GetWindowThreadProcessId(hwnd, NULL); >> ok(tid != GetCurrentThreadId(), "Window should have been >> created on a separate thread.\n"); >> + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & >> WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); >> + >> + SetParent(hwnd, parent); >> + SetWindowLongW(hwnd, GWL_STYLE, GetWindowLongW(hwnd, >> GWL_STYLE) | WS_CHILD); > > Does the application actually do this, or does it use > IVideoWindow::put_Owner()? If it is the former, I'd like to see a > comment to that effect. >
I actually don't know what the app does when it sets the parent. I wrote the test to see if Windows notifies the parent, and it seems that it doesn't (I also set the WS_CHILD just in case), despite not having the WS_EX_NOPARENTNOTIFY style in the first place. Wine would fail this test without the first patch, even without put_Owner.
I'll investigate what the app does. However, while this patchset fixes that app, I thought a more generic solution would be preferable, since that's how Windows seems to work (unless you have a better idea).
I don't understand how this window is set, just from this test alone. But, is it possible to subclass application window, and simply not forward certain messages?
You can probably even test if it was subclassed when message returns.
You mean subclass the parent? I'll look into that, but it seems a bit overkill. (that said, if Windows does it...) FWIW, the issue is that DestroyWindow notifies the parent if the child doesn't have that style.
On 09/01/2020 17:46, Gabriel Ivăncescu wrote:
On 09/01/2020 17:38, Nikolay Sivov wrote:
On Thu, Jan 9, 2020 at 6:23 PM Gabriel Ivăncescu <gabrielopcode(a)gmail.com <mailto:gabrielopcode(a)gmail.com>> wrote:
On 09/01/2020 16:46, Zebediah Figura wrote: > On 1/9/20 7:13 AM, Gabriel Ivăncescu wrote: >> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com <mailto:gabrielopcode(a)gmail.com>> >> --- >> dlls/quartz/tests/filtergraph.c | 35 ++++++++++++++++++++++++++++++++- >> 1 file changed, 34 insertions(+), 1 deletion(-) >> >> diff --git a/dlls/quartz/tests/filtergraph.c >> b/dlls/quartz/tests/filtergraph.c >> index ca45031..726c54a 100644 >> --- a/dlls/quartz/tests/filtergraph.c >> +++ b/dlls/quartz/tests/filtergraph.c >> @@ -4232,16 +4232,38 @@ 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(); >> + 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)) >> { >> @@ -4256,6 +4278,10 @@ static void test_window_threading(void) >> { >> tid = GetWindowThreadProcessId(hwnd, NULL); >> ok(tid != GetCurrentThreadId(), "Window should have been >> created on a separate thread.\n"); >> + ok(!(GetWindowLongW(hwnd, GWL_EXSTYLE) & >> WS_EX_NOPARENTNOTIFY), "Window has WS_EX_NOPARENTNOTIFY.\n"); >> + >> + SetParent(hwnd, parent); >> + SetWindowLongW(hwnd, GWL_STYLE, GetWindowLongW(hwnd, >> GWL_STYLE) | WS_CHILD); > > Does the application actually do this, or does it use > IVideoWindow::put_Owner()? If it is the former, I'd like to see a > comment to that effect. >
I actually don't know what the app does when it sets the parent. I wrote the test to see if Windows notifies the parent, and it seems that it doesn't (I also set the WS_CHILD just in case), despite not having the WS_EX_NOPARENTNOTIFY style in the first place. Wine would fail this test without the first patch, even without put_Owner.
I'll investigate what the app does. However, while this patchset fixes that app, I thought a more generic solution would be preferable, since that's how Windows seems to work (unless you have a better idea).
I don't understand how this window is set, just from this test alone. But, is it possible to subclass application window, and simply not forward certain messages?
You can probably even test if it was subclassed when message returns.
You mean subclass the parent? I'll look into that, but it seems a bit overkill. (that said, if Windows does it...)
FWIW, the issue is that DestroyWindow notifies the parent if the child doesn't have that style.
Scratch that, the parent doesn't even process messages, that's why it deadlocks in the first place. Subclass wouldn't really help at all...
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
+ 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; }
This is most likely wrong way of fixing this: if it's your own window then it should have correct styles from the start, if it's a foreign window then this requires a test case since it may break things. -- Dmitry.
On 09/01/2020 15:32, Dmitry Timoshkov wrote:
Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote:
+ 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; }
This is most likely wrong way of fixing this: if it's your own window then it should have correct styles from the start, if it's a foreign window then this requires a test case since it may break things.
I did add tests on the second patch in the series for exactly this. It's our own window, but it does seem that on Windows, it does not have the style set. However, it also does *not* notify the parent. So setting it on WM_CLOSE is the only way I could get it to pass the test so it matches Windows.
participants (4)
-
Dmitry Timoshkov -
Gabriel Ivăncescu -
Nikolay Sivov -
Zebediah Figura