August 2017 - Wine-Devel - list.winehq.org

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

by Anton Romanov

On Thu, Aug 17, 2017 at 9:35 PM, Anton Romanov <theli.ua(a)gmail.com> wrote: > 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. Sorry, this is wrong. It actually should've crashed even with v1 (and now that I think of it I think I did get couple of crashes in this codepath even with v1). So I guess with this patch for w/e reason timing have changed and that different race became more likely. And this is fine, but fontfile should be protected as well.

Aug. 18, 2017

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

by Anton Romanov

On Thu, Aug 17, 2017 at 11:14 AM, Nikolay Sivov <bunglehead(a)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(a)gmail.com> wrote: >>>> On Tue, Aug 15, 2017 at 4:40 AM, Nikolay Sivov <nsivov(a)codeweavers.com> wrote: >>>>> Signed-off-by: Nikolay Sivov <nsivov(a)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.

Aug. 18, 2017

Re: JACK driver for WINE

by Austin English

On Thu, Aug 17, 2017 at 2:14 PM, Jeremy Banks <korethr(a)gmail.com> wrote: > Hello all, > > I am wanting to output the audio from various applications run in WINE to > the JACK server on my computer. I would prefer that WINE do this natively > instead of routing across various wrapper setups with ALSA or PulseAudio. I > have looked on the WINE wiki page for sound[1] and see that there was a > JACK driver at one point, but it was removed when the driver architecture > was changed. > > I am interested in trying to implement a driver to get WINE talking to JACK > natively. The wiki says to ask on the mailing list before starting such a > project, and so here I am. I have downloaded a local copy of the source repo > and have started browsing through its contents. Being unfamiliar with the > WINE codebase, which portions of the source should I start studying first? I > am interested in studying the drivers for ALSA, OSS, and Pulse for > comparison; do those live in a particular place in the source tree? I'm > presently looking on the Wiki for other helpful information, but is there > some particular documentation I should be studying for this endeavor? Hi Jeremy, You're looking for https://source.winehq.org/git/wine.git/tree/HEAD:/dlls/winealsa.drv https://source.winehq.org/git/wine.git/tree/HEAD:/dlls/wineandroid.drv https://source.winehq.org/git/wine.git/tree/HEAD:/dlls/winecoreaudio.drv https://source.winehq.org/git/wine.git/tree/HEAD:/dlls/wineoss.drv https://source.winehq.org/git/wine.git/tree/HEAD:/dlls/winepulse.drv note that a lot of the sound code is in mmdevapi: https://source.winehq.org/git/wine.git/tree/HEAD:/dlls/mmdevapi Andrew Eikum maintains wine's sound infrastructure, he's the guy you want if you have more involved questions. Good luck! -- -Austin GPG: 14FB D7EA A041 937B

Aug. 17, 2017

Re: [PATCH] comctl32/listview: Keep the active item selected after hwndEdit is destroyed

by Nikolay Sivov

On 10.08.2017 7:54, Hugh McMaster wrote: > Signed-off-by: Hugh McMaster <hugh.mcmaster(a)outlook.com> > --- > dlls/comctl32/listview.c | 14 ++++++++++++-- > 1 file changed, 12 insertions(+), 2 deletions(-) > > diff --git a/dlls/comctl32/listview.c b/dlls/comctl32/listview.c > index 73eceff509..05bd375571 100644 > --- a/dlls/comctl32/listview.c > +++ b/dlls/comctl32/listview.c > @@ -10216,6 +10216,7 @@ static LRESULT LISTVIEW_LButtonDown(LISTVIEW_INFO *infoPtr, WORD wKey, INT x, IN > static BOOL bGroupSelect = TRUE; > POINT pt = { x, y }; > INT nItem; > + BOOL editing = FALSE; > > TRACE("(key=%hu, X=%u, Y=%u)\n", wKey, x, y); > > @@ -10309,11 +10310,20 @@ static LRESULT LISTVIEW_LButtonDown(LISTVIEW_INFO *infoPtr, WORD wKey, INT x, IN > else > { > if (!infoPtr->bFocus) > + { > + editing = IsWindow(infoPtr->hwndEdit); > SetFocus(infoPtr->hwndSelf); > + } > > - /* remove all selections */ > + /* keep the active item selected after hwndEdit is destroyed, or deselect all items */ > if (!(wKey & MK_CONTROL) && !(wKey & MK_SHIFT)) > - LISTVIEW_DeselectAll(infoPtr); > + { > + if (editing) > + LISTVIEW_DeselectAllSkipItem(infoPtr, infoPtr->nFocusedItem); > + else > + LISTVIEW_DeselectAll(infoPtr); > + } > + Maybe this could be reduced to a single unconditional call: LISTVIEW_DeselectAllSkipItem(infoPtr, infoPtr->nEditLabelItem); > ReleaseCapture(); > } > >

Aug. 17, 2017

Re: [PATCH resend] msxml3: Remove CRs in domtext_put_data and add them in domtext_get_xml.

by Nikolay Sivov

On 03.08.2017 18:17, Alex Henrie wrote: > Fixes https://bugs.winehq.org/show_bug.cgi?id=42468 > > To be clear, I am not trying to fix all whitespace processing issues. I > am only trying to prevent "" from being erroneously inserted into > the output. > > Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com> > --- > dlls/msxml3/tests/domdoc.c | 106 +++++++++++++++++++++++++++++++++++++-------- > dlls/msxml3/text.c | 42 +++++++++++++++--- > 2 files changed, 122 insertions(+), 26 deletions(-) > > diff --git a/dlls/msxml3/tests/domdoc.c b/dlls/msxml3/tests/domdoc.c I think it needs more tests, for xml:space and put_preserveWhiteSpace(). > diff --git a/dlls/msxml3/text.c b/dlls/msxml3/text.c > index 3a12d4bf62..6814c1f817 100644 > --- a/dlls/msxml3/text.c > +++ b/dlls/msxml3/text.c > @@ -519,10 +519,15 @@ static HRESULT WINAPI domtext_get_xml( > BSTR* p) > { > domtext *This = impl_from_IXMLDOMText( iface ); > + HRESULT hr; > > TRACE("(%p)->(%p)\n", This, p); > > - return node_get_xml(&This->node, FALSE, p); > + hr = node_get_xml(&This->node, FALSE, p); > + if (hr == S_OK) > + *p = EnsureCorrectEOL(*p); > + > + return hr; Will node_get_xml(..., TRUE, ...) work? > } > > static HRESULT WINAPI domtext_transformNode( > @@ -616,15 +621,38 @@ static HRESULT WINAPI domtext_put_data( > BSTR data) > { > domtext *This = impl_from_IXMLDOMText( iface ); > - static const WCHAR rnW[] = {'\r','\n',0}; > + BSTR normalized_data = NULL; > + HRESULT hr; > + int i, j; > > TRACE("(%p)->(%s)\n", This, debugstr_w(data)); > > - if (data && !strcmpW(rnW, data)) > - This->node.node->name = xmlStringTextNoenc; > - else > - domtext_reset_noenc(This); > - return node_set_content(&This->node, data); > + if (data) > + { > + /* normalize line endings */ > + normalized_data = SysAllocStringLen(NULL, SysStringLen(data)); > + if (!normalized_data) return E_OUTOFMEMORY; > + for (i = 0, j = 0; data[i]; i++) > + { > + if (data[i] == '\r') > + { > + if (data[i + 1] == '\n') > + continue; /* change \r\n to just \n */ > + else > + normalized_data[j] = '\n'; /* change \r by itself to \n */ > + } > + else > + normalized_data[j] = data[i]; > + j++; > + } > + normalized_data[j] = 0; The only purpose for that is to inhibit libxml2 escaping, right? Could this be done the way it is now, but with something more elaborate than comparing to "\r\n"? > + } > + > + domtext_reset_noenc(This); > + hr = node_set_content(&This->node, normalized_data); > + > + SysFreeString(normalized_data); > + return hr; > } > > static HRESULT WINAPI domtext_get_length( >

Aug. 17, 2017

JACK driver for WINE

by Jeremy Banks

Hello all, I am wanting to output the audio from various applications run in WINE to the JACK server on my computer. I would prefer that WINE do this natively instead of routing across various wrapper setups with ALSA or PulseAudio. I have looked on the WINE wiki page for sound[1] and see that there was a JACK driver at one point, but it was removed when the driver architecture was changed. I am interested in trying to implement a driver to get WINE talking to JACK natively. The wiki says to ask on the mailing list before starting such a project, and so here I am. I have downloaded a local copy of the source repo and have started browsing through its contents. Being unfamiliar with the WINE codebase, which portions of the source should I start studying first? I am interested in studying the drivers for ALSA, OSS, and Pulse for comparison; do those live in a particular place in the source tree? I'm presently looking on the Wiki for other helpful information, but is there some particular documentation I should be studying for this endeavor? Regards, Jeremy Banks 1. https://wiki.winehq.org/Sound#Backend_information

Aug. 17, 2017

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

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

Aug. 17, 2017

Re: [PATCH 1/2] user32/tests: Test DialogBoxParam using a dialog template with invalid control class.

by Andrew Eikum

The test bot re-ran all of the tests because the RC file was changed. None of the failures below are in dialog.c. Andrew On Thu, Aug 17, 2017 at 10:26:46AM -0500, Marvin wrote: > Hi, > > While running your changed tests on Windows, I think I found new failures. > Being a bot and all I'm not very good at pattern recognition, so I might be > wrong, but could you please double-check? > Full results can be found at > https://testbot.winehq.org/JobDetails.pl?Key=32717 > > Your paranoid android. > > > === wvistau64 (32 bit listbox) === > listbox.c:706: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY, *) filled with 7 entries, expected > 7 > listbox.c:941: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18 > > === wvistau64 (64 bit listbox) === > listbox.c:706: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY, *) filled with 7 entries, expected > 7 > listbox.c:941: Test failed: SendMessage(LB_DIR, DDL_DIRECTORY|DDL_EXCLUSIVE, *) failed err 18 > > === w864 (64 bit broadcast) === > The task timed out > > === w8 (32 bit sysparams) === > sysparams.c:2519: Test failed: Waiting for the WM_DISPLAYCHANGE message timed out > sysparams.c:2530: Test failed: Set bpp 32, but WM_DISPLAYCHANGE reported bpp -1 > > === w864 (32 bit sysparams) === > sysparams.c:2519: Test failed: Waiting for the WM_DISPLAYCHANGE message timed out > sysparams.c:2530: Test failed: Set bpp 32, but WM_DISPLAYCHANGE reported bpp -1

Aug. 17, 2017

Re: [PATCH 1/2] user32/tests: Test DialogBoxParam using a dialog template with invalid control class.

by Dmitry Timoshkov

Andrew Eikum <aeikum(a)codeweavers.com> wrote: > On Fri, Aug 18, 2017 at 12:08:14AM +0800, Dmitry Timoshkov wrote: > > Please don't try to pretend that you are participating in some process, > > especially when you clearly have no idea what's that. What you are doing > > is stealing the work done by somebody else, admitting at the same time > > that you are not familar with that code. > > > > Golly, I wonder why no one respects you. Thank you mister "no one". I guess that providing good argumentation or investigating a problem and creating a patch that solves it is much more work than stealing somebody else patches and once receiving a complaint start speculating around. -- Dmitry.

Aug. 17, 2017

Re: [PATCH 1/2] user32/tests: Test DialogBoxParam using a dialog template with invalid control class.

by Andrew Eikum

On Fri, Aug 18, 2017 at 12:08:14AM +0800, Dmitry Timoshkov wrote: > Please don't try to pretend that you are participating in some process, > especially when you clearly have no idea what's that. What you are doing > is stealing the work done by somebody else, admitting at the same time > that you are not familar with that code. > Golly, I wonder why no one respects you. Andrew

Aug. 17, 2017