Closed cgutman closed 2 years ago
The WGI documentation says specifically to use RO_INIT_MULTITHREADED
, so we should probably change WIN_CoInitialize() instead. Can you try that and create a PR if it looks good?
The WGI documentation says specifically to use RO_INIT_MULTITHREADED, so we should probably change WIN_CoInitialize() instead.
Oh interesting, do you have a link to the page that said that? The comments for each class in windows.gaming.input.h
say "Class Threading Model: Both Single and Multi Threaded" which seems to indicate WGI doesn't care.
Your info does seem to be accurate though because using COINIT_MULTITHREADED
fixes the crash in #5270.
Can you try that and create a PR if it looks good?
I can change WIN_CoInitialize()
to default to COINIT_MULTITHREADED
, but I think that may introduce some nasty issues. It'll be changing the default threading model for most SDL applications, so anyone that uses OLE or other interfaces that need STA will break.
The WGI documentation says specifically to use RO_INIT_MULTITHREADED, so we should probably change WIN_CoInitialize() instead.
Oh interesting, do you have a link to the page that said that? The comments for each class in
windows.gaming.input.h
say "Class Threading Model: Both Single and Multi Threaded" which seems to indicate WGI doesn't care.Your info does seem to be accurate though because using
COINIT_MULTITHREADED
fixes the crash in #5270.
I thought I read it elsewhere as well, but it may have been this: https://github.com/libsdl-org/SDL/issues/5270#issuecomment-1086246703
I can change
WIN_CoInitialize()
to default toCOINIT_MULTITHREADED
, but I think that may introduce some nasty issues. It'll be changing the default threading model for most SDL applications, so anyone that uses OLE or other interfaces that need STA will break.
That's a good point. Maybe we should add a hint for this, and default it to multi-threaded so SDL itself will work correctly? :)
That's a good point. Maybe we should add a hint for this, and default it to multi-threaded so SDL itself will work correctly? :)
Upon further inspection, even SDL itself isn't fully MTA-safe.
Our implementation of SDL_SYS_OpenURL()
requires an STA to be compatible with certain shell extensions per MSDN docs for ShellExecute.
In addition, our IME code requires an STA, otherwise we fail with REGDB_E_IIDNOTREG
on this line: https://github.com/libsdl-org/SDL/blob/49a2e4b0ea0d677aa749c3d478e0966e82e5efa3/src/video/windows/SDL_windowskeyboard.c#L1386
I'm going to poke around with WGI more tomorrow to see if maybe we're leaking a reference to something and that's why we crash when Windows.Gaming.Input.dll unloads. If not, we could be stuck having to do something annoying like running WGI on its own thread with an MTA.
Good catches, thanks!
OK, this rabbit hole goes deep, but I think I got somewhat to the bottom of what's going on in #5270.
(4ae8.33ec): Access violation - code c0000005 (first/second chance not available)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
Time Travel Position: 5C9306:0
Windows_Gaming_Input!GameController::~GameController+0x76:
00007ffe`c38862f6 488b4040 mov rax,qword ptr [rax+40h] ds:00007ffe`c2c81790=????????????????
0:000> k
# Child-SP RetAddr Call Site
00 00000078`6210ee90 00007ffe`c3886254 Windows_Gaming_Input!GameController::~GameController+0x76
01 00000078`6210eec0 00007ffe`c3888a40 Windows_Gaming_Input!GameController::`vector deleting destructor'+0x14
02 00000078`6210eef0 00007ffe`c388a8f0 Windows_Gaming_Input!Microsoft::WRL::Details::RuntimeClassImpl<Microsoft::WRL::RuntimeClassFlags<1>,1,1,0,Windows::Gaming::Input::IGameController,Windows::Gaming::Input::IGameControllerBatteryInfo,Microsoft::WRL::CloakedIid<Windows::Gaming::Input::Internal::IGameControllerPrivate>,Microsoft::WRL::FtmBase>::Release+0x80
03 00000078`6210ef20 00007ffe`c38644da Windows_Gaming_Input!Windows::Gaming::Input::Custom::Details::AggregableRuntimeClass<Windows::Gaming::Input::IGamepad,Windows::Gaming::Input::IGamepad2,Microsoft::WRL::CloakedIid<Windows::Gaming::Input::Custom::IGameControllerInputSink>,Microsoft::WRL::CloakedIid<Windows::Gaming::Input::Custom::IGipGameControllerInputSink>,Microsoft::WRL::CloakedIid<Windows::Gaming::Input::Custom::IHidGameControllerInputSink>,Microsoft::WRL::CloakedIid<Windows::Gaming::Input::Custom::IXusbGameControllerInputSink>,Microsoft::WRL::Details::Nil,Microsoft::WRL::Details::Nil,Microsoft::WRL::Details::Nil>::Release+0x40
04 00000078`6210ef50 00007ffe`c38d9051 Windows_Gaming_Input!Microsoft::WRL::ComPtr<Windows::Foundation::IAsyncOperation<enum Windows::Gaming::Input::ForceFeedback::ForceFeedbackLoadEffectResult> >::~ComPtr<Windows::Foundation::IAsyncOperation<enum Windows::Gaming::Input::ForceFeedback::ForceFeedbackLoadEffectResult> >+0x2a
05 00000078`6210ef80 00007ffe`c3877119 Windows_Gaming_Input!`eh vector destructor iterator'+0x5d
06 00000078`6210efe0 00007ffe`c3877094 Windows_Gaming_Input!Windows::Gaming::Input::Custom::Details::GameControllerCollection<Windows::Gaming::Input::RawGameController,Windows::Gaming::Input::IRawGameController>::~GameControllerCollection<Windows::Gaming::Input::RawGameController,Windows::Gaming::Input::IRawGameController>+0x59
07 00000078`6210f010 00007ffe`c386afb0 Windows_Gaming_Input!Windows::Gaming::Input::Custom::Details::GameControllerCollection<Windows::Gaming::Input::RawGameController,Windows::Gaming::Input::IRawGameController>::`vector deleting destructor'+0x14
08 00000078`6210f040 00007ffe`c387cacb Windows_Gaming_Input!Microsoft::WRL::Details::RuntimeClassImpl<Microsoft::WRL::RuntimeClassFlags<1>,1,1,0,Windows::Foundation::Collections::IIterable<Windows::Gaming::Input::ArcadeStick * __ptr64>,Windows::Foundation::Collections::IVectorView<Windows::Gaming::Input::ArcadeStick * __ptr64>,Microsoft::WRL::FtmBase>::Release+0x80
09 00000078`6210f070 00007ffe`c387ca44 Windows_Gaming_Input!Windows::Gaming::Input::Custom::Details::CustomGameControllerFactoryBase<Windows::Gaming::Input::FlightStick,Windows::Gaming::Input::FlightStick,Windows::Gaming::Input::IFlightStick,Windows::Gaming::Input::IFlightStickStatics,Microsoft::WRL::Details::Nil>::~CustomGameControllerFactoryBase<Windows::Gaming::Input::FlightStick,Windows::Gaming::Input::FlightStick,Windows::Gaming::Input::IFlightStick,Windows::Gaming::Input::IFlightStickStatics,Microsoft::WRL::Details::Nil>+0x5b
0a 00000078`6210f0a0 00007ffe`c386e451 Windows_Gaming_Input!Windows::Gaming::Input::Custom::Details::CustomGameControllerFactoryBase<Windows::Gaming::Input::FlightStick,Windows::Gaming::Input::FlightStick,Windows::Gaming::Input::IFlightStick,Windows::Gaming::Input::IFlightStickStatics,Microsoft::WRL::Details::Nil>::`vector deleting destructor'+0x14
0b 00000078`6210f0d0 00007ffe`c387b771 Windows_Gaming_Input!Microsoft::WRL::ActivationFactory<Microsoft::WRL::Implements<Microsoft::WRL::FtmBase,Microsoft::WRL::CloakedIid<Windows::Gaming::Input::Custom::ICustomGameControllerFactory> >,Windows::Gaming::Input::IFlightStickStatics,Microsoft::WRL::Details::Nil,0>::Release+0x51
0c 00000078`6210f100 00007ffe`c387b55c Windows_Gaming_Input!FactoryManager::FactoryListEntry::~FactoryListEntry+0x79
0d 00000078`6210f130 00007ffe`c3879985 Windows_Gaming_Input!NtList<FactoryManager::FactoryListEntry>::~NtList<FactoryManager::FactoryListEntry>+0x34
0e 00000078`6210f160 00007ffe`c3879944 Windows_Gaming_Input!FactoryManager::~FactoryManager+0x15
0f 00000078`6210f190 00007ffe`c3878481 Windows_Gaming_Input!FactoryManager::`vector deleting destructor'+0x14
10 00000078`6210f1c0 00007ffe`c3878d88 Windows_Gaming_Input!Microsoft::WRL::ActivationFactory<Microsoft::WRL::Implements<Microsoft::WRL::FtmBase,Windows::Gaming::Input::Custom::IGameControllerFactoryManagerStatics>,Windows::Gaming::Input::Custom::IGameControllerFactoryManagerStatics2,Microsoft::WRL::CloakedIid<Windows::Gaming::Input::Internal::IGameControllerFactoryManagerStaticsPrivate>,0>::Release+0x51
11 00000078`6210f1f0 00007ffe`c3872d71 Windows_Gaming_Input!Microsoft::WRL::Details::TerminateMap+0x138
12 00000078`6210f230 00007ffe`c38dbb70 Windows_Gaming_Input!Microsoft::WRL::Details::DefaultModule<1>::`vector deleting destructor'+0x21
13 00000078`6210f260 00007ffe`c38d8999 Windows_Gaming_Input!`dynamic atexit destructor for 'Microsoft::WRL::Details::StaticStorage<Microsoft::WRL::Details::DefaultModule<1>,0,int>::instance_''+0x30
14 00000078`6210f290 00007ffe`c38d8d50 Windows_Gaming_Input!CRT_INIT+0xcd
15 00000078`6210f2d0 00007ffe`d059fd17 Windows_Gaming_Input!__DllMainCRTStartup+0x1dc
16 00000078`6210f440 00007ffe`d05b2a3f ntdll!LdrpCallInitRoutine+0x6b
17 00000078`6210f4b0 00007ffe`d05b242f ntdll!LdrpProcessDetachNode+0x13b
18 00000078`6210f580 00007ffe`d05b23cf ntdll!LdrpUnloadNode+0x3f
19 00000078`6210f5d0 00007ffe`d05981fc ntdll!LdrpDecrementModuleLoadCountEx+0x5b
1a 00000078`6210f600 00007ffe`cdfa9c8e ntdll!LdrUnloadDll+0xcc
1b 00000078`6210f640 00007ffe`cf6e3e0a KERNELBASE!FreeLibrary+0x1e
1c 00000078`6210f670 00007ffe`cf71d3b8 combase!CClassCache::CDllPathEntry::CFinishObject::Finish+0x2a [onecore\com\combase\objact\dllcache.cxx @ 2801]
1d 00000078`6210f6a0 00007ffe`cf6e2b64 combase!CClassCache::CFinishComposite::Finish+0x54 [onecore\com\combase\objact\dllcache.cxx @ 2915]
1e 00000078`6210f6d0 00007ffe`cf6e1cb5 combase!CClassCache::CleanUpDllsForProcess+0x12c [onecore\com\combase\objact\dllcache.cxx @ 5869]
1f (Inline Function) --------`-------- combase!CCCleanUpDllsForProcess+0xe [onecore\com\combase\objact\dllcache.cxx @ 7600]
20 00000078`6210f910 00007ffe`cf71cdd8 combase!ProcessUninitialize+0x1f1 [onecore\com\combase\class\compobj.cxx @ 2181]
21 00000078`6210f940 00007ffe`cf71cf4b combase!DecrementProcessInitializeCount+0x5c [onecore\com\combase\class\compobj.cxx @ 984]
22 00000078`6210f970 00007ffe`cf7316db combase!wCoUninitialize+0xc7 [onecore\com\combase\class\compobj.cxx @ 4057]
23 00000078`6210f9c0 00007ffe`244845aa combase!CoUninitialize+0xeb [onecore\com\combase\class\compobj.cxx @ 3887]
We only have a single apartment in our process, so the final call to CoUninitialize()
on the main thread results in COM being uninitialized for the entire process. As part of that process-wide uninitialization, COM unloads DLLs for all in-proc COM servers. That unload is what is crashing here.
It turns out I was wrong that using an MTA solves the problem. It appears that using an MTA in combination with the partial failure of the IME code (mentioned in my last comment) causes the MTA to stick around beyond our last CoUninitialize()
call. This is probably a bug somewhere in the IME code, but I haven't tracked it down. In any case, since an apartment still exists in the process, the process-wide deinitialization of COM never happens and so no crash. The crash still happens with an MTA if you build SDL with SDL_DISABLE_WINDOWS_IME
defined.
Using time-travel debugging, I was able to figure out that the pointer WGI is trying to read used to belong to the vtable for Windows::System::UserDeviceAssociationImpl
:
Time Travel Position: 5C833F:54
combase!CoUninitialize:
00007ffe`cf7315f0 48895c2408 mov qword ptr [rsp+8],rbx ss:00000078`6210fab0=0000000000000000
0:000> dps 00007ffe`c2c81730 L10
00007ffe`c2c81730 00007ffe`c2c77e30 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::InternalGetRuntimeClassNameStatic
00007ffe`c2c81738 00007ffe`c2c77e20 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::InternalGetTrustLevelStatic
00007ffe`c2c81740 00007ffe`c2c887b8 Windows_System_UserDeviceAssociation!__objectFactory__UserDeviceAssociationImpl
00007ffe`c2c81748 00000000`00000000
00007ffe`c2c81750 00007ffe`c2c76a20 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::QueryInterface
00007ffe`c2c81758 00007ffe`c2c76a60 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::AddRef
00007ffe`c2c81760 00007ffe`c2c768a0 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::Release
00007ffe`c2c81768 00007ffe`c2c76a80 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::GetIids
00007ffe`c2c81770 00007ffe`c2c76780 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::GetRuntimeClassName
00007ffe`c2c81778 00007ffe`c2c769a0 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::GetTrustLevel
00007ffe`c2c81780 00007ffe`c2c72d00 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::FindUserFromDeviceId
00007ffe`c2c81788 00007ffe`c2c72d90 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::add_UserDeviceAssociationChanged
00007ffe`c2c81790 00007ffe`c2c72fe0 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::remove_UserDeviceAssociationChanged
00007ffe`c2c81798 00007ffe`c2c769e0 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::QueryInterface
00007ffe`c2c817a0 00007ffe`c2c76a40 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::AddRef
00007ffe`c2c817a8 00007ffe`c2c76840 Windows_System_UserDeviceAssociation!Windows::System::UserDeviceAssociationImpl::Release
Based on what the old vtable looked like prior to the unload, It appears that Windows_Gaming_Input!GameController::~GameController()
is trying to invoke Windows::System::UserDeviceAssociationImpl::remove_UserDeviceAssociationChanged
to unregister a UserDeviceAssociateChanged
callback, however Windows.System.UserDeviceAssociation.dll
has already been unloaded at this point. This looks eerily similar to The case of the C++/WinRT cached factories that pointed into freed memory except the cause is different.
Windows.System.UserDeviceAssociation.dll
was loaded by Windows.Gaming.Input.dll
in response to our request for Windows.Gaming.Input.RawGameController
. It's something specific to that class in particular, which is why it only triggers when using the WGI joystick driver, rather than RawInput (which also uses WGI under the hood).
WGI is directly loading Windows.System.UserDeviceAssociation.dll
, so I can't see how this is anything we're doing wrong:
ModLoad: 00007ffe`c2c70000 00007ffe`c2c8c000 C:\Windows\System32\Windows.System.UserDeviceAssociation.dll
ntdll!NtMapViewOfSection+0x14:
00007ffe`d0603c54 c3 ret
# Child-SP RetAddr Call Site
0b 00000042`38dff1e0 00007ffe`cf723aed KERNELBASE!LoadLibraryExW+0x172
0c 00000042`38dff250 00007ffe`cf723a30 combase!LoadLibraryWithLogging+0x2d [onecore\com\combase\common\internal\loadfree.cxx @ 160]
0d (Inline Function) --------`-------- combase!CClassCache::CDllPathEntry::LoadDll::__l4::<lambda_eebf69f5a3250f8786ebc7c3378db176>::operator()+0x15 [onecore\com\combase\objact\dllcache.cxx @ 2054]
0e (Inline Function) --------`-------- combase!DoImmediateCallback+0x1e [onecore\com\combase\inc\ImmediateCallback.hpp @ 55]
0f 00000042`38dff290 00007ffe`cf723829 combase!CClassCache::CDllPathEntry::LoadDll+0x50 [onecore\com\combase\objact\dllcache.cxx @ 2054]
10 00000042`38dff2e0 00007ffe`cf7382c0 combase!CClassCache::CDllPathEntry::Create+0x6d [onecore\com\combase\objact\dllcache.cxx @ 1967]
11 (Inline Function) --------`-------- combase!CClassCache::GetOrLoadDllPathEntryForWinrt+0x50a [onecore\com\combase\objact\dllcache.cxx @ 3896]
12 00000042`38dff390 00007ffe`cf7371c8 combase!CClassCache::GetOrLoadWinRTInprocClass+0x620 [onecore\com\combase\objact\dllcache.cxx @ 4027]
13 (Inline Function) --------`-------- combase!CCGetOrLoadWinRTInprocClass+0x1f [onecore\com\combase\objact\dllcache.cxx @ 7055]
14 (Inline Function) --------`-------- combase!WinRTGetActivationFactoryOfInprocClass+0x4e [onecore\com\combase\objact\objact.cxx @ 2235]
15 00000042`38dff500 00007ffe`c3886797 combase!_RoGetActivationFactory+0x478 [onecore\com\combase\winrtbase\winrtbase.cpp @ 963]
16 00000042`38dff920 00007ffe`c387aa73 Windows_Gaming_Input!GameController::RuntimeClassInitialize+0x35f
17 00000042`38dff9b0 00007ffe`c3879e19 Windows_Gaming_Input!FactoryManager::CreateControllerForProvider+0x307
18 00000042`38dffa10 00007ffe`c38bcf13 Windows_Gaming_Input!FactoryManager::OnProviderAdded+0xf9
19 00000042`38dffa50 00007ffe`c38acf38 Windows_Gaming_Input!ProviderManagerWorker::OnProviderAdded+0x7f
1a 00000042`38dffa80 00007ffe`c38bcb60 Windows_Gaming_Input!HidClient::CreateProvider+0x1a0
1b 00000042`38dffb00 00007ffe`c38bb34a Windows_Gaming_Input!ProviderManagerWorker::OnPnpDeviceAdded+0xc8
1c 00000042`38dffb60 00007ffe`c38baa2d Windows_Gaming_Input!PnpDeviceWatcher::StartDeviceObject+0x142
1d 00000042`38dffb90 00007ffe`c38bd1e0 Windows_Gaming_Input!PnpDeviceWatcher::ProcessDeviceNotifications+0x24d
1e 00000042`38dffbd0 00007ffe`c38bd02c Windows_Gaming_Input!ProviderManagerWorker::WorkerThreadProc+0x170
1f 00000042`38dffe60 00007ffe`cfb354e0 Windows_Gaming_Input!ProviderManagerWorker::WorkerThreadProcThunk+0x1c
20 00000042`38dffe90 00007ffe`d056485b KERNEL32!BaseThreadInitThunk+0x10
21 00000042`38dffec0 00000000`00000000 ntdll!RtlUserThreadStart+0x2b
I don't know enough about COM to know whether Windows.Gaming.Input
or Windows.System.UserDeviceAssociation
is at fault for this but it certainly seems to be an OS bug either way.
I'm not sure if there's a better workaround, but I did find that keeping the MTA around (even if nobody is using it) will prevent COM from uninitializing for the entire process and avoid unloading those DLLs. That can be done by calling CoIncrementMTAUsage()
after RoInitialize()
(even if the current thread has a STA).
That's awesome debugging, and we should implement that workaround, but it still doesn't solve the fact that SDL uses multiple COM threading models. Or does this mean we can actually initialize WGI with apartment threaded model?
STA or MTA doesn't seem to make a difference as far as the crash is concerned. We still hit it either way, so I think it's safe to use either once we decide on a workaround for the bug.
Let's go ahead and implement the CoIncrementMTAUsage()
workaround for now. Can you submit a PR that implements that and uses the correct flags for CoInitialize()
and RoInitialize()
so everything works?
@walbourn, @cgutman tracked down the CoUninitialize() crash in WGI, and it looks like it may be unrelated to whether we use RoInitialize(). The details are in https://github.com/libsdl-org/SDL/issues/5552#issuecomment-1103452448.
Let's go ahead and implement the
CoIncrementMTAUsage()
workaround for now. Can you submit a PR that implements that and uses the correct flags forCoInitialize()
andRoInitialize()
so everything works?
Yep, I updated PR #5554 with the workaround.
@walbourn, @cgutman tracked down the CoUninitialize() crash in WGI, and it looks like it may be unrelated to whether we use RoInitialize(). The details are in #5552 (comment).
I made a small reproducer for the bug by just adding a couple WGI calls to the default Win32 application template that Visual Studio generates, so I can definitely confirm SDL isn't involved at all.
If anyone at Microsoft wants to investigate, you can build and run the attached project. You must have a PS4 controller connected to the PC (possibly other non-Xbox gamepads may work) and you must leave the app running for at least around 30 seconds to hit the crash.
While retesting #4915, I discovered that WGI initialization now deterministically fails (since https://github.com/libsdl-org/SDL/commit/8ebef12d31bff35aec6ac659ae7d2d8fcb6ea5b0) unless the application calls
CoInitializeEx(NULL, COINIT_MULTITHREADED)
prior to initializing the joystick stubsystem.Our
RoInitialize()
call fails due to incompatible concurrency model (RPC_E_CHANGED_MODE
) with a previousCoInitializeEx()
call.The first call to
CoInitializeEx()
comes from DirectInput:WIN_CoInitialize()
tries to use an apartment threading model if COM is not already initialized:So we will end up with apartment threading unless the application has already initialized COM outside of SDL.
Next we will try to call
RoInitialize(RO_INIT_MULTITHREADED)
, which is a conflicting concurrency model with apartment threading that we used earlier.This fails with
RPC_E_CHANGED_MODE
and causes us to fail to initialize the WGI joystick driver. It looks likeRO_INIT_SINGLETHREADED
is equivalent toCOINIT_APARTMENTTHREADED
, so we should probably try that likeWIN_CoInitialize()
does.