[PATCH v3] comctl32: Initialize early font metrics in edit control.

[Roman Pišl]

v2: Introduced helper function.
v3: Improved helper function (I really didn't like the previous).

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48803
Signed-off-by: Roman Pišl 
---
 dlls/comctl32/edit.c       | 38 ++++++++++++++++--------
 dlls/comctl32/tests/edit.c | 59 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+), 12 deletions(-)

diff --git a/dlls/comctl32/edit.c b/dlls/comctl32/edit.c
index d02d7af7b9..e7a3f9b7f9 100644
--- a/dlls/comctl32/edit.c
+++ b/dlls/comctl32/edit.c
@@ -3724,25 +3724,14 @@ static DWORD get_font_margins(HDC hdc, const TEXTMETRICW *tm)
    return MAKELONG(left, right);
 }

-/*********************************************************************
- *
- *	WM_SETFONT
- *
- * With Win95 look the margins are set to default font value unless
- * the system font (font == 0) is being set, in which case they are left
- * unchanged.
- *
- */
-static void EDIT_WM_SetFont(EDITSTATE *es, HFONT font, BOOL redraw)
+static DWORD set_font(EDITSTATE *es, HFONT font)
 {
    TEXTMETRICW tm;
    HDC dc;
    HFONT old_font = 0;
-	RECT clientRect;
    DWORD margins;

es->font = font;
-	EDIT_InvalidateUniscribeData(es);
    dc = GetDC(es->hwndSelf);
    if (font)
    	old_font = SelectObject(dc, font);
@@ -3754,6 +3743,26 @@ static void EDIT_WM_SetFont(EDITSTATE *es, HFONT font, BOOL redraw)
    	SelectObject(dc, old_font);
    ReleaseDC(es->hwndSelf, dc);

+	return margins;
+}
+
+/*********************************************************************
+ *
+ *	WM_SETFONT
+ *
+ * With Win95 look the margins are set to default font value unless
+ * the system font (font == 0) is being set, in which case they are left
+ * unchanged.
+ *
+ */
+static void EDIT_WM_SetFont(EDITSTATE *es, HFONT font, BOOL redraw)
+{
+	RECT clientRect;
+	DWORD margins;
+
+	EDIT_InvalidateUniscribeData(es);
+	margins = set_font(es, font);
+
    /* Reset the format rect and the margins */
    GetClientRect(es->hwndSelf, &clientRect);
    EDIT_SetRectNP(es, &clientRect);
@@ -4488,6 +4497,11 @@ static LRESULT EDIT_WM_NCCreate(HWND hwnd, LPCREATESTRUCTW lpcs)
         else if (es->style & WS_BORDER)
    	SetWindowLongW(hwnd, GWL_STYLE, es->style & ~WS_BORDER);

+	/*
+	 * Initialize font metrics - line_height and char_width so that
+	 * early messages before WM_CREATE don't lead to division by zero.
+	 */
+	set_font(es, 0);
    return TRUE;

cleanup:
diff --git a/dlls/comctl32/tests/edit.c b/dlls/comctl32/tests/edit.c
index cec6025077..3bfbee4ddd 100644
--- a/dlls/comctl32/tests/edit.c
+++ b/dlls/comctl32/tests/edit.c
@@ -1768,6 +1768,64 @@ static BOOL is_font_installed(const char*name)
     return ret;
 }

+static WNDPROC orig_class_proc;
+
+static LRESULT CALLBACK test_class_proc(HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam)
+{
+    RECT rect;
+    LRESULT result;
+
+    switch (message)
+    {
+        case WM_NCCREATE:
+            result = CallWindowProcA(orig_class_proc, hwnd, message, wParam, lParam);
+
+            memset(&rect, 0, sizeof(rect));
+            SendMessageA(hwnd, EM_GETRECT, 0, (LPARAM)&rect);
+            ok(!rect.right && !rect.bottom, "Invalid size after NCCREATE: %d x %d\n", rect.right, rect.bottom);
+
+            /* test that early messages don't crash or cause unexpected behavior */
+            SendMessageA(hwnd, EM_SETSEL, 0, 0);
+            SendMessageA(hwnd, WM_SIZE, 0, 0);
+
+            return result;
+
+        case WM_CREATE:
+            /* test that early messages don't crash or cause unexpected behavior */
+            SendMessageA(hwnd, EM_SETSEL, 0, 0);
+            SendMessageA(hwnd, WM_SIZE, 0, 0);
+
+            break;
+    }
+
+    return CallWindowProcA(orig_class_proc, hwnd, message, wParam, lParam);
+}
+
+static void test_early_messages(void)
+{
+    BOOL ret;
+    ATOM atom;
+    HWND hwEdit;
+    WNDCLASSA cls;
+
+    ret = GetClassInfoA(NULL, "Edit", &cls);
+    ok(ret, "Failed to get class info.\n");
+
+    orig_class_proc = cls.lpfnWndProc;
+    cls.lpfnWndProc = test_class_proc;
+    cls.lpszClassName = "TestClassName";
+
+    atom = RegisterClassA(&cls);
+    ok(atom != 0, "Failed to register class.\n");
+
+    hwEdit = CreateWindowExA(0, (LPCSTR)MAKEINTATOM(atom), "Text Text", ES_MULTILINE | WS_BORDER | ES_AUTOHSCROLL | ES_AUTOVSCROLL,
+        10, 10, 300, 300, NULL, NULL, hinst, NULL);
+    ok(hwEdit != NULL, "Failed to create a window.\n");
+
+    DestroyWindow(hwEdit);
+    UnregisterClassA((LPCSTR)MAKEINTATOM(atom), hinst);
+}
+
 static void test_margins(void)
 {
     DWORD old_margins, new_margins;
@@ -3393,6 +3451,7 @@ START_TEST(edit)
     test_edit_control_6();
     test_edit_control_limittext();
     test_edit_control_scroll();
+    test_early_messages();
     test_margins();
     test_margins_font_change();
     test_text_position();

[Roman Pišl]

Hello,
I just tested all affected applications from bugzilla and just setting 
es->line_height = 1 and es->char_width = 1 in NCCreate is sufficient, 
since all other content is zero anyway and proper value is set during 
WM_CREATE.

So a less intrusive change is possible..

Roman

Dne 26. 03. 20 v 16:54 Roman Pišl napsal(a):
> v2: Introduced helper function.
> v3: Improved helper function (I really didn't like the previous).
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48803
> Signed-off-by: Roman Pišl
>
> dlls/comctl32/edit.c       | 38 ++++++++++++++++--------
>   dlls/comctl32/tests/edit.c | 59 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 85 insertions(+), 12 deletions(-)
>
> diff --git a/dlls/comctl32/edit.c b/dlls/comctl32/edit.c
> index d02d7af7b9..e7a3f9b7f9 100644
> --- a/dlls/comctl32/edit.c
> +++ b/dlls/comctl32/edit.c
> @@ -3724,25 +3724,14 @@ static DWORD get_font_margins(HDC hdc, const TEXTMETRICW *tm)
>    return MAKELONG(left, right);
>   }
>
> -/*********************************************************************
> -
> -
> -
> - WM_SETFONT
> -
> -
> -
> - With Win95 look the margins are set to default font value unless
> -
> - the system font (font == 0) is being set, in which case they are left
> -
> - unchanged.
> -
> -
> - */
>
> -static void EDIT_WM_SetFont(EDITSTATE *es, HFONT font, BOOL redraw)
> +static DWORD set_font(EDITSTATE *es, HFONT font)
>   {
>    TEXTMETRICW tm;
>    HDC dc;
>    HFONT old_font = 0;
> -
>
> RECT clientRect;
> DWORD margins;
>
> es->font = font;
> -
>
> EDIT_InvalidateUniscribeData(es);
> dc = GetDC(es->hwndSelf);
> if (font)
> old_font = SelectObject(dc, font);
>
> @@ -3754,6 +3743,26 @@ static void EDIT_WM_SetFont(EDITSTATE *es, HFONT font, BOOL redraw)
>    	SelectObject(dc, old_font);
>    ReleaseDC(es->hwndSelf, dc);
> - return margins;
>
> +}
> -
>
> +/*********************************************************************
> -
> -
> -
> - WM_SETFONT
> -
> -
> -
> - With Win95 look the margins are set to default font value unless
> -
> - the system font (font == 0) is being set, in which case they are left
> -
> - unchanged.
> -
> -
> - */
>
> +static void EDIT_WM_SetFont(EDITSTATE *es, HFONT font, BOOL redraw)
> +{
> - RECT clientRect;
> - DWORD margins;
> -
> - EDIT_InvalidateUniscribeData(es);
> - margins = set_font(es, font);
> - /* Reset the format rect and the margins */
>  GetClientRect(es->hwndSelf, &clientRect);
>  EDIT_SetRectNP(es, &clientRect);
>
> @@ -4488,6 +4497,11 @@ static LRESULT EDIT_WM_NCCreate(HWND hwnd, LPCREATESTRUCTW lpcs)
>           else if (es->style & WS_BORDER)
>    	SetWindowLongW(hwnd, GWL_STYLE, es->style & ~WS_BORDER);
> -
>
> /*
> -
> * Initialize font metrics - line_height and char_width so that
> -
> * early messages before WM_CREATE don't lead to division by zero.
> -
> */
> -
>
> set_font(es, 0);
> return TRUE;
>
> cleanup:
>
> diff --git a/dlls/comctl32/tests/edit.c b/dlls/comctl32/tests/edit.c
> index cec6025077..3bfbee4ddd 100644
> --- a/dlls/comctl32/tests/edit.c
> +++ b/dlls/comctl32/tests/edit.c
> @@ -1768,6 +1768,64 @@ static BOOL is_font_installed(const char*name)
>       return ret;
>   }
>
> +static WNDPROC orig_class_proc;
> -
>
> +static LRESULT CALLBACK test_class_proc(HWND hwnd, UINT message, WPARAM wParam, LPARAM lParam)
> +{
> - RECT rect;
> - LRESULT result;
> -
> - switch (message)
> - {
> -
>    case WM_NCCREATE:
> -
>        result = CallWindowProcA(orig_class_proc, hwnd, message, wParam, lParam);
> -
> -
>        memset(&rect, 0, sizeof(rect));
> -
>        SendMessageA(hwnd, EM_GETRECT, 0, (LPARAM)&rect);
> -
>        ok(!rect.right && !rect.bottom, "Invalid size after NCCREATE: %d x %d\n", rect.right, rect.bottom);
> -
> -
>        /* test that early messages don't crash or cause unexpected behavior */
> -
>        SendMessageA(hwnd, EM_SETSEL, 0, 0);
> -
>        SendMessageA(hwnd, WM_SIZE, 0, 0);
> -
> -
>        return result;
> -
> -
>    case WM_CREATE:
> -
>        /* test that early messages don't crash or cause unexpected behavior */
> -
>        SendMessageA(hwnd, EM_SETSEL, 0, 0);
> -
>        SendMessageA(hwnd, WM_SIZE, 0, 0);
> -
> -
>        break;
> - }
> -
> - return CallWindowProcA(orig_class_proc, hwnd, message, wParam, lParam);
>
> +}
> -
>
> +static void test_early_messages(void)
> +{
> - BOOL ret;
> - ATOM atom;
> - HWND hwEdit;
> - WNDCLASSA cls;
> -
> - ret = GetClassInfoA(NULL, "Edit", &cls);
> - ok(ret, "Failed to get class info.\n");
> -
> - orig_class_proc = cls.lpfnWndProc;
> - cls.lpfnWndProc = test_class_proc;
> - cls.lpszClassName = "TestClassName";
> -
> - atom = RegisterClassA(&cls);
> - ok(atom != 0, "Failed to register class.\n");
> -
> - hwEdit = CreateWindowExA(0, (LPCSTR)MAKEINTATOM(atom), "Text Text", ES_MULTILINE | WS_BORDER | ES_AUTOHSCROLL | ES_AUTOVSCROLL,
> -
>    10, 10, 300, 300, NULL, NULL, hinst, NULL);
> - ok(hwEdit != NULL, "Failed to create a window.\n");
> -
> - DestroyWindow(hwEdit);
> - UnregisterClassA((LPCSTR)MAKEINTATOM(atom), hinst);
>
> +}
> - static void test_margins(void)
> {
>     DWORD old_margins, new_margins;
>
> @@ -3393,6 +3451,7 @@ START_TEST(edit)
>       test_edit_control_6();
>       test_edit_control_limittext();
>       test_edit_control_scroll();
> - test_early_messages();
>  test_margins();
>  test_margins_font_change();
>  test_text_position();

[Nikolay Sivov]

On 3/26/20 10:07 PM, Roman Pišl wrote:
> Hello,
> I just tested all affected applications from bugzilla and just setting 
> es->line_height = 1 and es->char_width = 1 in NCCreate is sufficient, 
> since all other content is zero anyway and proper value is set during 
> WM_CREATE.
>
> So a less intrusive change is possible..

I think we should be aiming at something that's more correct than less 
intrusive.
> Roman
>
> Dne 26. 03. 20 v 16:54 Roman Pišl napsal(a):
> > v2: Introduced helper function.
> > v3: Improved helper function (I really didn't like the previous).
> >
> > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48803
> > Signed-off-by: Roman Pišl
> >
> > dlls/comctl32/edit.c       | 38 ++++++++++++++++--------
> >   dlls/comctl32/tests/edit.c | 59 ++++++++++++++++++++++++++++++++++++++
> >   2 files changed, 85 insertions(+), 12 deletions(-)
> >
> > diff --git a/dlls/comctl32/edit.c b/dlls/comctl32/edit.c
> > index d02d7af7b9..e7a3f9b7f9 100644
> > --- a/dlls/comctl32/edit.c
> > +++ b/dlls/comctl32/edit.c
> > @@ -3724,25 +3724,14 @@ static DWORD get_font_margins(HDC hdc, const 
> > TEXTMETRICW *tm)
> >       return MAKELONG(left, right);
> >   }
> > -/*********************************************************************
> > -
> > -
> > - *    WM_SETFONT
> > -
> > -
> > -
> > - With Win95 look the margins are set to default font value unless
> > -
> > - the system font (font == 0) is being set, in which case they are
> >
> > left
> > -
> > - unchanged.
> > -
> > -
> > - */
> >
> > -static void EDIT_WM_SetFont(EDITSTATE *es, HFONT font, BOOL redraw)
> > +static DWORD set_font(EDITSTATE *es, HFONT font)
> >   {
> >       TEXTMETRICW tm;
> >       HDC dc;
> >       HFONT old_font = 0;
> > -    RECT clientRect;
> >       DWORD margins;
> >         es->font = font;
> > -    EDIT_InvalidateUniscribeData(es);
> >       dc = GetDC(es->hwndSelf);
> >       if (font)
> >           old_font = SelectObject(dc, font);
> > @@ -3754,6 +3743,26 @@ static void EDIT_WM_SetFont(EDITSTATE *es, 
> > HFONT font, BOOL redraw)
> >           SelectObject(dc, old_font);
> >       ReleaseDC(es->hwndSelf, dc);
> >   +    return margins;
> > +}
> > -
> >
> > +/*********************************************************************
> > -
> > -
> > - *    WM_SETFONT
> > -
> > -
> > -
> > - With Win95 look the margins are set to default font value unless
> > -
> > - the system font (font == 0) is being set, in which case they are
> >
> > left
> > -
> > - unchanged.
> > -
> > -
> > - */
> >
> > +static void EDIT_WM_SetFont(EDITSTATE *es, HFONT font, BOOL redraw)
> > +{
> > +    RECT clientRect;
> > +    DWORD margins;
> > -
> >
> > +    EDIT_InvalidateUniscribeData(es);
> > +    margins = set_font(es, font);
> > -
> >
> > /* Reset the format rect and the margins */
> >       GetClientRect(es->hwndSelf, &clientRect);
> >       EDIT_SetRectNP(es, &clientRect);
> > @@ -4488,6 +4497,11 @@ static LRESULT EDIT_WM_NCCreate(HWND hwnd, 
> > LPCREATESTRUCTW lpcs)
> >           else if (es->style & WS_BORDER)
> >           SetWindowLongW(hwnd, GWL_STYLE, es->style & ~WS_BORDER);
> >   +    /*
> > +     * Initialize font metrics - line_height and char_width so that
> > +     * early messages before WM_CREATE don't lead to division by zero.
> > +     */
> > +    set_font(es, 0);
> >       return TRUE;
> >     cleanup:
> > diff --git a/dlls/comctl32/tests/edit.c b/dlls/comctl32/tests/edit.c
> > index cec6025077..3bfbee4ddd 100644
> > --- a/dlls/comctl32/tests/edit.c
> > +++ b/dlls/comctl32/tests/edit.c
> > @@ -1768,6 +1768,64 @@ static BOOL is_font_installed(const char*name)
> >       return ret;
> >   }
> >   +static WNDPROC orig_class_proc;
> > -
> >
> > +static LRESULT CALLBACK test_class_proc(HWND hwnd, UINT message, 
> > WPARAM wParam, LPARAM lParam)
> > +{
> > +    RECT rect;
> > +    LRESULT result;
> > -
> >
> > +    switch (message)
> > +    {
> > +        case WM_NCCREATE:
> > +            result = CallWindowProcA(orig_class_proc, hwnd, message, 
> > wParam, lParam);
> > -
> >
> > +            memset(&rect, 0, sizeof(rect));
> > +            SendMessageA(hwnd, EM_GETRECT, 0, (LPARAM)&rect);
> > +            ok(!rect.right && !rect.bottom, "Invalid size after 
> > NCCREATE: %d x %d\n", rect.right, rect.bottom);
> > -
> >
> > +            /* test that early messages don't crash or cause 
> > unexpected behavior */
> > +            SendMessageA(hwnd, EM_SETSEL, 0, 0);
> > +            SendMessageA(hwnd, WM_SIZE, 0, 0);
> > -
> >
> > +            return result;
> > -
> >
> > +        case WM_CREATE:
> > +            /* test that early messages don't crash or cause 
> > unexpected behavior */
> > +            SendMessageA(hwnd, EM_SETSEL, 0, 0);
> > +            SendMessageA(hwnd, WM_SIZE, 0, 0);
> > -
> >
> > +            break;
> > +    }
> > -
> >
> > +    return CallWindowProcA(orig_class_proc, hwnd, message, wParam, 
> > lParam);
> > +}
> > -
> >
> > +static void test_early_messages(void)
> > +{
> > +    BOOL ret;
> > +    ATOM atom;
> > +    HWND hwEdit;
> > +    WNDCLASSA cls;
> > -
> >
> > +    ret = GetClassInfoA(NULL, "Edit", &cls);
> > +    ok(ret, "Failed to get class info.\n");
> > -
> >
> > +    orig_class_proc = cls.lpfnWndProc;
> > +    cls.lpfnWndProc = test_class_proc;
> > +    cls.lpszClassName = "TestClassName";
> > -
> >
> > +    atom = RegisterClassA(&cls);
> > +    ok(atom != 0, "Failed to register class.\n");
> > -
> >
> > +    hwEdit = CreateWindowExA(0, (LPCSTR)MAKEINTATOM(atom), "Text 
> > Text", ES_MULTILINE | WS_BORDER | ES_AUTOHSCROLL | ES_AUTOVSCROLL,
> > +        10, 10, 300, 300, NULL, NULL, hinst, NULL);
> > +    ok(hwEdit != NULL, "Failed to create a window.\n");
> > -
> >
> > +    DestroyWindow(hwEdit);
> > +    UnregisterClassA((LPCSTR)MAKEINTATOM(atom), hinst);
> > +}
> > -
> >
> > static void test_margins(void)
> >   {
> >       DWORD old_margins, new_margins;
> > @@ -3393,6 +3451,7 @@ START_TEST(edit)
> >       test_edit_control_6();
> >       test_edit_control_limittext();
> >       test_edit_control_scroll();
> > +    test_early_messages();
> >       test_margins();
> >       test_margins_font_change();
> >       test_text_position();

[Roman Pišl]

Dne 26. 03. 20 v 21:24 Nikolay Sivov napsal(a):
> On 3/26/20 10:07 PM, Roman Pišl wrote:
> > Hello,
> > I just tested all affected applications from bugzilla and just setting 
> > es->line_height = 1 and es->char_width = 1 in NCCreate is sufficient, 
> > since all other content is zero anyway and proper value is set during 
> > WM_CREATE.
> >
> > So a less intrusive change is possible..
>
> I think we should be aiming at something that's more correct than less 
> intrusive.

Yes, but with so many divisions in such a spaghetti code, if not 
allowing the divisor to be zero is sufficient to solve all 
division-by-zero long standing issues from bugzilla with a zero 
potential to break anything, that seems to be a good enough solution 
from my point of view..
> > Roman