On Tue Aug 30 11:17:43 2022 +0000, Nikolay Sivov wrote:
It seems to me that such comparison will always match now, because you're checking class name from within WM_CREATE of the same class. I don't know exactly what was the point of original condition, maybe it's an equivalent of IsWindowUnicode() check instead? What you could probably test is which message is sent when mci window is created with CreateWindowA vs CreateWindowW, with or without a parent. And when with a parent, with both A and W parents.
Hi @nsivov, thanks for feedback!
I went down the rabbit hole, trying get a deeper understanding of this (18 year old!) code. Below are my findings (and a bit of background).
There are two ways to create an MCI window:
1. Call `MCIWndRegisterClass()` and then `CreateWindowEx(MCIWND_WINDOW_CLASS, ..., optional_path_to_open)` manually. 2. `MCIWndCreateA/W(optional_path_to_open)` which will register the class and call `CreateWindowExW()` internally
When an app calls `CreateWindowEx(MCIWND_WINDOW_CLASS, path)` we cannot tell whether the data is ansi or unicode, although I guess the implicit expectation is that it should match the CreateWindowExA/W function variant used. `IsWindowUnicode()` on the mci window itself is not useful since it relies on information from the registered class, which is always unicode in our case (see `MCIWndRegisterClass()`) rather on which function is used to create the window.
To work around this in the general case, the current code uses the unicode status of the *parent* window as a hint about whether the passed parameter is ansi or or unicode.
When using `MCIWndCreateA/W` in particular, the code knows that it always passes a unicode argument, and wants to communicate that to `MCIWND_Create`. This was previously achieved by checking for pointer equality between the internal class string memory used by `MCIWndCreateA/W` when calling `CreateWindowEx` and the class string received in the window process/`MCIWND_Create`. If these two matched we knew we were being called from `MCIWndCreateA/W` and thus that any passed string was unicode.
The changes in e41c255b broke this string pointer comparison, and so the MCIWndCreateW special path was invalidated. It's my understanding that the new behavior in that commit is correct, i.e., there is no guarantee that the string memory pointed to by the CREATESTRUCT lpszClass is the same as what was passed in `CreateWindowEx`. However, if that's not the case, and the new behavior is incorrect, it will of course void this whole MR and discussion.
My proposed fix which checks for (case-insensitive) class name equality fixes the `MCIWndCreateA/W` path, but, as you note, the class comparison is now always true, so it effectively disables the fallback heuristic, so now all strings are treated as unicode.
The bottom line is that we need to find a different mechanism for `MCIWndCreateA/W` to communicate at window creation that what it passed is in fact a unicode value.
Here are two (hacky and fragile, but not totally unreasonable) ideas:
1. Since class names are case-insensitive, we can use a string with an uncommon capitalization when we call `CreateWindowEx` from `MCIWndCreateA/W`, and check for that capitilization instead of performing a pointer comparison (e.g., "mcIwNDcLasS").
2. `MCIWndCreateA` could pass the path prefixed by some bytes that we wouldn't expect to find in a normal ansi or unicode path string. For example, use 0x02 0x00 as the first bytes to communicate that this is a unicode string.
But I would very much be interested in some cleaner solution. Any ideas?