Even if one asserts that Silverlight is worth supporting in any capacity (which
I am not for the moment taking any position on), Pipelight is unusable without
downgrading Firefox. As Erich put it to me, "the point of Pipelight was to not
have to run the entire browser in Wine," but now a user would have to go to at
least such an extent anyway.
And, of course, nothing here is useful outside of Pipelight, or if it is, we
want to explicitly know about it so that we can document and properly support
those knobs.
This patch set is currently broken, as the RtlEnterCriticalSection()
implementation has changed without corresponding changes to the inline version.
More broadly, it is very surprising, and never really substantiated, that
inlining helps significantly. There is no record of why the patches were
written, but my guess is that they were written in an attempt to optimize heap
allocation, and my further guess is that targeting critical sections in
particular was motivated by perf traces.
Besides the fact that perf traces are unreliable on the best days, and that
anything that spins or uses atomics like our CS implementation is going to be
overrepresented especially relative to the practical impact, the heap
implementation was optimized for cases that matter with the introduction of the
LFH, and crucially, the LFH is lock free.
As for threadpools, I suspect that Sebastian took note of them as the only other
user of locking in ntdll, and that the inline version was used there because
there was no real reason not to.
At the end of the day, these patches are incorrect, probably help nothing, and
even if we did find they helped something we'd want to do a lot more
investigation and probably solve the problem a different way. Remove them.
This has sat here for a long time pending careful review, because the logic is
not easy to follow. Fortunately I think I understand it now.
The described race is pretty much accurate. When a thread called
RtlExitUserThread() in 1.7.38, it first decremented nb_threads. If it was the
last thread, it would call exit() with the thread exit status; if not, it would
mask off signals [the order here is important] and then call pthread_exit() with
the status.
When a thread called RtlExitUserProcess(), this happened:
* The caller does a terminate_process() request to the server, which sends
SIGQUIT to every thread *but* the caller.
* The SIGQUIT handler calls terminate_thread() with a zero status.
terminate_thread() masks off signals, then decrements nb_threads. If the
aborting thread is the last thread, it would call _exit(), otherwise, it'd
again just pthread_exit().
* Finally, the original thread would call exit(), with the intended status
code.
[All of the intermediate function calls and helpers are skipped for brevity and
clarity].
The problem happens if both of these happen at the same time in different
threads. In this case the RtlExitUserThread() thread could decrement nb_threads,
then get interrupted by SIGQUIT and decrement nb_threads again. The end result
is that, instead of the RtlExitUserProcess() thread exiting with the intended
status, instead one of the SIGQUIT threads will be the "last" thread, and exit
with the status that SIGQUIT uses, which is zero as described.
A more serious race than this can be constructed if a thread is terminated by
another thread while already exiting. In this case nb_threads would be executed
twice, but the consequence would be that the *penultimate* thread to exit,
later, would end up killing the process, since it thinks it's the ultimate
thread.
2334f4e64582a518e4d5a7627472a0d817b147ef changed this. Now a thread calling
RtlExitUserThread() does not decrement nb_threads, but instead asks the server
if it's the last thread, and if so exit the whole process [at the time via
exit(); later via RtlExitUserProcess().] If not the last thread, the threads
mask off signals and then call pthread_exit() as before.
This avoided the race, but added a different one, essentially the opposite
problem: if two threads exit cleanly at the same time, neither one of them will
think they're the last thread, then both will exit without calling exit().
Apparently (from IRC logs) this would leave the thread in a weird state where
it'd still be running somehow, although it's not really clear how.
In any case, this problem was fixed by fac1aabbef3753afc53a4ea4f933b3d0516fd302
upstream. Now if two threads call NtTerminateThread() on themselves at the same
time, they really will exit cleanly and one will terminate the process.
Critically, this is now safe from the original race, because decrementing
nb_threads is done after masking off signals.
Erich can't remember the purpose of these, and suspects that it was incidental
to the other patches for TransmitFile (which used to be in the same Wine-Staging
patch set); those were for WineHQ bug 5048.
Moreover, these aren't correct, and a correct implementation would take a lot of
work, and wouldn't really be able to use anything from these patches as a
reference.
Hence, they're not providing any value to anyone, so remove them.
