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

[Anton Romanov]

On Thu, Aug 17, 2017 at 11:14 AM, Nikolay Sivov <bunglehead at gmail.com> wrote:
> 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(-)


So I dived a bit deeper into that and it looks like there is now a
race for fontfile:

004c:trace:dwrite:dwritefontface_Release (0x189c09a0)->(0)
005c:trace:dwrite:dwritefontface_GetSimulations (0x189c09a0)
005c:trace:dwrite:dwritefontface_GetFiles (0x189c09a0)->(0xa5fe400 0xa5fe408)
005c:trace:dwrite:dwritefontfile_AddRef (0x15cf7f00)->(3)
005c:trace:dwrite:dwritefontfile_GetReferenceKey
(0x15cf7f00)->(0xa5fe404, 0xa5fe3fc)
005c:trace:dwrite:dwritefontfile_Release (0x15cf7f00)->(2)
005c:trace:dwrite:dwritefontface_QueryInterface
(0x189c09a0)->({27f2a904-4eb8-441d-9678-0563f53e3e2f} 0xa5fe57c)
005c:warn:dwrite:factory_get_cached_fontface Failed to get
{27f2a904-4eb8-441d-9678-0563f53e3e2f} from fontface, hr 0x80004005.
005c:trace:dwrite:factory_get_cached_fontface returning cached
fontface 0x189c09a0
005c:trace:dwrite:shareddwritefactory_AddRef (0x3a0cf18)
004c:trace:dwrite_file:localfontfilestream_Release (0x1633b128)->(0)
005c:trace:dwrite:dwritefontfile_GetLoader (0x15cf7f00)->(0xa5fe408)
005c:trace:dwrite:localfontfileloader_AddRef (0x15cf5108)->(225)
005c:trace:dwrite:dwritefontfile_GetReferenceKey
(0x15cf7f00)->(0xa5fe410, 0xa5fe40c)
005c:trace:dwrite:localfontfileloader_CreateStreamFromKey
(0x15cf5108)->(0x15cf8fc0, 64, 0x1633aff8)
005c:trace:dwrite:localfontfileloader_CreateStreamFromKey name:
L"C:\\windows\\fonts\\Ariali.TTF"
005c:trace:dwrite_file:localfontfilestream_AddRef (0x1633b128)->(1)
005c:trace:dwrite:localfontfileloader_Release (0x15cf5108)->(224)
005c:trace:dwrite:get_stream_from_file 0x15cf7f00 -> 0x1633b128
005c:trace:dwrite:dwritefontfile_AddRef (0x15cf7f00)->(3)
005c:trace:dwrite_file:localfontfilestream_ReadFileFragment
(0x1633b128)->(0xa5fe318, 0x0, 0xc, 0xa5fe31c)

The crash is in thread 5c with backtrace being:
=>0 0x7d442918 opentype_get_font_table+0xa8(stream_desc=0xa5fe410,
tag=0x322f534f, table_data=0xa5fe39c, table_context=0xa5fe38c,
table_size=0x0(nil), found=0x0(nil))
[/home/theli/Projects/wine-build/32/dlls/dwrite/../../../../wine/dlls/dwrite/opentype.c:1075]
in dwrite (0x0a5fe338)
  1 0x7d442f0e opentype_get_font_metrics+0x5d(stream_desc=<is not
available>, metrics=<is not available>, caret=<is not available>)
[/home/theli/Projects/wine-build/32/dlls/dwrite/../../../../wine/dlls/dwrite/opentype.c:1209]
in dwrite (0x0a5fe3b8)
  2 0x7d42a089 create_fontface+0x2c8(desc=0xa5fe470,
cached_list=0x3a0cf34, ret=0xa5fe57c)
[/home/theli/Projects/wine-build/32/dlls/dwrite/../../../../wine/dlls/dwrite/font.c:4354]
in dwrite (0x0a5fe438)
  3 0x7d42a396 get_fontface_from_font.isra+0xb5() in dwrite (0x0a5fe4a8)
  4 0x7d42a420 dwritefont3_CreateFontFace+0x5f(iface=<couldn't compute
location>, fontface=<couldn't compute location>)
[/home/theli/Projects/wine-build/32/dlls/dwrite/../../../../wine/dlls/dwrite/font.c:1674]
in dwrite (0x0a5fe4d8)
  5 0x7d41d42b dwritefont_CreateFontFace+0x5a(iface=<couldn't compute
location>, fontface=<couldn't compute location>)
[/home/theli/Projects/wine-build/32/dlls/dwrite/../../include/dwrite_3.h:1208]
in dwrite (0x0a5fe518)


And the reason it didn't crash with v1 is that the whole chain of
releases (from fontface to factory, file, etc) was protected by lock.
While this version releases the lock after removing fontface from the
cache.

Also with this patch the line
TRACE("returning cached fontface %p\n", cached->fontface);
Should probably be under the else branch as it outputs this regardless
of whether it actually acquired a reference.