fontforge/fontforge — pull request #480 by mskala (2013-03-26 20:08:28+00:00)
Title: Memory leak in script Close()

At present, FontViewRemove checks whether the font view being removed is the only one loaded; and if so, it doesn't actually free the memory. There is an explanatory comment that "Freeing a large font can take forever, and if we're just going to exit there's no real reason to do so..." That's okay in a GUI session, because closing the last open font implicitly causes the program to exit. However, this function is called by the scripting Close() command. Scripts can execute in a state of "no font loaded" and can enter and leave that state without exiting FontForge, any number of times. The current code leaks the entire loaded font's memory (which may be tens of megabytes) on every Close() of the only loaded font. Consider a script that opens a large font, closes it, opens it again, closes it, and so on, many times.

Even on my largest fonts (Korean fonts with roughly 13000 complicated glyphs), the amount of time saved by skipping the deallocation is only a second or two, not "forever." It would be less on most fonts. Also note that you only save this for the one font that happens to be closed last, even if many others, and larger, happened to be loaded at the time - all the others but the last, with the current code, have to be deallocated the hard way.

Exacerbating factor: because Revert() doesn't work, script programmers may be inclined to do "Close(); Open();" to simulate its effect, increasing the likelihood of triggering the memory leak.

Workaround for scripts that must work with unpatched FontForge: open a small dummy font at the start of the script, and keep it open throughout, so that you never actually enter the "no fonts loaded" state after that point.

In the future, it may not be just scripts that will leak memory because if we ever comply with standard user interface guidelines, we may allow the program to remain open when there's no font loaded. Then closing the last font to return to the "no font loaded" state will leak that font's memory.

This patch deletes the test for "is this the last font open?" and makes it really deallocate the memory every time. That may make exiting slower. An alternative would be to add a further check for "are we in the GUI?" and skip the free operation if and only if it's the last font AND we are in the GUI. But keeping the deallocation skip in GUI mode will break if in the future the GUI allows closing the last font without exiting; it's only a tiny performance issue anyway; and failing to deallocate the memory because we think we know better already screws up stuff like valgrind memory leak checking. I think this optimization of skipping the deallocation, is a foolish one.

If someone HAD to have the deallocation skip behaviour, the right way to do it would be to test "Are we in the process of exiting the program?" not "Are we in the GUI?", and skip deallocation when in the process of exiting. That could be made more global than just this one check - when we are committed to exit, set a flag that says "don't bother freeing memory anymore," and then have all freeing go through a wrapper that checks that flag. I think this would be a silly thing to do (freeing memory isn't THAT expensive) but it would be the right way to implement such a feature if we thought we needed it.