On Thu, Aug 17, 2017 at 12:40 AM, Sebastian Lackner sebastian@fds-team.de wrote:
On 17.08.2017 07:11, Anton Romanov wrote:
On Wed, Aug 16, 2017 at 10:05 PM, Anton Romanov theli.ua@gmail.com wrote:
On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov nsivov@codeweavers.com wrote:
Signed-off-by: Nikolay Sivov nsivov@codeweavers.com
dlls/dwrite/dwrite_private.h | 15 ++++++++--- dlls/dwrite/font.c | 30 +++++++++++++-------- dlls/dwrite/main.c | 63 +++++++++++++++++++++++++++++++------------- 3 files changed, 74 insertions(+), 34 deletions(-)
Since apps seem to freely use fontface interfaces from multiple threads - Isn't the following race possible?
+----------------+------------------------+-------------------------+ | T1 | T2 | T3 | +----------------+------------------------+-------------------------+ | Release | | | | Decrement | | | | == 0 -> True | | | | | Query_Interface/AddRef | | | | Increment | |
After the last Release call the app should not be calling any function on the object directly.
The only valid code path to obtain a new reference is going through factory_get_cached_fontface. The code in this function is written in such a way that it only uses very specific safe methods. Most importantly, destroyed font faces are skipped, so for properly written apps no race conditions are possible.
| | | Release | | | | Decrement | | free(cached) | | | | factory_unlock | | | | | | use after free (cached) | +----------------+------------------------+-------------------------+
Actually, with this version of the patch looks like this is exactly the crash I'm consistently getting now (was fine with v1):
0093:trace:dwrite:dwritefontface_Release (0x97936c8)->(1) 0093:trace:dwrite:dwritefontface_Release (0x97936c8)->(0) 004c:trace:dwrite:dwritefontface_GetFiles (0x97936c8)->(0x339d58 0x339d50) 004c:trace:dwrite:dwritefontface_GetIndex (0x97936c8) 004c:trace:dwrite:dwritefontface_TryGetFontTable (0x97936c8)->("GSUB" 0x339e60 0x339e68 0x339e64 0x339e5c)
It is interesting that there is no "returning cached fontface" in the log above. This suggests that your interpretation of the error is wrong, the app basically operates on a destroyed object (or there is an error somewhere else).
Does this really work reliable on Windows? If yes it means the current idea of thread safety is not really sufficient. Maybe Windows preserves cache entries for some specific time (or for the duration of the factory), even after the last reference is gone? Or maybe the factory has a reference on each font face?
Sorry, I did misread the patch a bit. I'll take a closer look after I have some sleep. Here is the full lifetime of that fontface grepped from the log: https://paste.ee/p/3No74 ( egrep "(0x97936c8|warn)" ~/Desktop/Magic_Crash.log ) And at the very list I cannot reproduce this crash on either windows or wine with v1 of this patch.