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:
- 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?