Fontconfig - Jan 2010 - The reference counter handling in FcConfigGetCurrent() and FcConfigReference().

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

Hi,

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().
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
>

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
>>

On Fri, 22 Jan 2010 06:57:39 +0900, mpsuzuki at hiroshima-u.ac.jp
wrote:
> 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?
It would be a very bad design if any library attempted to finalize
another shared resource at a random point in time, for precisely the
reasons you have outlined.

Cleaning up of global state can only be performed by the application once
it has guaranteed that those resources are no longer in use - typically
during the shutdown of the last thread, primarily for the purposes of leak
checking.
-ickle

-- 
Chris Wilson, Intel Open Source Technology Centre

On 01/21/2010 04:57 PM, mpsuzuki at hiroshima-u.ac.jp
wrote:
> 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?
Yes, FcFini() may only be called by applications, not libraries.

behdad