ref-counting issue ...

[Michael Meeks <michael ximian com>]

Hi guys,

	I though I'd post my thoughts on my latest issue, to garner
some moral support before committing this.

	Essentialy, we have coalesced the CORBA_Object with the
BonoboObject, and this reveals some leaks that have been hidden
until now by the ORB keeping track of references.

	So, the primary issues is that CORBA has no real lifecycle
management.

	Consequently if you want to hand out references to a server,
you must CORBA_Object_duplicate the references you pass out, however
what to do with the lifecycle of the initial reference - that which
you associate with the object when it is created - is not clear.

	So, to overcome this problem, the bonobo ref-counting layer is
built on top of CORBA, and when an aggregates' bonobo ref count drops
to 0, all its Gtk+ objects are destroyed, however at this stage there
are still a number of CORBA object references associated, 1 per
interface. When the bonobo ref count drops to 0 the CORBA object
references are all released.

	So, there are several issues - primarily affecting the local
case, since releasing a non-existant remote object is painless. So,
consider:

	BonoboObject *o = create_object ();
	bonobo_object_unref (o);

	Needs to destroy the associated CORBA resources,

	BonoboObject *o = create_object ();
	bonobo_object_release_unref (BONOBO_OBJREF (o), NULL);

	Similarly needs to destroy the associated CORBA resources,

	However, if we examine bonobo_object_release_unref, we see
that it does:

	Bonobo_Unknown_unref (object, ev);
	CORBA_Object_release (object, ev);

	The issue being that with a local object on its last ref,
we release the built in reference in the Unknown_unref leaving
the CORBA_Object_release to operate on an invalid object - segv.

	Essentialy the problem is created by the mix of:

	bonobo_object_ref, bonobo_object_unref and
	bonobo_object_dup_ref, bonobo_object_release_unref

	As an internal implementation detail ORBit maintains its own
ref count, but note that this is not == the bonobo ref count except
for the corner case of a single interface in the aggregate.

	As long as this is understood, the evilness of not only 2
reference counting schemes, but 2 different ways of fiddling with the
bonobo reference, is manageable.

	However, there is no real reason why:

	control = bonobo_control_new (gtk_entry_new ());
	bonobo_object_release_unref (BONOBO_OBJREF (control), NULL);

	should crash, since it is quite possible to add a patch like
this:

	Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/bonobo/ChangeLog,v
retrieving revision 1.1045
diff -u -r1.1045 ChangeLog
--- ChangeLog	2001/04/24 21:29:29	1.1045
+++ ChangeLog	2001/04/24 21:55:13
@@ -1,5 +1,10 @@
 2001-04-24  Michael Meeks  <michael ximian com>
 
+	* bonobo/bonobo-object.c (bonobo_object_release_unref):
+	skip unrefs if we are local and <= 1 ref remains.
+
+2001-04-24  Michael Meeks  <michael ximian com>
+
 	* bonobo/bonobo-xobject.c (bonobo_x_object_instance_init):
 	re-order debug.
 
Index: bonobo/bonobo-object.c
===================================================================
RCS file: /cvs/gnome/bonobo/bonobo/bonobo-object.c,v
retrieving revision 1.104
diff -u -r1.104 bonobo-object.c
--- bonobo/bonobo-object.c	2001/04/12 16:32:56	1.104
+++ bonobo/bonobo-object.c	2001/04/24 21:55:13
@@ -424,6 +424,7 @@
 			     CORBA_Environment *ev)
 {
 	CORBA_Environment tmpev, *rev;
+	gboolean          skip_unref;
 
 	if (object == CORBA_OBJECT_NIL)
 		return;
@@ -435,8 +436,14 @@
 		CORBA_exception_init (rev);
 	}
 
+	if (BONOBO_OBJECT_IS_LOCAL (object))
+		skip_unref = (ORBIT_ROOT_OBJECT (object)->refs <= 1);
+	else
+		skip_unref = FALSE;
+
 	Bonobo_Unknown_unref (object, rev);
-	CORBA_Object_release (object, rev);
+	if (!skip_unref)
+		CORBA_Object_release (object, rev);
 
 	if (!ev)
 		CORBA_exception_free (&tmpev);
Index: bonobo/bonobo-object.h
===================================================================
RCS file: /cvs/gnome/bonobo/bonobo/bonobo-object.h,v
retrieving revision 1.55
diff -u -r1.55 bonobo-object.h
--- bonobo/bonobo-object.h	2001/03/16 11:29:31	1.55
+++ bonobo/bonobo-object.h	2001/04/24 21:55:13
@@ -26,6 +26,7 @@
 #define BONOBO_IS_OBJECT_CLASS(k) (GTK_CHECK_CLASS_TYPE ((k),
BONOBO_OBJECT_TYPE))
 
 #define
BONOBO_OBJREF(o)          (bonobo_object_corba_objref(BONOBO_OBJECT(o)))
+#define BONOBO_OBJECT_IS_LOCAL(o) ((o)->servant && (o)->vepv)
 
 /*
  * If you're using a custom servant for your CORBA objects, just make


	So, I plan to commit the above shortly, unless anyone screams.
I see no reason why they should - but ref counting is always tricky,
and I've probably lost it, again. Either way, the fix makes me pretty
queasy just looking at it - yet another argument for tighter component
/ orb integration.

	Regards,

		Michael.

-- 
 mmeeks gnu org  <><, Pseudo Engineer, itinerant idiot