On Wed Aug 31 13:16:08 2022 +0000, Alexandros Frantzis wrote:
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:
- 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:
- 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").
- `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?
What matters is to replicate what happens on Windows. MCIWndCreateA/MCIWndCreateW will work as is I guess, because we control CreateWindow argument there. Regarding CreateWindowExA/CreateWindowExW, I don't think there is a way to tell from WM_CREATE which one was called, so maybe it should use some heuristic like it does now. This should be testable though. Basically you can register the class, then replace class procedure, and see what's coming in.