Prevent deadlock when calling SetWindowPos from another thread #14306

popy2k14 · 2018-08-16T18:14:24Z

flag SWP_ASYNCWINDOWPOS, see https://msdn.microsoft.com/en-us/library/windows/desktop/ms633545%28v=vs.85%29.aspx?f=255&MSPPError=-2147217396

Description

When SetWindowPos is called from another thread with the main gui HWND, kodi freezes!
This was the case on ExternalPlayer.cpp, see ticket: https://trac.kodi.tv/ticket/17988
My PR adds the SWP_ASYNCWINDOWPOS flag on all SetWindowPos.
So the risk of ever running into the same problem, when calling ->Show() is low.

Motivation and Context

Kodi freezes when external player is active and STOP is pressed on keyboard or received, json, remote...

How Has This Been Tested?

On my dev machine, the issue is reproduce able like described in the ticket.
After the PR, issue is gone for me.

Types of change

[X ] Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
[X ] Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
None of the above (please explain below)

Checklist:

[X ] My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
[X ] I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

afedchin

please drop changes that I commented, because Kodi expects old behavior in these places. I think it will not cause an issue, but I don't want to risk at this stage of development v18.

xbmc/windowing/windows/WinSystemWin32.cpp

@@ -401,7 +401,7 @@ void CWinSystemWin32::AdjustWindow(bool forceResize)
    rc.top,
    rc.right - rc.left,
    rc.bottom - rc.top,
-    SWP_SHOWWINDOW | SWP_DRAWFRAME
+    SWP_SHOWWINDOW | SWP_DRAWFRAME | SWP_ASYNCWINDOWPOS


xbmc/windowing/windows/WinSystemWin32.cpp

@@ -586,7 +586,7 @@ bool CWinSystemWin32::DPIChanged(WORD dpi, RECT windowRect) const
    resizeRect.top,
    resizeRect.right - resizeRect.left,
    resizeRect.bottom - resizeRect.top,
-    SWP_NOZORDER | SWP_NOACTIVATE);
+    SWP_NOZORDER | SWP_NOACTIVATE | SWP_ASYNCWINDOWPOS);


xbmc/windowing/windows/WinSystemWin32.cpp

@@ -1122,7 +1122,7 @@ void CWinSystemWin32::NotifyAppFocusChange(bool bGaining)
    ReleaseBackBuffer();

    if (bGaining)
-      SetWindowPos(m_hWnd, HWND_TOP, 0, 0, 0, 0, SWP_NOMOVE | SWP_NOSIZE | SWP_NOACTIVATE | SWP_NOREDRAW);
+      SetWindowPos(m_hWnd, HWND_TOP, 0, 0, 0, 0, SWP_NOMOVE | SWP_NOSIZE | SWP_NOACTIVATE | SWP_NOREDRAW | SWP_ASYNCWINDOWPOS);


xbmc/windowing/windows/WinSystemWin32.cpp

@@ -1132,7 +1132,7 @@ void CWinSystemWin32::NotifyAppFocusChange(bool bGaining)
    SetDeviceFullScreen(bGaining, res);

    if (!bGaining)
-      SetWindowPos(m_hWnd, HWND_NOTOPMOST, 0, 0, 0, 0, SWP_NOMOVE | SWP_NOSIZE | SWP_NOACTIVATE | SWP_NOREDRAW);
+      SetWindowPos(m_hWnd, HWND_NOTOPMOST, 0, 0, 0, 0, SWP_NOMOVE | SWP_NOSIZE | SWP_NOACTIVATE | SWP_NOREDRAW | SWP_ASYNCWINDOWPOS);


afedchin · 2018-08-17T09:24:29Z

after some thoughts I concluded that you fixed consequence but not a root reason. CExternalPlayer shouldn't change window not from main thread. So in my opinion acceptable solution is sending a message/event to the main thread to change window. @FernetMenta what do you think?

FernetMenta · 2018-08-17T09:47:39Z

Right, SetWindowPos is Windows API function and must only be called by windowing component.

So in my opinion acceptable solution is sending a message/event to the main thread to change window

I am not sure if this is Windows only or something generic.

popy2k14 · 2018-08-17T14:49:13Z

You are right but if anybody uses SetWindowPos from another thread again!?
This could happen also when this issue is solved with an event to the main thread.

FernetMenta · 2018-08-17T15:07:25Z

You are right but if anybody uses SetWindowPos from another thread again!?

won't happen if g_hWnd gets eliminated.

afedchin · 2018-08-17T15:17:29Z

You are right but if anybody uses SetWindowPos from another thread again!?

nobody except windowing shouldn't use SetWindowPos. Using SetWindowPos from ExternalPlayer is bad. Fernet is right g_hWnd must be eliminated.

This could happen also when this issue is solved with an event to the main thread.

application events are processed from main thread only.

popy2k14 · 2018-08-17T19:24:35Z

@afedchin @FernetMenta when ill do the changes to the PR and leave the async flag in exernal player (which is a quick fix), do you merge it?

We than can create an ticket for removing the g_hWnd variable.

What do you think?

FernetMenta · 2018-08-18T06:14:15Z

I never do such quick hacks but I leave the decision to @afedchin

… (flag SWP_ASYNCWINDOWPOS)

popy2k14 · 2018-08-18T11:18:37Z

I'll removed the flag from other code playces and just leave it there which belongs to ExternalPlayer.
@afedchin Please, whats your decision? Would be nice if you merge this because some of use using ExternalPlayer currently :-)

We can make a ticket which describes the issue and thats the best way is to eleminate the g_hWnd variable.

Thanks

fritsch · 2018-08-18T11:54:38Z

If it is not fixed properly now, nobody will care afterwards. We have enough tickets open already. popy2k14 <notifications@github.com> schrieb am Sa., 18. Aug. 2018, 13:18:

…

I'll removed the flag from other code playces and just leave it there which belongs to ExternalPlayer. @afedchin <https://github.com/afedchin> Please, whats your decision? Would be nice if you merge this because some of use using ExternalPlayer currently :-) We can make a ticket which describes the issue and thats the best way is to eleminate the g_hWnd variable. Thanks — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#14306 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABCfHamF2CRkGGNyqPGF5idm2LgOUmBeks5uR_gRgaJpZM4WAVvl> .

popy2k14 · 2018-08-18T12:13:39Z

Yeah. but eliminating the variable will not be that easy.
I'ts used in several other components where windows api is needing an hwnd.
Here are the references where they are used:

\xbmc\xbmc\cores\AudioEngine\Sinks\AESinkDirectSound.cpp(163): HWND tmp_hWnd = g_hWnd == nullptr ? GetDesktopWindow() : g_hWnd;
\xbmc\xbmc\cores\ExternalPlayer\ExternalPlayer.cpp(349): SetForegroundWindow(g_hWnd);
\xbmc\xbmc\network\mdns\ZeroconfBrowserMDNS.cpp(27):extern HWND g_hWnd;
\xbmc\xbmc\network\mdns\ZeroconfMDNS.cpp(204): WSAAsyncSelect( (SOCKET) DNSServiceRefSockFD( m_service ), g_hWnd, BONJOUR_EVENT, 0 );
\xbmc\xbmc\platform\win32\WIN32Util.cpp(254): ShowWindow(g_hWnd,SW_MINIMIZE);
\xbmc\xbmc\platform\win32\WindowHelper.cpp(49): SetForegroundWindow(g_hWnd);
\xbmc\xbmc\powermanagement\PowerManager.cpp(202): SetForegroundWindow(g_hWnd);
\xbmc\xbmc\windowing\windows\WinEventsWin32.cpp(263): g_hWnd = nullptr;

As an example in the ZeroconfBrowserMDNS.cpp the WSAAsyncSelect needs an hwnd where the event gets passed after select is ok, so there the correct hwnd is needed for this to work.
Did'nt look a the other files, but i assume the same.
So elliminating the global hwnd is not the right way.

afedchin · 2018-08-18T14:09:18Z

Please, whats your decision? Would be nice if you merge this because some of use using ExternalPlayer currently :-)

If current fix resolves the issue, I can aprove it.

So elliminating the global hwnd is not the right way.

It's right way, you just doesn't see how to resolve it.

popy2k14 · 2018-08-18T15:54:24Z

The current fix resolves the main issue, please aprove it.
Should ill create a ticket regarding the global hwnd?

afedchin · 2018-08-20T05:38:28Z

Should ill create a ticket regarding the global hwnd?

no need. I've added this to my todo list

afedchin · 2018-08-20T05:39:02Z

thanks. merged

popy2k14 · 2018-08-20T06:19:32Z

Thanks!

Rechi requested a review from afedchin August 16, 2018 18:17

afedchin requested changes Aug 17, 2018

View reviewed changes

Prevent deadlock when calling SetWindowPos from ExternalPlayer thread…

8336dfb

… (flag SWP_ASYNCWINDOWPOS)

popy2k14 force-pushed the SetWindowPosAsync branch from 38bc8bb to 8336dfb Compare August 18, 2018 11:16

afedchin approved these changes Aug 20, 2018

View reviewed changes

afedchin merged commit 50d4a65 into xbmc:master Aug 20, 2018

Rechi added Type: Fix non-breaking change which fixes an issue Platform: Windows v18 Leia labels Aug 20, 2018

MartijnKaijser added this to the Leia 18.0-beta1 milestone Aug 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent deadlock when calling SetWindowPos from another thread #14306

Prevent deadlock when calling SetWindowPos from another thread #14306

popy2k14 commented Aug 16, 2018

afedchin left a comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

afedchin commented Aug 17, 2018

FernetMenta commented Aug 17, 2018

popy2k14 commented Aug 17, 2018

FernetMenta commented Aug 17, 2018

afedchin commented Aug 17, 2018

popy2k14 commented Aug 17, 2018

FernetMenta commented Aug 18, 2018

popy2k14 commented Aug 18, 2018

fritsch commented Aug 18, 2018 via email

popy2k14 commented Aug 18, 2018

afedchin commented Aug 18, 2018

popy2k14 commented Aug 18, 2018

afedchin commented Aug 20, 2018

afedchin commented Aug 20, 2018

popy2k14 commented Aug 20, 2018

Prevent deadlock when calling SetWindowPos from another thread #14306

Prevent deadlock when calling SetWindowPos from another thread #14306

Conversation

popy2k14 commented Aug 16, 2018

Description

Motivation and Context

How Has This Been Tested?

Types of change

Checklist:

afedchin left a comment

Choose a reason for hiding this comment

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

afedchin commented Aug 17, 2018

FernetMenta commented Aug 17, 2018

popy2k14 commented Aug 17, 2018

FernetMenta commented Aug 17, 2018

afedchin commented Aug 17, 2018

popy2k14 commented Aug 17, 2018

FernetMenta commented Aug 18, 2018

popy2k14 commented Aug 18, 2018

fritsch commented Aug 18, 2018 via email

popy2k14 commented Aug 18, 2018

afedchin commented Aug 18, 2018

popy2k14 commented Aug 18, 2018

afedchin commented Aug 20, 2018

afedchin commented Aug 20, 2018

popy2k14 commented Aug 20, 2018