[PATCH 2/2] winmm: Use a thread to send driver messages on Win9x (try 2)

[Bruno Jesus]

On Thu, Dec 1, 2016 at 1:14 PM, Andrew Eikum <aeikum at codeweavers.com> wrote:
> On Thu, Dec 01, 2016 at 12:27:43PM -0200, Bruno Jesus wrote:
>> On Thu, Dec 1, 2016 at 12:11 PM, Andrew Eikum <aeikum at codeweavers.com> wrote:
>> > This seems like something you should be able to write a test for. I'm
>> > also curious how this manages to work on Windows >= XP
>>
>> The test is at [1], if you run in Win9x it will fail (Win9x uses
>> different thread), while running at >= XP it will succeed (>=XP uses
>> same thread). This works in XP due to a shim that blindly ignores the
>> SuspendThread/ResumeThread calls when inside a WinMM callback.
>>
>> [1] http://source.winehq.org/source/dlls/winmm/tests/wave.c#0570
>>
>
> See, I told you we could write a test for it. I already did ;)

Sorry for not saying before that the test previously existed.

> I'm still not sure exactly how this shim works. Is that something we
> can replicate in Wine instead of this threaded solution? Requiring
> users to switch the Windows version isn't ideal. Does this game
> require you to set 9x compatibility mode on Windows?

No, manually setting compatibility is not required in XP (that is why
IMO its is a shim, more below). Just the game installer needs
compatibility or it complains about unsupported OS.

I could not find any way to disable SuspendThread, I tried it back in
[1] but got no replies, internet also did not help me at this subject.

[1] https://www.winehq.org/pipermail/wine-devel/2015-November/110308.html

To prove my point at [2] (source at [3]) there is a winmm_crosstest
that calls SuspendThread inside the callback. If you run this at
Windows >= XP it will get stuck. If you turn compatibility mode on it
will pass as if nothing was called (tested on XP and 8).

[2] http://alexa.pro.br/~bruno/wine/winmm_crosstest.exe
[3] http://alexa.pro.br/~bruno/wine/winmm.txt

>> > You need to be more careful about thread shutdown. See
>> > WINMM_StartDevicesThread and WINMM_DevicesThreadProc. I think you
>> > could use the same logic, including WINMM_DevicesThreadDone, to
>> > determine when to quit your new thread.
>>
>> I tried understand what you say but I'm not sure yet.
>> WINMM_DeleteWaveform is only called on process detach, can the DLL
>> live after that?
>>
>
> The issue is that the callback thread may still be running after you
> exit DllMain and the DLL is unloaded on a different thread, which will
> cause a crash.

Makes sense to me.

> Because you can't use the Wait functions during DllMain, you need to
> ensure no winmm code is being run on any thread *before*
> PROCESS_DETACH is called. This means your thread needs to hold a
> reference to the module to ensure it isn't unloaded. However, some
> apps require winmm to be able to unload, so the thread can't just hold
> the reference forever. Since your callback thread will only do useful
> work if devices are opened (right?), you can use
> WINMM_DevicesThreadDone to determine when to quit without depending on
> Wait in DllMain. Finally, FreeLibraryAndExitThread allows you to exit
> winmm code permanently in order to free the reference safely, in case
> your thread is the last reference holder.

Yes, the thread is only useful when devices are opened.

> Note that this all means you'll need to start the callback thread
> dynamically instead of just once. Since your thread and the existing
> devices thread have the same lifespans (right?), I think you can just
> piggyback on WINMM_StartDevicesThread and start both threads in there.

You are right, the lifespans are the same.

> Also, double-check that g_devthread_token is incremented in all
> instances where your callback thread may be used while no devices
> exist. This situation might never occur, but double-check to make
> sure.
>
> I thought we had a bug about this, but I can't find it. See commit
> 2d76befbddc798f02812fe48a12ab2b80d25181b.
>
> Hope that makes sense, it took a while to wrap my head around it when
> I wrote that commit...

Thanks, it all makes sense. I have to study further now.

> Andrew

Thank you very much again.