On 17.08.2017 10:40, Sebastian Lackner 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?
Yes, it's possible there's some MRU list as well. I'm almost sure factory does not keep a reference, at least not in a way IUnknown exposes it, I'll need to check if we have tests for that, and maybe add some.