[Fontconfig] The reference counter handling in FcConfigGetCurrent() and FcConfigReference().

[mpsuzuki at hiroshima-u.ac.jp]

Hi,

On Thu, 21 Jan 2010 15:32:55 -0500
Behdad Esfahbod <behdad at behdad.org> wrote:
>What you are observing is simply that fontconfig is caching one instance of
>FcConfig.  That's by design.  If you want that freed, you can call FcFini().

Hmm, if an application links multiple libraries that some
of them call FcConfigGetCurrent() or FcInit() by themselves
(so FcConfigGetCurrent() or FcInit() are called multiply),
FcFini() should be called by the application to restrict
only one freeing? Or, all libraries can call FcFini() by
their own scope?

As both of FcInit() and FcConfigCurrent() does not increment
the reference counter, I'm afraid that multiple calls of
FcFini() can cause freeing the object in use.

Regards,
mpsuzuki

>That's not a *leak*.  A leak is memory lost.  Forever.
>
>behdad
>
>On 01/21/2010 06:27 AM, mpsuzuki at hiroshima-u.ac.jp wrote:
>> Dear fontconfig maintainers,
>> 
>> FcConfig object includes its reference counter, it prevents
>> that FcConfigDestroy() frees an object in use. I think it's
>> good design.
>> 
>> In the current implementation, I'm questionable about 2 points:
>> 
>> P1) FcConfigGetCurrent() does not increment the reference
>>     counter when it returns existing FcConfig object.
>> 
>> P2) FcConfigReference() increments the reference counter
>>     even when it returns new object created by FcConfigGetCurrent().
>> 
>> By P1, some libraries linking other libraries which uses
>> fontconfig too cannot free FcConfig object safely.
>> 
>> For example, poppler links fontconfig by itself (to import
>> font resources out of the document), and sometimes it links
>> cairo (to render PDF to scalable graphics) which links
>> fontconfig too.
>> 
>> The poppler invokes FcConfigGetCurrent() when it reads
>> given PDF document, and sometimes cairo invoked by poppler
>> invokes FcConfigGetCurrent() again, to write an output file.
>> Unfortunately, the dependency of poppler onto cairo is
>> not essential, so, poppler does not pass FcConfig object
>> to cairo. Also no cairo API takes FcConfig object from
>> the caller.
>> 
>> How we can free the object obtained by FcConfigGetCurrent()?
>> In current implementation, we cannot know how many clients
>> invoked FcConfigGetCurrent() from FcConfig->ref, so invoking
>> FcConfigDestroy() is not safe. As a result, poppler does not
>> invoke FcConfigDestroy() at all, although poppler frees an
>> object including the reference to FcConfig obtained by
>> FcConfigGetCurrent(). So, valgrind finds a memory leak in
>> some programs. See recent discussion in poppler mail list:
>> 
>> http://lists.freedesktop.org/archives/poppler/2010-January/005429.html
>> http://lists.freedesktop.org/archives/poppler/2010-January/005446.html
>> http://lists.freedesktop.org/archives/poppler/2010-January/005453.html
>> 
>> FcConfigReference() looks slightly better about its handling
>> of reference counter. Invoking FcConfigDestroy() for the
>> object obtained by FcConfigReference( NULL ) is safer than
>> the object obtained by FcConfigGetCurrent(). However, by P2,
>> the minimum reference counter of the object obtained by
>> FcConfigGetCurrent( NULL ) is 2, not 1. FcConfigGetCurrent()
>> sets the reference counter to 1, and FcConfigReference()
>> increments it. As a result,
>>   - Invoke FcConfigReference( NULL ) once
>>   - Invoke FcConfigDestroy( config ) twice
>> is required for real freeing of the FcConfig object. This is
>> not intuitive.
>> 
>> In summary, I want to propose following modifications:
>> 
>> M1) FcConfigGetCurrent() increments the reference counter
>>     when it returns existing object.
>> 
>> M2) FcConfigDestroy() does not increment the reference
>>     counter when it is invoked with NULL pointer and
>>     FcConfigGetCurrent() is invoked.
>> 
>> Please give me comment.
>> 
>> Regards,
>> mpsuzuki
>> _______________________________________________
>> Fontconfig mailing list
>> Fontconfig at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/fontconfig
>>