[PATCH 0/1] MR5023: quartz: Release critical section before IVideoWindow_put_Visible
The IVideoWindow_put_Visible runs the NtUserShowWindow which may trigger the VideoWindow_NotifyOwnerMessage with a WM_PAINT message and afterwards try to send a WM_ERASEBKND. This will cause the VideoWindow_NotifyOwnerMessage to be stuck waiting on the lock. This causes a deadlock because the IVideoWindow_put_Visible is never able to return, as it is waiting for the WM_ERASEBKND message to go through in the send_erase function, which can only go through once the WM_PAINT has returned. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5023
From: Christopher Liebich Kolle <cl.kolle(a)stud.uis.no> The IVideoWindow_put_Visible runs the NtUserShowWindow which may trigger the VideoWindow_NotifyOwnerMessage with a WM_PAINT message and afterwards try to send a WM_ERASEBKND. This will cause the VideoWindow_NotifyOwnerMessage to be stuck waiting on the lock. This causes a deadlock because the IVideoWindow_put_Visible is never able to return, as it is waiting for the WM_ERASEBKND message to go through in the send_erase function, which can only go through once the WM_PAINT has returned. --- dlls/quartz/filtergraph.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dlls/quartz/filtergraph.c b/dlls/quartz/filtergraph.c index 4100e71be3a..488b7716d10 100644 --- a/dlls/quartz/filtergraph.c +++ b/dlls/quartz/filtergraph.c @@ -4176,11 +4176,12 @@ static HRESULT WINAPI VideoWindow_put_Visible(IVideoWindow *iface, LONG Visible) hr = GetTargetInterface(This, &IID_IVideoWindow, (LPVOID*)&pVideoWindow); + LeaveCriticalSection(&This->cs); + + if (hr == S_OK) hr = IVideoWindow_put_Visible(pVideoWindow, Visible); - LeaveCriticalSection(&This->cs); - return hr; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5023
Is this the correct solution? Have you tested this? This sounds like this is for bug 51140. I suspect the right solution is (instead? additionally?) to not send WM_ERASEBKGND when a child is hidden; see my comments there. Of course that also needs tests, and it needs bug 6006 to be retested and/or re-debugged. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5023#note_60053
Yes I have tested it, and I was not aware of bug 51140 when making this patch. I was working on fixing an issue in a game called [ISLAND](https://github.com/ValveSoftware/Proton/issues/452) in proton. I tested the game on Windows 10, and with message spy++ I could see that indeed WM_ERASEBKGDN was sending when ending the video. Here is the relavant logs from spy++, it was sending WM_PAINT during the video and when I clicked stop it sent WM_SHOWWINDOW ``` <004863> 0014041A P WM_PAINT hdc:00000000 <004864> 0014041A P WM_PAINT hdc:00000000 <004865> 0014041A P WM_PAINT hdc:00000000 <004866> 0014041A P WM_PAINT hdc:00000000 <004867> 0014041A S WM_SHOWWINDOW fShow:False fuStatus:0 (ShowWindow was called) <004868> 0014041A R WM_SHOWWINDOW <004869> 0014041A S WM_WINDOWPOSCHANGING lpwp:15E9FEAC <004870> 0014041A R WM_WINDOWPOSCHANGING <004871> 00170F76 P WM_PAINT hdc:00000000 <004872> 0014041A S WM_WINDOWPOSCHANGED lpwp:15E9FEAC <004873> 00170F76 S WM_ERASEBKGND hdc:29012B2D <004874> 0014041A R WM_WINDOWPOSCHANGED <004875> 00170F76 R WM_ERASEBKGND fErased:False ``` Here you can see the position is clearly changing and both WM_PAINT and WM_ERASEBKGND is being sent This is very similar to my logs when ending the window in wine [log](https://gist.github.com/CKolle/2b44d8a0f8626cecd3e591d8f8597857) but here I get a deadlock. Leading me to conclude that VideoWindow_put_Visible most likely doesn't hold the lock when doing the ShowWindow. Quick note, I had to enable "Aditonal Windows -> All WIndows in System" to see the WM_SHOWWINDOW in spy++. Also in my wine log there is some TRACE for when I was debugging. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5023#note_60126
This is how I interpert the spy++ log taken on windows 10 while capturing the game ISLAND. I can see that `0014041A` refers to a window called `ActiveMovie Window VideoRenderer` This almost certainly is the directshow/quartz, and we can see it sends and recieves a WM_SHOWWINDOW, like I see in my wine logs. Further we have the parent window `00170F76` named `CS_VideoWidnow` in spy++ and in wine. This window Posts a WM_PAINT, which most likely was forwarded from NotifyOwnerMessage, as that is what we see in wine. It looks to be trying to forward a WM_PAINT to its parent window, but doesn't have the lock as put_Visible hasn't returned. Later we see a WM_ERASEBKGND get sent to the parent window `00170F76`. Same happens in wine it tries to do send_inter_thread_message, but this is wher it stop. Because WM_PAINT hasn't gone through as it is waiting on the lock. Since windows doesn't stop when the WM_ERASEBKGND is sent and I see a WM_PAINT got posted, then I must conclude that VideoWindow_put_Visible must not be holding the lock while doing the ShowWindow on the windows side. As the windows im plementation of ShowWindow is most likely not finished before `WM_WINDOWPOSCHANGED`. But you also has the possiblity of some of these actions being async or WM_ERASEBKGND is sent with send_notify_message? Or the entire ShowWindow is instead ShowWindowAsync. Both of which I think is probally wrong. If there are any questions feel free to ask. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5023#note_60342
participants (3)
-
Christopher Liebich Kolle -
CKolle (＠CKolle)
-
Zebediah Figura (＠zfigura)