After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 344766 - Memory leak in get_ruleset() in modules/basic/basic-fc.c
Memory leak in get_ruleset() in modules/basic/basic-fc.c
Status: RESOLVED FIXED
Product: pango
Classification: Platform
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: Behdad Esfahbod
pango-maint
Depends on:
Blocks:
 
 
Reported: 2006-06-13 14:22 UTC by Klaus Stengel
Modified: 2006-07-07 18:02 UTC
See Also:
GNOME target: ---
GNOME version: 2.13/2.14


Attachments
Possible fix (1.40 KB, patch)
2006-06-26 07:54 UTC, Klaus Stengel
none Details | Review
Updated patch (1.42 KB, patch)
2006-06-28 12:40 UTC, Klaus Stengel
none Details | Review

Description Klaus Stengel 2006-06-13 14:22:49 UTC
Please describe the problem:
While working with the diagram editor "dia" I noticed a major memory
leak when editing text objects. I tracked this down to a problem in the
above function. From what I understand the function looks for an ruleset
associated with the given font face and in case there is none, the
ruleset is created with pango_ot_ruleset_new() and finally attached to
the font face. The g_object_unref() is given as "notification callback"
so that the ruleset is destroyed when the font face is finalized.

In theory this should work, but unfortunately it doesn't in practice,
because pango_ot_ruleset_new() internally references "info" itself, thus
creating a circular dependency that keeps the objects alive
indefinitely.

Steps to reproduce:
Use some program that heavily creates and destroys PangoLayout Objects like the diagram editor "dia".

Actual results:
The program takes up more and more memory and finally crashes or is killed by the kernel.

Expected results:
The library shouldn't leak memory.

Does this happen every time?
Yes.

Other information:
Comment 1 Behdad Esfahbod 2006-06-16 20:08:50 UTC
Matthias, do you any comments on how something like this should be implemented correctly?
Comment 2 Matthias Clasen 2006-06-16 23:37:01 UTC
Behdad, I don't claim to fully understand the lifecycles and dependencies
of all involved object, but I don't see a cyclic reference here.

The face has a ref to the info (see pango_ot_info_get)
The face has a ref to the ruleset (see get_ruleset)
The ruleset has a ref to the info (set pango_ot_ruleset_new)

But I don't see the info own a ref to the ruleset
Comment 3 Klaus Stengel 2006-06-17 09:12:01 UTC
(In reply to comment #2)
> Behdad, I don't claim to fully understand the lifecycles and dependencies
> of all involved object, but I don't see a cyclic reference here.
> 
> The face has a ref to the info (see pango_ot_info_get)

Correct.

> The face has a ref to the ruleset (see get_ruleset)

No, and that's the problem... It's not the face itself, but the info object that stores the reference. Look at the last lines of get_ruleset():
g_object_set_qdata_full (G_OBJECT (info), ruleset_quark, ruleset, (GDestroyNotify) g_object_unref);

'info' is obtained from face in the beginning of get_ruleset():
info = pango_ot_info_get (face);

So, info has a ref to the ruleset (attached via GQuark).

> The ruleset has a ref to the info (set pango_ot_ruleset_new)

Correct. This increases info's reference count to 2.
When the face is destroyed, info's ref count never drops to zero, because face references info just once...

> But I don't see the info own a ref to the ruleset

+--------+         +--------+            +---------+
|  Face  |-------->|  Info  |<-----------| Ruleset |
+--------+         +-+------+-+          +---------+
                     | GQuark | . . . . . . . .^
		     +--------+

I hope it's now clear...
Comment 4 Klaus Stengel 2006-06-17 09:29:26 UTC
+--------+         +--------+            +---------+
|  Face  |-------->|  Info  |<-----------| Ruleset |
+--------+         +-+------+-+          +---------+
                     | GQuark |----------------^
		     +--------+

Comment 5 Matthias Clasen 2006-06-18 00:30:52 UTC
Ah, ok. I see the problem now. 

Does it ever make sense to keep the info alive after the face is gone ?

One way to fix this would be to explicitly dispose the info when the
face is gone.
Comment 6 Behdad Esfahbod 2006-06-19 01:13:22 UTC
That's actually a good point.  The info in fact should have been refing the face, but it's not.  Ok, how am I supposed to explicitly dispose the info when the face is gone?  Given that multiple rulesets may have been attached to the info by different modules?  Making ruleset not ref info doesn't sound right to me.

A solution is to make modules attach ruleset to PangoFont instead of info, but I prefer a solution that doesn't change every module.  Not sure if it's possible.
Comment 7 Owen Taylor 2006-06-19 12:09:30 UTC
I don't really agree that the info should be reffing the face.
Leaving aside the fact that FT_Face's aren't refcounted, that would cause
no face to ever be freed. The idea is that the info is cached for the
duration of the face. 

I'd suggest: Simply don't have the ruleset reference the info; after
all using the ruleset once the FT_Face is gone is not OK, even if the
Info is still around. So there is no API change.

Or do the same, but use a weak reference from the ruleset to the info,
to avoid segfaults in favor of critical warnings if someone keeps
the ruleset around past the lifetime of the face and tries to use it.

You can't use g_object_dispose() because that doesn't clear qdata.

You could also try to change how modules attach the ruleset
to the info ... but that seems like more work. (Attaching the ruleset
to the Font, is however wrong, since PangoOTInfo has a reference
to the FT_Face, which has a shorter lifespan than the font... the
face is only guaranteed to be valid while the font is locked.)


Comment 8 Behdad Esfahbod 2006-06-19 15:47:24 UTC
Thanks Owen.
Comment 9 Klaus Stengel 2006-06-26 07:54:03 UTC
Created attachment 68016 [details] [review]
Possible fix

Ok, I created a patch that should fix the problem using weak references, like Owen suggested. I'm not sure what should happen if a ruleset is created with a NULL pointer as PangoOTInfo object, though. Currently the Ruleset is created but not usable... Maybe it should return just a NULL pointer; But this is public API and may confuse programs that expect to get a valid pointer in every case.
Comment 10 Matthias Clasen 2006-06-27 07:35:54 UTC
If the function is not explicitly documented as accepting a NULL parameter,
it should just g_return_val_if_fail (arg != NULL, NULL) and treat this as
a programmeing error.
Comment 11 Klaus Stengel 2006-06-28 12:40:47 UTC
Created attachment 68118 [details] [review]
Updated patch

Thanks Matthias. I updated the patch. It applies cleanly to current CVS now. Older versions should also work (tested with 1.12), even if 'patch' may report some "fuzz". Please test and include it in current CVS and 1.12 branch.
Comment 12 Jordan Erickson 2006-07-07 16:15:24 UTC
Just wanted to drop a note and say 'thank you' for all of your help. I'm not sure if I was the one who originally spotted this bug 'in the wild' but I really do appreciate the collaboration and fixes. I use Dia all the time for my customers and this bug was a show-stopper. Thanks again for everything! You guys rock.

Jordan
Comment 13 Behdad Esfahbod 2006-07-07 17:50:56 UTC
Committed to HEAD and pango-1-12 branch with minor changes.  Thanks.  I replaced ruleset->info != NULL checks with PANGO_IS_OT_INFO (ruleset->info).

Klaus, can you also look into fixing bug 143542?



2006-07-07  Behdad Esfahbod  <behdad@gnome.org>

        Bug 344766 – Memory leak in get_ruleset() in modules/basic/basic-fc.c

        * pango/pango-ot-private.h: Rename PANGO_OT_IS_RULESET is
        PANGO_IS_OT_RULESET.

        * pango/pango-ot-ruleset.c (pango_ot_ruleset_finalize),
        (pango_ot_ruleset_new), (pango_ot_ruleset_add_feature),
        (pango_ot_ruleset_substitute), (pango_ot_ruleset_position):
        Use weak pointers to reference ruleset->info, to avoid circular
        dependency.