[PATCH libXft] Remove an extraneous FcPatternDestroy in XftFontOpenInfo

[Jeremy Huddleston]

If that's the case, then this is a terrible API.  If the function returns NULL, the caller has no information about the state of its passed pattern.  It may have been deallocated and NULL returned during 'goto bail' or it may not have been deallocated and NULL returned because of invalid paramaters (currently just info == NULL).

Since the function isn't documented, I wonder if the "fix" would be to copy pattern and write documentation to this effect.  The problem would then be a leak for those expecting it to be freed, but that is certainly preferred, IMO.  Also, the leak is easily fixed by requiring the new libXft and doing their own FcPatternDestroy.

This confusion has caused problems in external projects who expect it to not be freed.  The only non-xorg hit for XftFontOpenInfo in codesearch is actually a long comment debugging this issue and removing their FcPatternDestroy:

http://google.com/codesearch#Lbvu5g5_Qs8/mrxvt-0.5.2/src/main.c&q=XftFontOpenInfo%20lang:%5Ec$&type=cs


On Oct 3, 2011, at 09:53, Alan Coopersmith wrote:

> On 10/ 2/11 02:55 PM, Jeremy Huddleston wrote:
>> 
>> https://bugs.freedesktop.org/show_bug.cgi?id=5745
>> 
>> Signed-off-by: Jeremy Huddleston<jeremyhu at apple.com>
>> ---
>>  src/xftfreetype.c |    1 -
>>  1 files changed, 0 insertions(+), 1 deletions(-)
>> 
>> diff --git a/src/xftfreetype.c b/src/xftfreetype.c
>> index 3f8dfef..1c5967a 100644
>> --- a/src/xftfreetype.c
>> +++ b/src/xftfreetype.c
>> @@ -798,7 +798,6 @@ XftFontOpenInfo (Display	*dpy,
>>  	{
>>  	    if (!font->ref++)
>>  		--info->num_unref_fonts;
>> -	    FcPatternDestroy (pattern);
>>  	    return&font->public;
>>  	}
>> 
> 
> I'm not sure, since if it passes that point, it saves the pattern in
> the newly created font's font->public.pattern and then does the
> FcPatternDestroy of it in XftFontDestroy.
> 
> Perhaps we just need to document that the pattern passed to XftFontOpenInfo
> becomes the exclusive property of Xft and the caller is not allowed to use
> it any more?
> 
> (Though it seems XftFontOpenInfo is missing from the function list in the
> Xft man page, so needs more love than that.)
> 
> -- 
> 	-Alan Coopersmith-        alan.coopersmith at oracle.com
> 	 Oracle Solaris Platform Engineering: X Window System
> 
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
>