Details

      Description

      When you enable theming, if you start a win32 gui app that is ANSI (and not UNICODE), some controls get unicode strings where they should not be, and you get garbled text.
      I tested on ReactOS trunk r64112, with russian language, the following app: qemu-w32-setup-20140801.exe, available at http://qemu.weilnetz.de/w32/qemu-w32-setup-20140801.exe . See the screenshots and logs for more information.

      1. comctl_subclass_ansi.patch
        2 kB
        Sylvain Deverre
      2. comctl32.patch
        3 kB
        HBelusca
      3. failed_ansi_unicode_test.7z
        1 kB
        Sylvain Deverre
      4. non_themed_log.txt
        5 kB
        HBelusca
      5. ntuser_window.patch
        2 kB
        HBelusca
      6. test_comctl32_theme.7z
        1 kB
        Sylvain Deverre
      7. themed_log.txt
        6 kB
        HBelusca
      8. user32.patch
        2 kB
        HBelusca
      9. uxtheme.patch
        3 kB
        HBelusca

        Issue Links

          Activity

          Hide
          HBelusca
          added a comment -

          Logs displaying backtraces when the ThemeDefWindowProcA/W functions from uxtheme.dll get called with the WM_SETTEXT message (the message that is sent to display the text for the taskbar button and the static text of the progress dialog).

          Show
          HBelusca
          added a comment - Logs displaying backtraces when the ThemeDefWindowProcA/W functions from uxtheme.dll get called with the WM_SETTEXT message (the message that is sent to display the text for the taskbar button and the static text of the progress dialog).
          Hide
          HBelusca
          added a comment -

          Patch that can be applied to uxtheme to get similar backtrace (activate the __debugbreak(); commands and before testing the app, enable "set condition * first always" in the kernel debugger).

          Show
          HBelusca
          added a comment - Patch that can be applied to uxtheme to get similar backtrace (activate the __debugbreak(); commands and before testing the app, enable "set condition * first always" in the kernel debugger).
          Hide
          HBelusca
          added a comment -

          Additional stuff I changed in user32. I don't think it has a big influence in the outcome of the theming issue, but if you want to have a look at it...

          Show
          HBelusca
          added a comment - Additional stuff I changed in user32. I don't think it has a big influence in the outcome of the theming issue, but if you want to have a look at it...
          Hide
          HBelusca
          added a comment -

          comctl32.patch: Enabling or disabling dialog and / or button window procedures hooks, has an influence on the overall look: basically disabling the hooks make everything working again...

          Show
          HBelusca
          added a comment - comctl32.patch: Enabling or disabling dialog and / or button window procedures hooks, has an influence on the overall look: basically disabling the hooks make everything working again...
          Hide
          HBelusca
          added a comment -

          ntuser_window.patch: Finally, this chunk of code that is commented there, looks suspect to me. When doing the backtraces it wasn't commented. If you comment it (see patch) then now, only ThemeDefWindowProcA gets called, but with a valid Unicode string (the ansi one is therefore invalid).

          Show
          HBelusca
          added a comment - ntuser_window.patch: Finally, this chunk of code that is commented there, looks suspect to me. When doing the backtraces it wasn't commented. If you comment it (see patch) then now, only ThemeDefWindowProcA gets called, but with a valid Unicode string (the ansi one is therefore invalid).
          Hide
          HBelusca
          added a comment -

          Giannis Adamopoulos Can you quickly check whether your fix of r65077 may improve the situation of this bug? (I cannot test atm.)

          Show
          HBelusca
          added a comment - Giannis Adamopoulos Can you quickly check whether your fix of r65077 may improve the situation of this bug? (I cannot test atm.)
          Hide
          jedi-to-be
          added a comment -

          Hey! =) What is the status?

          Show
          jedi-to-be
          added a comment - Hey! =) What is the status?
          Hide
          Robert Naumann
          added a comment -

          This is the same issue

          Show
          Robert Naumann
          added a comment - This is the same issue
          Hide
          Jared Smudde
          added a comment -

          Which of these patches fix the issue?

          Show
          Jared Smudde
          added a comment - Which of these patches fix the issue?
          Hide
          Robert Naumann
          added a comment -

          none. these are only tests by HBelusca to find the source of the bug.

          Show
          Robert Naumann
          added a comment - none. these are only tests by HBelusca to find the source of the bug.
          Hide
          Jared Smudde
          added a comment -

          Did we find the source of the bug?

          Show
          Jared Smudde
          added a comment - Did we find the source of the bug?
          Hide
          jimtabor
          added a comment -

          Has anyone use /trunk/rostests/tests/combotst for testing this? ATM, WIP on moving Menu and Scroll to server side and this includes some code changes in UxTheme. So most of those patches will be out of date.

          Show
          jimtabor
          added a comment - Has anyone use /trunk/rostests/tests/combotst for testing this? ATM, WIP on moving Menu and Scroll to server side and this includes some code changes in UxTheme. So most of those patches will be out of date.
          Hide
          jimtabor
          added a comment -

          Wondering if it is a Peek Message A issue? A to W or W to A. Post A should be Peek A, well...

          Show
          jimtabor
          added a comment - Wondering if it is a Peek Message A issue? A to W or W to A. Post A should be Peek A, well...
          Hide
          Giannis Adamopoulos
          added a comment -

          jimtabor: That would be a simple case actually. Here comctl32 subclasses the dialog class and then tries to call the DefWindowProc. Then uxtheme gets the call and tries to handle it and draw the themed caption and stuff like that. In the mean time we have several A->W and W->A conversions in this whole process.

          Show
          Giannis Adamopoulos
          added a comment - jimtabor : That would be a simple case actually. Here comctl32 subclasses the dialog class and then tries to call the DefWindowProc. Then uxtheme gets the call and tries to handle it and draw the themed caption and stuff like that. In the mean time we have several A->W and W->A conversions in this whole process.
          Hide
          Sylvain Deverre
          added a comment - - edited

          I think it's only a comctl32-only issue, since forcing a testing ANSI application to use ReactOS comctl32 on Windows also triggers the bug (garbled text), which means Windows uxtheme doesn't seem to convert anything from ANSI to Unicode unlike Wine's uxtheme on which the bug doesn't appear.

          However comctl32 seems to handle correctly ANSI/Unicode in its own components, only subclassed controls seems to have the bug.

          Show
          Sylvain Deverre
          added a comment - - edited I think it's only a comctl32-only issue, since forcing a testing ANSI application to use ReactOS comctl32 on Windows also triggers the bug (garbled text), which means Windows uxtheme doesn't seem to convert anything from ANSI to Unicode unlike Wine's uxtheme on which the bug doesn't appear. However comctl32 seems to handle correctly ANSI/Unicode in its own components, only subclassed controls seems to have the bug.
          Hide
          Sylvain Deverre
          added a comment -

          I made a dummy ANSI application in attachment to test comctl32 behaviour (the source is in attachment), and observed two weird things both in Windows XP AND ReactOS:

          • SendMessageA with WM_SETTEXT and Unicode string seems to be sent "as is" to wndproc
          • SendMessageW with WM_SETTEXT and Unicode string get converted to ANSI string.

          PS: to use my dummy program, on Windows, you can use test-win.exe and provide a copy of ReactOS's comctl32.dll in the same folder of the test app, and on ReactOS, you can run test.exe directly (it will load comctl32.exe). Themes must be enabled to test, since comctl32 aborts and doesn't subclass anything if no theme is loaded.

          Show
          Sylvain Deverre
          added a comment - I made a dummy ANSI application in attachment to test comctl32 behaviour (the source is in attachment), and observed two weird things both in Windows XP AND ReactOS: SendMessageA with WM_SETTEXT and Unicode string seems to be sent "as is" to wndproc SendMessageW with WM_SETTEXT and Unicode string get converted to ANSI string. PS: to use my dummy program, on Windows, you can use test-win.exe and provide a copy of ReactOS's comctl32.dll in the same folder of the test app, and on ReactOS, you can run test.exe directly (it will load comctl32.exe). Themes must be enabled to test, since comctl32 aborts and doesn't subclass anything if no theme is loaded.
          Hide
          Sylvain Deverre
          added a comment -

          Finally, I investigated more on the issue, and figured that when the application is ANSI, comctl32's wndproc replacement is called as ANSI wndproc but unicode original instead of ANSI one.

          So I made a patch to retrieve and save original ANSI WndProc, and make THEMING_CallOriginalClass to call it when window is ANSI window, which seems to fix the issue on both Windows and ReactOS for my test program.

          Show
          Sylvain Deverre
          added a comment - Finally, I investigated more on the issue, and figured that when the application is ANSI, comctl32's wndproc replacement is called as ANSI wndproc but unicode original instead of ANSI one. So I made a patch to retrieve and save original ANSI WndProc, and make THEMING_CallOriginalClass to call it when window is ANSI window, which seems to fix the issue on both Windows and ReactOS for my test program.
          Hide
          HBelusca
          added a comment -

          Sylvain Deverre Thanks for the patch; can you check whether Wine (from which we sync comctl32) has made a similar fix recently?

          Show
          HBelusca
          added a comment - Sylvain Deverre Thanks for the patch; can you check whether Wine (from which we sync comctl32) has made a similar fix recently?
          Hide
          jimtabor
          added a comment -

          This is much more correct! Better than hack win32k/user32 ;^)

          Show
          jimtabor
          added a comment - This is much more correct! Better than hack win32k/user32 ;^)
          Hide
          Sylvain Deverre
          added a comment -

          @HBelusca: Wine doesn't seem to have the patch in their git, however "buggy" comctl32 behaves correctly in Wine, so I think they did some hacks in their win32 subsystem to make it work. We also encounter drawing problems that don't appear in Wine but appear in Windows and ReactOS (https://jira.reactos.org/browse/CORE-10761), even if my patches doesn't seem to change anything in Wine.

          @jimtabor: thanks

          Show
          Sylvain Deverre
          added a comment - @HBelusca: Wine doesn't seem to have the patch in their git, however "buggy" comctl32 behaves correctly in Wine, so I think they did some hacks in their win32 subsystem to make it work. We also encounter drawing problems that don't appear in Wine but appear in Windows and ReactOS ( https://jira.reactos.org/browse/CORE-10761 ), even if my patches doesn't seem to change anything in Wine. @jimtabor: thanks
          Hide
          Sylvain Deverre
          added a comment - - edited

          Well, my patch makes Resource Hacker from rapps uninstallable (setup based on InnoSetup). I believe the setup is mixing ANSI and Unicode wndprocs, and I don't know if it is a bug in ANSI/Unicode translation bug in win32k or if it only belongs to comctl32 (which is "unicode only" according to MSDN, and I think it reimplements every user32 classes and do not call original wndprocs).

          EDIT: the problem seems to affect all (recent ?) InnoSetup installers.

          Show
          Sylvain Deverre
          added a comment - - edited Well, my patch makes Resource Hacker from rapps uninstallable (setup based on InnoSetup). I believe the setup is mixing ANSI and Unicode wndprocs, and I don't know if it is a bug in ANSI/Unicode translation bug in win32k or if it only belongs to comctl32 (which is "unicode only" according to MSDN, and I think it reimplements every user32 classes and do not call original wndprocs). EDIT: the problem seems to affect all (recent ?) InnoSetup installers.
          Hide
          HBelusca
          added a comment -

          The question: does it break all those installers when being run on Windows with patched ROS comctl32?

          Show
          HBelusca
          added a comment - The question: does it break all those installers when being run on Windows with patched ROS comctl32?
          Hide
          Sylvain Deverre
          added a comment -

          It will need a little more work (mostly understand how InnoSetup worker extracted to temp file works, patch the right file to load the right comctl32, etc), but I'm investigating on this.

          Show
          Sylvain Deverre
          added a comment - It will need a little more work (mostly understand how InnoSetup worker extracted to temp file works, patch the right file to load the right comctl32, etc), but I'm investigating on this.
          Hide
          Sylvain Deverre
          added a comment - - edited

          The problem also occurs on Windows, so it's again a comctl32 problem.

          One difference though, edit control behaves correctly, but buttons displaying is messed up (only first char appears)

          Show
          Sylvain Deverre
          added a comment - - edited The problem also occurs on Windows, so it's again a comctl32 problem. One difference though, edit control behaves correctly, but buttons displaying is messed up (only first char appears)
          Hide
          Sylvain Deverre
          added a comment -

          After analysis, I found it is a problem with superclassing: in InnoSetup, Delphi version used in the installer (Delphi 1) is creating a window class for each Delphi control, and has its own wndproc that uses CallWindowProcA with Windows control's wndproc.
          However, since our comctl32 puts only Unicode wndproc, Win32 subsystem set a default ANSI wndproc telling the system to convert strings in messages to Unicode before calling the proc, and since the hWnd is still ANSI, our comctl32 calls the saved ANSI user32 wndproc, with the Unicode wndproc. So we need to find a way to detect reliably if we have Unicode message or ANSI one.
          I succeded to reproduce a minimal program which shows the bug on both Windows and ReactOS (and a bonus one in ReactOS, possibly related to https://jira.reactos.org/browse/CORE-9998), you can find in attachment.

          Show
          Sylvain Deverre
          added a comment - After analysis, I found it is a problem with superclassing: in InnoSetup, Delphi version used in the installer (Delphi 1) is creating a window class for each Delphi control, and has its own wndproc that uses CallWindowProcA with Windows control's wndproc. However, since our comctl32 puts only Unicode wndproc, Win32 subsystem set a default ANSI wndproc telling the system to convert strings in messages to Unicode before calling the proc, and since the hWnd is still ANSI, our comctl32 calls the saved ANSI user32 wndproc, with the Unicode wndproc. So we need to find a way to detect reliably if we have Unicode message or ANSI one. I succeded to reproduce a minimal program which shows the bug on both Windows and ReactOS (and a bonus one in ReactOS, possibly related to https://jira.reactos.org/browse/CORE-9998 ), you can find in attachment.
          Hide
          Sylvain Deverre
          added a comment -

          After more testings on my Windows machine, I saw that comctl32 doesn't subclass Win32 controls but REPLACES them with its own copies, thanks to SxS mechanisms (seems that both Windows and ReactOS subclassing mechanisms get easily lost with ANSI/Unicode translations in messages, as my tests and comctl32 bugs showed).
          So it seems that we will need proper SxS implementation (regarding class redirections) and also having two versions of comctl32: a legacy one that doesn't subclass window controls to apply themes, and another comctl32 as a SxS assembly used when application need themes to get rid of that bugs without hacks that can't handle 100% of cases. The bad news here is that it seems we'll need to copy and adapt user32 code in the new comctl32.

          Show
          Sylvain Deverre
          added a comment - After more testings on my Windows machine, I saw that comctl32 doesn't subclass Win32 controls but REPLACES them with its own copies, thanks to SxS mechanisms (seems that both Windows and ReactOS subclassing mechanisms get easily lost with ANSI/Unicode translations in messages, as my tests and comctl32 bugs showed). So it seems that we will need proper SxS implementation (regarding class redirections) and also having two versions of comctl32: a legacy one that doesn't subclass window controls to apply themes, and another comctl32 as a SxS assembly used when application need themes to get rid of that bugs without hacks that can't handle 100% of cases. The bad news here is that it seems we'll need to copy and adapt user32 code in the new comctl32.
          Hide
          swyter
          added a comment - - edited

          A year ago the devs were discussing SxS after having problems with theming/subclassing and they decided that SxS wasn't worth it for the time being, with the difficulty of implementation and deduplication of the various yet slightly different copies of the assemblies.

          I don't know how things are today but I wouldn't bet my tasty olives (yum yum) on it.

          Show
          swyter
          added a comment - - edited A year ago the devs were discussing SxS after having problems with theming/subclassing and they decided that SxS wasn't worth it for the time being, with the difficulty of implementation and deduplication of the various yet slightly different copies of the assemblies. I don't know how things are today but I wouldn't bet my tasty olives (yum yum) on it.
          Hide
          Sylvain Deverre
          added a comment -

          I'm just reporting tests I did on my Windows machine, devs will take the final decision

          However, it seems that there is a beginning of SxS implementation in ReactOS (SxS stuff output appeared in my debug logs), and maybe it will receive more love from the devs

          Show
          Sylvain Deverre
          added a comment - I'm just reporting tests I did on my Windows machine, devs will take the final decision However, it seems that there is a beginning of SxS implementation in ReactOS (SxS stuff output appeared in my debug logs), and maybe it will receive more love from the devs
          Hide
          serrox
          added a comment - - edited

          i found note about how it is done in windows in Raymond Chen's blog: https://blogs.msdn.microsoft.com/oldnewthing/20081106-00/?p=20303
          It seems that your assumption on it is right

          Upd: one more related article from.Raymond Chen's blog: https://blogs.msdn.microsoft.com/oldnewthing/20080129-00/?p=23663

          Show
          serrox
          added a comment - - edited i found note about how it is done in windows in Raymond Chen's blog: https://blogs.msdn.microsoft.com/oldnewthing/20081106-00/?p=20303 It seems that your assumption on it is right Upd: one more related article from.Raymond Chen's blog: https://blogs.msdn.microsoft.com/oldnewthing/20080129-00/?p=23663
          Hide
          Giannis Adamopoulos
          added a comment -

          Should be fixed in r73803.

          Show
          Giannis Adamopoulos
          added a comment - Should be fixed in r73803.
          Hide
          Jared Smudde
          added a comment -

          Giannis Adamopoulos Seems fixed to me.

          Show
          Jared Smudde
          added a comment - Giannis Adamopoulos Seems fixed to me.
          Hide
          Sylvain Deverre
          added a comment -

          Jared Smudde Could you try with the "tests" I attached too ? (I'll do it when I'm back to home otherwise)

          Show
          Sylvain Deverre
          added a comment - Jared Smudde Could you try with the "tests" I attached too ? (I'll do it when I'm back to home otherwise)
          Hide
          Sylvain Deverre
          added a comment -

          My tests don't show the bug anymore on reactOS, so it should be fixed now.

          Show
          Sylvain Deverre
          added a comment - My tests don't show the bug anymore on reactOS, so it should be fixed now.
          Hide
          Jared Smudde
          added a comment -

          Closing then.

          Show
          Jared Smudde
          added a comment - Closing then.
          Hide
          Rafael
          added a comment -

          Problem is still there, if you use another themes (royal blue, zune, luna, posready). if use - lautus default theme, then everything is ok.

          Show
          Rafael
          added a comment - Problem is still there, if you use another themes (royal blue, zune, luna, posready). if use - lautus default theme, then everything is ok.
          Hide
          Sylvain Deverre
          added a comment -

          Rafael This looks like a font issue for me, not related to what this bugreport was for (I am not able to reproduce your issue with a latin charset).

          Changing the window title font to "Tahoma" or "Ubuntu" should fix your issue.

          Show
          Sylvain Deverre
          added a comment - Rafael This looks like a font issue for me, not related to what this bugreport was for (I am not able to reproduce your issue with a latin charset). Changing the window title font to "Tahoma" or "Ubuntu" should fix your issue.
          Hide
          Rafael
          added a comment -

          Sylvain Deverre, problem disappeared, after changing the font, thanks,.

          Show
          Rafael
          added a comment - Sylvain Deverre , problem disappeared, after changing the font, thanks,.

            People

            • Assignee:
              Bug Zilla
              Reporter:
              HBelusca
            • Votes:
              2 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: