Re: [v4 PATCH 1/2] dwrite: Protect cached fontface list when accessed from multiple threads

[Sebastian Lackner]

On 10.08.2017 15:41, Nikolay Sivov wrote:
> @@ -861,6 +877,10 @@ HRESULT factory_get_cached_fontface(IDWriteFactory5 *iface, IDWriteFontFile * co
>          const void *cached_key;
>          IDWriteFontFile *file;
>
> +        if (IDWriteFontFace4_AddRef(cached->fontface) == 1)
> +            continue;
> +        IDWriteFontFace4_Release(cached->fontface);

This will not work as expected. The refcount also matters when the object is
about to be destroyed. There could be multiple threads running
factory_get_cached_fontface() before _Release() can finally remove the fontface
from the list. Please note that always calling _Release() also wouldn't be
correct because it would run the destructor twice.

It is not really necessary to access the private structs from other parts of
dwrite, but you should either:

* change AddRef to use something like interlocked_dec_if_nonzero (see ntdll/sync.c)

* implement the check in QueryInterface (manually run InterlockedDecrement to
decrement the refcount without going through the destructor)

There are probably also other ways, for example using a flag to make sure the
destructor is not called twice.

Best regards,
Sebastian