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?