[PATCH 0/1] MR5893: uniscribe: Update ETO_GLYPH_INDEX flag even if script cache is freed.
If the script cache was already freed before ScriptTextOut() runs and hence the sfnt member is not available, use the definition from init_script_cache() to conditionally apply the ETO_GLYPH_INDEX flag. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5893
From: Danyil Blyschak <dblyschak(a)codeweavers.com> If the script cache was already freed before ScriptTextOut() runs and hence the sfnt member is not available, use the definition from init_script_cache() to conditionally apply the ETO_GLYPH_INDEX flag. --- dlls/gdi32/uniscribe/usp10.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/gdi32/uniscribe/usp10.c b/dlls/gdi32/uniscribe/usp10.c index 72e75224069..b1551967b7f 100644 --- a/dlls/gdi32/uniscribe/usp10.c +++ b/dlls/gdi32/uniscribe/usp10.c @@ -3524,7 +3524,7 @@ HRESULT WINAPI ScriptTextOut(const HDC hdc, SCRIPT_CACHE *psc, int x, int y, UIN fuOptions &= ETO_CLIPPED | ETO_OPAQUE; fuOptions |= ETO_IGNORELANGUAGE; - if (!psa->fNoGlyphIndex && *psc && ((ScriptCache *)*psc)->sfnt) + if (!psa->fNoGlyphIndex && (*psc ? ((ScriptCache *)*psc)->sfnt : NtGdiGetFontData(hdc, MS_MAKE_TAG('h','e','a','d'), 0, NULL, 0) != GDI_ERROR)) fuOptions |= ETO_GLYPH_INDEX; /* We do actually have glyph indices */ if (!(lpDx = heap_calloc(cGlyphs, 2 * sizeof(*lpDx)))) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5893
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=146455 Your paranoid android. === debian11b (build log) === 002c:err:winediag:h264_encoder_create GStreamer doesn't support H.264 encoding, please install appropriate plugins
Huw Davies (@huw) commented about dlls/gdi32/uniscribe/usp10.c:
fuOptions &= ETO_CLIPPED | ETO_OPAQUE; fuOptions |= ETO_IGNORELANGUAGE; - if (!psa->fNoGlyphIndex && *psc && ((ScriptCache *)*psc)->sfnt) + if (!psa->fNoGlyphIndex && (*psc ? ((ScriptCache *)*psc)->sfnt : NtGdiGetFontData(hdc, MS_MAKE_TAG('h','e','a','d'), 0, NULL, 0) != GDI_ERROR))
This looks like a hack to me. Shouldn't we just call `init_script_cache()` to ensure the cache is initialised? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5893#note_73907
What was the full call sequence? When cache is released normally the pointer is set to null as well, and here it looks more like sfnt wasn't set during initialization (no 'head' table somehow?), and then you simply set to potentially different data using current hdc. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5893#note_73923
On Fri Jun 21 18:26:12 2024 +0000, Nikolay Sivov wrote:
What was the full call sequence? When cache is released normally the pointer is set to null as well, and here it looks more like sfnt wasn't set during initialization (no 'head' table somehow?), and then you simply set to potentially different data using current hdc. I originally looked into this because of a Word 2016 bug where this flag wasn't being set when it should be. On runs where that bug replicates, before the ScriptTextOut() call there are a few calls to uniscribe:_ItemizeInternal, and then shortly before that is when that specific cache is freed with ScriptFreeCache(), alongside a few other caches.
In ScriptTextOut(), we're passed a pointer to a pointer to a ScriptCache struct. As you mention, when the cache is freed prior to this call, the pointer to ScriptCache is indeed set to null, so it looks like the prior ScriptFreeCache() call is working properly. What was happening before was that ScriptTextOut() assumed this cache must exist in order to update the ETO_GLYPH_INDEX flag, and so when *psc evaluated to false then it short circuited the if statement and didn't update the flag. So, I don't think sfnt is the issue, especially since using the same expression from its assignment here gets rid of the bug. Some questions: 1. Do you think the cache being freed before this function is invoked is problematic? If yes, then there might be an alternate fix where we can address that (after finding out if it's Wine's fault, or the application's). If not, then it seems to me that we need to make the logic around the ETO_GLYPH_INDEX flag more robust in the event that the cache doesn't exist. My motivation for this fix was: a. check psa->fNoGlyphIndex (like we did before) b. if the cache exists, check the sfnt member c. if the cache doesn't exist, effectively check the same thing as b, by calling NtGdiGetFontData (since this is what sfnt was initialized to when the cache was created). By the way, since sfnt is the only use of the cache in this function, I didn't initialize a new one with init_script_cache to avoid the overhead of doing so and possibly introducing a memory leak if we don't free it later. 2. You bring a great point about the hdc being different. My first thought is that we would want to use the most recent hdc when calling this function anyway, and we would have to be more careful when using the cache since that might be initialized with an earlier hdc. Do you think that using the hdc passed to ScriptTextOut() could cause issues? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5893#note_73932
On Fri Jun 21 18:26:12 2024 +0000, Danyil Blyschak wrote:
I originally looked into this because of a Word 2016 bug where this flag wasn't being set when it should be. On runs where that bug replicates, before the ScriptTextOut() call there are a few calls to uniscribe:_ItemizeInternal, and then shortly before that is when that specific cache is freed with ScriptFreeCache(), alongside a few other caches. In ScriptTextOut(), we're passed a pointer to a pointer to a ScriptCache struct. As you mention, when the cache is freed prior to this call, the pointer to ScriptCache is indeed set to null, so it looks like the prior ScriptFreeCache() call is working properly. What was happening before was that ScriptTextOut() assumed this cache must exist in order to update the ETO_GLYPH_INDEX flag, and so when *psc evaluated to false then it short circuited the if statement and didn't update the flag. So, I don't think sfnt is the issue, especially since using the same expression from its assignment here gets rid of the bug. Some questions: 1. Do you think the cache being freed before this function is invoked is problematic? If yes, then there might be an alternate fix where we can address that (after finding out if it's Wine's fault, or the application's). If not, then it seems to me that we need to make the logic around the ETO_GLYPH_INDEX flag more robust in the event that the cache doesn't exist. My motivation for this fix was: a. check psa->fNoGlyphIndex (like we did before) b. if the cache exists, check the sfnt member c. if the cache doesn't exist, effectively check the same thing as b, by calling NtGdiGetFontData (since this is what sfnt was initialized to when the cache was created). By the way, since sfnt is the only use of the cache in this function, I didn't initialize a new one with init_script_cache to avoid the overhead of doing so and possibly introducing a memory leak if we don't free it later. 2. You bring a great point about the hdc being different. My first thought is that we would want to use the most recent hdc when calling this function anyway, and we would have to be more careful when using the cache since that might be initialized with an earlier hdc. Do you think that using the hdc passed to ScriptTextOut() could cause issues? Adding a test for `psc` on line 2870 of `dlls/usp10/tests/usp10.c` shows that `ScriptTextOut()` creates a cache if a freed one is passed in. So we should do that too.
With respect to changing the hdc, we should probably check for that in `init_script_cache()` and invalidate it if the hdc has changed. Is Word using the same hdc in this case? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5893#note_73936
On Fri Jun 21 19:25:29 2024 +0000, Huw Davies wrote:
Adding a test for `psc` on line 2870 of `dlls/usp10/tests/usp10.c` shows that `ScriptTextOut()` creates a cache if a freed one is passed in. So we should do that too. With respect to changing the hdc, we should probably check for that in `init_script_cache()` and invalidate it if the hdc has changed. Is Word using the same hdc in this case? Changing hdc is a separate issue, I think. Could be irrelevant for this particular case. There are ways to make uniscribe select different fonts automatically, and that makes caching for some font, that used to be selected at some point, not useful. I don't remember which api does that, maybe String* functions. For ScriptTextOut() we probably need to fully initialize cache in a "normal" way.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5893#note_74003
On Sat Jun 22 07:56:35 2024 +0000, Nikolay Sivov wrote:
Changing hdc is a separate issue, I think. Could be irrelevant for this particular case. There are ways to make uniscribe select different fonts automatically, and that makes caching for some font, that used to be selected at some point, not useful. I don't remember which api does that, maybe String* functions. For ScriptTextOut() we probably need to fully initialize cache in a "normal" way. Yes, agreed, we don't need to worry about the hdc being changed in this MR. It's clear the fix is to call `init_script_cache()` from `ScriptTextOut()`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5893#note_74138
participants (5)
-
Danyil Blyschak -
Danyil Blyschak (＠blyss)
-
Huw Davies (＠huw)
-
Marvin -
Nikolay Sivov (＠nsivov)