[v5 PATCH 1/3] dwrite: Protect cached fontface list when accessed from multiple threads

[Nikolay Sivov]

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 at gmail.com> wrote:
>>> On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov <nsivov at codeweavers.com> wrote:
>>>> Signed-off-by: Nikolay Sivov <nsivov at 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.

> 
>