Matthias, do you any comments on how something like this should be implemented correctly?

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

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

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

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.

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.

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

Thanks Owen.

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.

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.

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.

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

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.