[PATCH] gdiplus: Use refcounting for GpFontFamily instead of cloning.

[Dmitry Timoshkov]

.Net 4.7+ depends on this behaviour and expects to be able to do pointer
equality tests for FontFamily objects.

Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru>
---
 dlls/gdiplus/font.c            | 62 ++++++++++++++--------------------
 dlls/gdiplus/gdiplus_private.h |  1 +
 dlls/gdiplus/tests/font.c      | 57 ++++++++++++++++++++++++++++++-
 3 files changed, 83 insertions(+), 37 deletions(-)

diff --git a/dlls/gdiplus/font.c b/dlls/gdiplus/font.c
index eee272082f..2266e474df 100644
--- a/dlls/gdiplus/font.c
+++ b/dlls/gdiplus/font.c
@@ -115,8 +115,6 @@ typedef struct
 #define MS_OS2_TAG MS_MAKE_TAG('O','S','/','2')
 #define MS_HHEA_TAG MS_MAKE_TAG('h','h','e','a')
 
-static GpStatus clone_font_family(const GpFontFamily *, GpFontFamily **);
-
 static GpFontCollection installedFontCollection = {0};
 
 /*******************************************************************************
@@ -183,13 +181,8 @@ GpStatus WINGDIPAPI GdipCreateFont(GDIPCONST GpFontFamily *fontFamily,
     (*font)->unit = unit;
     (*font)->emSize = emSize;
     (*font)->otm = otm;
-
-    stat = clone_font_family(fontFamily, &(*font)->family);
-    if (stat != Ok)
-    {
-        heap_free(*font);
-        return stat;
-    }
+    (*font)->family = (GpFontFamily *)fontFamily;
+    InterlockedIncrement(&(*font)->family->ref);
 
     TRACE("<-- %p\n", *font);
 
@@ -322,7 +315,10 @@ GpStatus WINGDIPAPI GdipGetFamily(GpFont *font, GpFontFamily **family)
     if (!(font && family))
         return InvalidParameter;
 
-    return GdipCloneFontFamily(font->family, family);
+    InterlockedIncrement(&font->family->ref);
+    *family = font->family;
+
+    return Ok;
 }
 
 static REAL get_font_size(const GpFont *font)
@@ -518,8 +514,6 @@ GpStatus WINGDIPAPI GdipGetLogFontW(GpFont *font, GpGraphics *graphics, LOGFONTW
  */
 GpStatus WINGDIPAPI GdipCloneFont(GpFont *font, GpFont **cloneFont)
 {
-    GpStatus stat;
-
     TRACE("(%p, %p)\n", font, cloneFont);
 
     if(!font || !cloneFont)
@@ -528,11 +522,10 @@ GpStatus WINGDIPAPI GdipCloneFont(GpFont *font, GpFont **cloneFont)
     *cloneFont = heap_alloc_zero(sizeof(GpFont));
     if(!*cloneFont)    return OutOfMemory;
 
+    InterlockedIncrement(&font->family->ref);
     **cloneFont = *font;
-    stat = GdipCloneFontFamily(font->family, &(*cloneFont)->family);
-    if (stat != Ok) heap_free(*cloneFont);
 
-    return stat;
+    return Ok;
 }
 
 /*******************************************************************************
@@ -769,6 +762,7 @@ GpStatus WINGDIPAPI GdipCreateFontFamilyFromName(GDIPCONST WCHAR *name,
     ffamily->descent = fm.descent;
     ffamily->line_spacing = fm.line_spacing;
     ffamily->dpi = fm.dpi;
+    ffamily->ref = 1;
 
     *FontFamily = ffamily;
 
@@ -777,16 +771,6 @@ GpStatus WINGDIPAPI GdipCreateFontFamilyFromName(GDIPCONST WCHAR *name,
     return Ok;
 }
 
-static GpStatus clone_font_family(const GpFontFamily *family, GpFontFamily **clone)
-{
-    *clone = heap_alloc_zero(sizeof(GpFontFamily));
-    if (!*clone) return OutOfMemory;
-
-    **clone = *family;
-
-    return Ok;
-}
-
 /*******************************************************************************
  * GdipCloneFontFamily [GDIPLUS.@]
  *
@@ -799,19 +783,19 @@ static GpStatus clone_font_family(const GpFontFamily *family, GpFontFamily **clo
  * RETURNS
  *  SUCCESS: Ok
  */
-GpStatus WINGDIPAPI GdipCloneFontFamily(GpFontFamily* FontFamily, GpFontFamily** clonedFontFamily)
+GpStatus WINGDIPAPI GdipCloneFontFamily(GpFontFamily *family, GpFontFamily **clone)
 {
-    GpStatus status;
+    if (!family || !clone) return InvalidParameter;
 
-    if (!(FontFamily && clonedFontFamily)) return InvalidParameter;
+    TRACE("%p (%s), %p\n", family, debugstr_w(family->FamilyName), clone);
 
-    TRACE("%p (%s), %p\n", FontFamily,
-            debugstr_w(FontFamily->FamilyName), clonedFontFamily);
+    *clone = heap_alloc_zero(sizeof(**clone));
+    if (!*clone) return OutOfMemory;
 
-    status = clone_font_family(FontFamily, clonedFontFamily);
-    if (status != Ok) return status;
+    **clone = *family;
+    (*clone)->ref = 1;
 
-    TRACE("<-- %p\n", *clonedFontFamily);
+    TRACE("<-- %p\n", *clone);
 
     return Ok;
 }
@@ -867,11 +851,17 @@ GpStatus WINGDIPAPI GdipGetFamilyName (GDIPCONST GpFontFamily *family,
  */
 GpStatus WINGDIPAPI GdipDeleteFontFamily(GpFontFamily *FontFamily)
 {
-    if (!FontFamily)
+    LONG ref;
+
+    if (!FontFamily || !FontFamily->ref)
         return InvalidParameter;
-    TRACE("Deleting %p (%s)\n", FontFamily, debugstr_w(FontFamily->FamilyName));
 
-    heap_free (FontFamily);
+    ref = InterlockedDecrement(&FontFamily->ref);
+    if (!ref)
+    {
+        TRACE("Deleting %p (%s)\n", FontFamily, debugstr_w(FontFamily->FamilyName));
+        heap_free(FontFamily);
+    }
 
     return Ok;
 }
diff --git a/dlls/gdiplus/gdiplus_private.h b/dlls/gdiplus/gdiplus_private.h
index 8c4fcceded..6b48c360e6 100644
--- a/dlls/gdiplus/gdiplus_private.h
+++ b/dlls/gdiplus/gdiplus_private.h
@@ -518,6 +518,7 @@ struct GpFontCollection{
 };
 
 struct GpFontFamily{
+    LONG ref;
     WCHAR FamilyName[LF_FACESIZE];
     UINT16 em_height, ascent, descent, line_spacing; /* in font units */
     int dpi;
diff --git a/dlls/gdiplus/tests/font.c b/dlls/gdiplus/tests/font.c
index 33b75c5bc5..3cada58e39 100644
--- a/dlls/gdiplus/tests/font.c
+++ b/dlls/gdiplus/tests/font.c
@@ -141,7 +141,6 @@ static void test_createfont(void)
     expect(Ok, stat);
     stat = GdipGetFamilyName(fontfamily2, familyname, 0);
     expect(Ok, stat);
-todo_wine
     ok (fontfamily == fontfamily2, "Unexpected family instance.\n");
     ok (lstrcmpiW(Tahoma, familyname) == 0, "Expected Tahoma, got %s\n",
             wine_dbgstr_w(familyname));
@@ -1282,6 +1281,61 @@ static void test_GdipGetFontCollectionFamilyCount(void)
     ok(status == InvalidParameter, "Unexpected status %d.\n", status);
 }
 
+static void test_CloneFont(void)
+{
+    GpStatus status;
+    GpFont *font, *font2;
+    GpFontFamily *family, *family2;
+    REAL height;
+    Unit unit;
+    int style;
+
+    status = GdipCreateFontFamilyFromName(Tahoma, NULL, &family);
+    expect(Ok, status);
+
+    status = GdipCreateFont(family, 30.0f, FontStyleRegular, UnitPixel, &font);
+    expect(Ok, status);
+
+    status = GdipGetFontUnit(font, &unit);
+    expect(Ok, status);
+    ok(unit == UnitPixel, "got %u\n", unit);
+
+    status = GdipGetFontSize(font, &height);
+    expect(Ok, status);
+    ok(height == 30.0f, "got %f\n", height);
+
+    status = GdipGetFontStyle(font, &style);
+    expect(Ok, status);
+    ok(style == FontStyleRegular, "got %d\n", style);
+
+    status = GdipGetFamily(font, &family2);
+    expect(Ok, status);
+    ok(family == family2, "got %p\n", family2);
+
+    status = GdipCloneFont(font, &font2);
+    expect(Ok, status);
+
+    status = GdipGetFontUnit(font2, &unit);
+    expect(Ok, status);
+    ok(unit == UnitPixel, "got %u\n", unit);
+
+    status = GdipGetFontSize(font2, &height);
+    expect(Ok, status);
+    ok(height == 30.0f, "got %f\n", height);
+
+    status = GdipGetFontStyle(font2, &style);
+    expect(Ok, status);
+    ok(style == FontStyleRegular, "got %d\n", style);
+
+    status = GdipGetFamily(font2, &family2);
+    expect(Ok, status);
+    ok(family == family2, "got %p\n", family2);
+
+    GdipDeleteFont(font2);
+    GdipDeleteFont(font);
+    GdipDeleteFontFamily(family);
+}
+
 START_TEST(font)
 {
     struct GdiplusStartupInput gdiplusStartupInput;
@@ -1301,6 +1355,7 @@ START_TEST(font)
 
     GdiplusStartup(&gdiplusToken, &gdiplusStartupInput, NULL);
 
+    test_CloneFont();
     test_long_name();
     test_font_transform();
     test_font_substitution();
-- 
2.20.1

[Vincent Povirk (they/them)]

I'm not convinced this requires reference counting. Maybe FontFamily
objects only exist as part of a FontCollection, and their lifetime is
based on that.

[Dmitry Timoshkov]

"Vincent Povirk (they/them)" <vincent(a)codeweavers.com> wrote:
> I'm not convinced this requires reference counting. Maybe FontFamily
> objects only exist as part of a FontCollection, and their lifetime is
> based on that.

I thought about that, however I think that GdipCloneFontFamily needs to
allocate a copy and in order to avoid a memory leak GdipDeleteFontFamily
should be able to free it, and reference counting solves this problem.

-- 
Dmitry.

[Vincent Povirk (they/them)]

The only test we currently have for GdipCloneFontFamily says that the
returned pointer is equal.