Fix createFontMem to not use garbage-collected memory

[klausweiss]

I've had problems when using createFontMem. I didn't use the action directly, but via the monomer library (see the related PR there - fjvallarino/monomer#199 (comment)). Long story short, text wouldn't render when I used createFontMem but would with createFont and the same file.

What I believe is happening: while the font is in memory when fonsAddFontMem is called, it very likely is garbage-collected after the FFI call finishes. The freed memory is reused for another purpose by the Haskell runtime, but from the C point of view the pointer is still valid. When the font is to be used, the same pointer is used (now pointing to gibberish) and the text isn't rendered - garbage in, garbage out.

I tried proving it, but ended up in the FreeType internals which was just a bit too much for me. I have a non-functional monomer example though, which the changes here fix (see the linked PR).

---

The binding tells fontstash that it manages the memory of the font bytes on its own (by passing 0 to the freeData argument):

nanovg-hs/src/NanoVG/Internal/Text.chs

Lines 107 to 112 in cc8dfa0

	-- | Creates image by loading it from the specified memory chunk.
	-- Returns handle to the font.
	{#fun unsafe nvgCreateFontMem as createFontMem
	{`Context',withCString*`T.Text',useAsCStringLen'*`ByteString'&,zero-`CInt'} -> `Maybe Font'safeFont#}

The solution is to tell fontstash to manage the memory allocated for font bytes and to malloc it manually, so that it's not garbage-collected. Note that's also what fonsAddFont does -

nanovg-hs/nanovg/src/fontstash.h

Lines 922 to 929 in cc8dfa0

	data = (unsigned char*)malloc(dataSize);
	if (data == NULL) goto error;
	readed = fread(data, 1, dataSize, fp);
	fclose(fp);
	fp = 0;
	if (readed != (size_t)dataSize) goto error;
	return fonsAddFontMem(stash, name, data, dataSize, 1, fontIndex);

Happy to hear your thoughts.

[cocreature]

Nice, thank you! Looked through the nanovg & freetype and this was definitely buggy, good catch! Two minor comments to address before merging.

[cocreature]

Can you please add a comment that this is freed by NanovVG as part of nvgDeleteGL3?

[klausweiss]

Done. Feel free to change it to your liking if you think it could be improved.

Also thanks for maintenance and looking at it rather promptly!

[klausweiss]

Please excuse me this gentle nudge, but I was wondering if there was a chance for this fix to land in a hackage release soonish. I was looking into using the fixed version in monomer. I understand it may not be a priority though.

[cocreature]

I just uploaded 0.8.1.0 to hackage which includes your fix.

[klausweiss]

Thank you so much!

[cocreature]

np, thanks for the fix!

[Henry-Johnson-100]

I've been using monomer as well and have been tracking down a large memory leak with images for a while now. I eventually traced it back to this PR. I don't want to reverse this PR but it should be known that the default method of rendering an image with monomer uses createImageRGBA which has its own call to useAsPtr which copies the image data passed to it with no obvious method of freeing this copy. I've been trying to think of a good way fix this issue without adding back in GC to these FFI helpers and I think the only way may be to expose some of the internal generated functions like createImageRGBA'_ along with the helpers like useAsPtr so a manual free can be added to them.

For instance, I have had plenty of success just doing something like:

image cxt ch cw flags imgData = useAsPtr imgData $ \p -> do
  image' <- createImageRGBA'_ cxt ch cw (bitMask flags) p
  free p
  return . safeImage $ image'

If it is the case that deleteGL3 is meant to free this copy then I can accept that too to make a PR over at the monomer library.

[klausweiss]

Ah, I was wrong with my comment here then, sorry.

nanovg-hs/src/NanoVG/Internal/FFIHelpers.hs

Line 32 in 283516a

-- In the case of 'createFontMem' and 'createFontMemAtIndex' (the only places using it)

Totally missed nvgCreateImageMem

nanovg-hs/src/NanoVG/Internal/Image.chs

Lines 32 to 33 in 283516a

	{#fun unsafe nvgCreateImageMem as createImageMem
	{`Context',bitMask`S.Set ImageFlags',useAsCStringLen'*`ByteString'&} -> `Maybe Image'safeImage#}

If the same useAsCStringLen' remains in use for both fonts and images, could you update the comment to mention that, please? 🙏

[Henry-Johnson-100]

Sure thing, I submitted a PR #20 for useAsPtr but I missed createImageMem too. I think the fact that the FFI helpers are used relatively sparsely and that we now seem to have divergent use cases for how their pointers are handled means that it might be worth looking into just having Text and Image modules keep their own definitions of these functions.

[Henry-Johnson-100]

I have another question after thinking about this for some time, and maybe you've answered this elsewhere already @klausweiss, was using a StablePtr considered? I do not have a lot of expertise with how Haskell manages memory but StablePtr documentation says that it is guaranteed not to be deallocated and that its value will not change. Would it be possible to wrap the font data in a StablePtr in monomer before passing it to any bindings in this library?

[Henry-Johnson-100]

Or perhaps doing this to copy the bytestring in Monomer before passing it here.

copyAllocateByteString :: ByteString -> IO ByteString
copyAllocateByteString bs =
  useAsCStringLen bs $ \(cch, l) -> do
    let intLen = fromIntegral l
    to <- mallocBytes intLen
    copyBytes to cch intLen
    packCStringLen (to, l)

[klausweiss]

I have another question after thinking about this for some time, and maybe you've answered this elsewhere already @klausweiss, was using a StablePtr considered? I do not have a lot of expertise with how Haskell manages memory but StablePtr documentation says that it is guaranteed not to be deallocated and that its value will not change.

To answer your question, I've never heard of StablePtr before, haven't considered it. Adding that to my to read list ;)