[PATCH] quartz: Increment reference count for the advise thread to avoid crashes caused by DLL unloading.
Some games crash when the DLL is unloaded during SleepConditionVariableCS on SystemClockAdviseThread. This is due to the fact that DllCanUnloadNow returns S_OK even though the advise thread is running. Incrementing the reference count when creating the advise thread resolves it. Signed-off-by: Hiroki Awata <castaneai(a)by.black> --- dlls/quartz/systemclock.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c index 73a923a7b99..7302de0e58f 100644 --- a/dlls/quartz/systemclock.c +++ b/dlls/quartz/systemclock.c @@ -104,6 +104,7 @@ static ULONG WINAPI system_clock_inner_Release(IUnknown *iface) WakeConditionVariable(&clock->cv); WaitForSingleObject(clock->thread, INFINITE); CloseHandle(clock->thread); + InterlockedDecrement(&object_locks); } LIST_FOR_EACH_ENTRY_SAFE(sink, cursor, &clock->sinks, struct advise_sink, entry) @@ -207,6 +208,8 @@ static HRESULT add_sink(struct system_clock *clock, DWORD_PTR handle, if (!InterlockedCompareExchange(&clock->thread_created, TRUE, FALSE)) { clock->thread = CreateThread(NULL, 0, SystemClockAdviseThread, clock, 0, NULL); + if (clock->thread) + InterlockedIncrement(&object_locks); } WakeConditionVariable(&clock->cv); -- 2.27.0
Hello Hiroki, thanks for the patch! On 8/10/21 5:38 PM, Hiroki Awata wrote:
Some games crash when the DLL is unloaded during SleepConditionVariableCS on SystemClockAdviseThread. This is due to the fact that DllCanUnloadNow returns S_OK even though the advise thread is running. Incrementing the reference count when creating the advise thread resolves it.
This doesn't seem right; the system clock object itself should be holding a reference [added by DSCF_CreateInstance(), removed by system_clock_inner_Release()] and waits for the thread to stop before removing it. Are you sure we're not leaking a reference elsewhere?
Hi, Zebediah. Thank you for your quick reply. It was a very interesting problem and I logged the changes in object_locks. In my tests, object_locks goes to 0 without system_clock_inner_Release being called. The game I tested does not have a free version, so I apologize for not sharing it. The game is clickable to skip video playback, and the crash occurs when clicking. I have attached the log of my test for your reference. Thank you, 2021年8月11日(水) 7:52 Zebediah Figura (she/her) <zfigura(a)codeweavers.com>:
Hello Hiroki, thanks for the patch!
On 8/10/21 5:38 PM, Hiroki Awata wrote:
Some games crash when the DLL is unloaded during SleepConditionVariableCS on SystemClockAdviseThread. This is due to the fact that DllCanUnloadNow returns S_OK even though the advise thread is running. Incrementing the reference count when creating the advise thread resolves it.
This doesn't seem right; the system clock object itself should be holding a reference [added by DSCF_CreateInstance(), removed by system_clock_inner_Release()] and waits for the thread to stop before removing it. Are you sure we're not leaking a reference elsewhere?
On 8/10/21 6:40 PM, 粟田大樹 wrote:
Hi, Zebediah. Thank you for your quick reply.
It was a very interesting problem and I logged the changes in object_locks. In my tests, object_locks goes to 0 without system_clock_inner_Release being called. The game I tested does not have a free version, so I apologize for not sharing it. The game is clickable to skip video playback, and the crash occurs when clicking.
I have attached the log of my test for your reference. Thank you,
From examination of the code I believe the problem is that we call system_clock_create() from dsound_render_create(), which bypasses the increment. Either that should be changed to CoCreateInstance, or we should only increment object_locks from creation functions. I'm kind of leaning toward the latter, even though it's more code, since it's more consistent (along the lines of "always grab and release a resource in the same place"). I note also that enum_reg_filter_create() and enum_moniker_create() don't reference the module, and should.
Hi, Zebediah. Thanks for the detailed research. I've revert the first patch, applied the next one and the test was successful! Is the code fix as intended? If it is ok, I will send a PATCH v2. diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c index 73a923a7b99..0eb339420db 100644 --- a/dlls/quartz/systemclock.c +++ b/dlls/quartz/systemclock.c @@ -338,6 +338,7 @@ HRESULT system_clock_create(IUnknown *outer, IUnknown **out) list_init(&object->sinks); InitializeCriticalSection(&object->cs); object->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": SystemClockImpl.cs"); + InterlockedIncrement(&object_locks); TRACE("Created system clock %p.\n", object); *out = &object->IUnknown_inner; 2021年8月11日(水) 8:56 Zebediah Figura (she/her) <zfigura(a)codeweavers.com>:
On 8/10/21 6:40 PM, 粟田大樹 wrote:
Hi, Zebediah. Thank you for your quick reply.
It was a very interesting problem and I logged the changes in object_locks. In my tests, object_locks goes to 0 without system_clock_inner_Release being called. The game I tested does not have a free version, so I apologize for not sharing it. The game is clickable to skip video playback, and the crash occurs when clicking.
I have attached the log of my test for your reference. Thank you,
From examination of the code I believe the problem is that we call system_clock_create() from dsound_render_create(), which bypasses the increment.
Either that should be changed to CoCreateInstance, or we should only increment object_locks from creation functions. I'm kind of leaning toward the latter, even though it's more code, since it's more consistent (along the lines of "always grab and release a resource in the same place").
I note also that enum_reg_filter_create() and enum_moniker_create() don't reference the module, and should.
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=95434 Your paranoid android. === debiant2 (build log) === error: corrupt patch at line 12 Task: Patch failed to apply === debiant2 (build log) === error: corrupt patch at line 12 Task: Patch failed to apply
On 8/10/21 7:18 PM, 粟田大樹 wrote:
Hi, Zebediah. Thanks for the detailed research. I've revert the first patch, applied the next one and the test was successful! Is the code fix as intended? If it is ok, I will send a PATCH v2.
diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c index 73a923a7b99..0eb339420db 100644 --- a/dlls/quartz/systemclock.c +++ b/dlls/quartz/systemclock.c @@ -338,6 +338,7 @@ HRESULT system_clock_create(IUnknown *outer, IUnknown **out) list_init(&object->sinks); InitializeCriticalSection(&object->cs); object->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": SystemClockImpl.cs"); + InterlockedIncrement(&object_locks);
TRACE("Created system clock %p.\n", object); *out = &object->IUnknown_inner;
Well, it's not quite as simple as that hunk; now you're leaking references for any system clock created via CoCreateInstance(). You'll need to do the same for all other quartz objects, and remove the InterlockedIncrement() from DSCF_CreateInstance().
Oh, I see. I'm not familiar with quartz and I'm not sure I can cover everything. Would you be able to create a patch? 2021年8月11日(水) 10:23 Zebediah Figura (she/her) <zfigura(a)codeweavers.com>:
On 8/10/21 7:18 PM, 粟田大樹 wrote:
Hi, Zebediah. Thanks for the detailed research. I've revert the first patch, applied the next one and the test was successful! Is the code fix as intended? If it is ok, I will send a PATCH v2.
diff --git a/dlls/quartz/systemclock.c b/dlls/quartz/systemclock.c index 73a923a7b99..0eb339420db 100644 --- a/dlls/quartz/systemclock.c +++ b/dlls/quartz/systemclock.c @@ -338,6 +338,7 @@ HRESULT system_clock_create(IUnknown *outer, IUnknown **out) list_init(&object->sinks); InitializeCriticalSection(&object->cs); object->cs.DebugInfo->Spare[0] = (DWORD_PTR)(__FILE__ ": SystemClockImpl.cs"); + InterlockedIncrement(&object_locks);
TRACE("Created system clock %p.\n", object); *out = &object->IUnknown_inner;
Well, it's not quite as simple as that hunk; now you're leaking references for any system clock created via CoCreateInstance(). You'll need to do the same for all other quartz objects, and remove the InterlockedIncrement() from DSCF_CreateInstance().
participants (4)
-
Hiroki Awata -
Marvin -
Zebediah Figura (she/her) -
粟田大樹