[Mono-dev] COM Callable Wrapper Patch

[Paolo Molaro]

On 12/04/06 Jon Chambers wrote:
> >You can't insert a managed object inside a GHashTable, since objects
> >can move and the GC can't see inside GHashTable, you'll get a broken
> >pointer and a crash. If the hash must not keep alive the object and you
> >used a GHashTable as a way to implement weak references, you must use
> >the runtime support for weak references instead. Also, remember that for
> >object hashes you must use the mono_object_hash() function.
> 
> 
> The object is not put in hashtable, but just used as the key in this case. I
> changed the hash table to use mono_object_hash() as the hash function where
> the object is the key. I've removed the MonoObject* field from the MonoCCW
> struct and just look it up using the gc_handle when needed instead.

The key can change if it is an object address, so you may end up not
finding the value from a previous object or finding it for an object
that wasn't registered yet.

> >Can you explain this MonoCCW data structure here? It has a pointer
> >to a reference which isn't GC-tracked and so it's bad. It looks to be
> >created with a 0 refcount and an handle is created for the object, so
> >there doesn't seem to be any need to store also an object in it.
> >Also, the reference count increase is done in a thread-unsafe way.
> 
> 
> I've removed the object reference. I only allocate a weak handle and set the
> reference count to 0. If the unmanaged client code decides to addref and
> hold onto the CCW, I then allocate a strong handle. Once the reference count
> goes back to 0 I go back to a weak handle.

Ok, please add the description as a comment.

> >> +     if (mono_marshal_free_ccw (o) && !finalizer)
> >> +             return;
> >
> >So what happens if the object has a finalizer and wants to use COM
> >features in it? Is that not possible? We need a comment here.
> >You may also want to check what happens with resurrection (ie the
> >objects stores itself in a static field inside the finalizer).
> 
> 
> Added better comment. If an object has a CCW associated with it and has no
> finalizer we can safely return after destroying the CCW since we are only
> being finalized since COM Interop registered the object for finalization to
> clean up the CCW. In all other cases we continue on (including the case you
> brought up where an object has a finalizer and has been exposed to unmanaged
> code via a CCW) to call the objects finalizer. I've added a FIXME for
> resurrection, and also if the user calls SuppressFinalize the CCW will leak
> currently. I see what is done for delegates in that case, but this is a bit
> different. I currently have no way to determine that an object with a
> finalizer needs it's CCW destroyed but don't need it's finalizer run.

For delegates we just check and return in SuppressFinalize.
You could add a similar check there with the usual fast path for when no
com support has been used.

> Also, there are some places I know I need to make threadsafe. Should I
> create a new critical section, or just use the marshal one?

Yes, please.

> Index: mono/mono/metadata/marshal.c
> ===================================================================
> --- mono/mono/metadata/marshal.c	(revision 68977)
> +++ mono/mono/metadata/marshal.c	(working copy)
> @@ -1823,18 +1904,31 @@
> @@ -2102,20 +2196,20 @@
>  	case MONO_MARSHAL_CONV_OBJECT_INTERFACE:
>  	case MONO_MARSHAL_CONV_OBJECT_IDISPATCH:
>  	case MONO_MARSHAL_CONV_OBJECT_IUNKNOWN: {
> -		guint32 pos_failed = 0, pos_rcw = 0;
> -		char * msg;
> +		guint32 pos_null = 0, pos_rcw = 0, pos_end = 0;
>  
> +		/* COM types are initialized lazily */
> +		mono_init_com_types ();
> +
>  		mono_mb_emit_ldloc (mb, 1);
> -		//mono_mb_emit_ldloc (mb, 0);
> -		mono_mb_emit_ptr (mb, 0);
> -		//mono_mb_emit_byte (mb, CEE_LDIND_U1);
> +		mono_mb_emit_byte (mb, CEE_LDNULL);
>  		mono_mb_emit_byte (mb, CEE_STIND_I);

ldnull loads an object and an object can't be stored with stind.i: if
you really need to store a NULL pointer (and not a null reference), load
a 0 constant and convert it with conv.u. Otherwise use stind.ref to
store a reference.

> @@ -3295,6 +3413,20 @@
>  	return res;
>  }
>  
> +/* Maps a managed object to its unmanaged representation 
> + * i.e. it's COM Callable Wrapper (CCW). 
> + * Key: MonoObject*
> + * Value: MonoCCW*
> + */
> +static GHashTable* ccw_hash = NULL;

As explained above, you can't really use an object address as the key,
since it changes over time. One possible implementation idea: use
mono_object_hash() as the key and a list as the value: you then walk the
list and get the object to compare from the GC handle. There may be also
faster ways for sure and if this is accessed during finalizations we
should check that the Boehm GC doesn't null the weak refs before running
the finalizers (but I guess that if you find a null target in the gc
handle you can free the ccw struct right away...)

> +	if (!ccw_entry) {
> +		int vtable_index = method_count-1+start_slot;
> +		mono_loader_lock ();
> +		vtable = mono_mempool_alloc0 (klass->image->mempool, sizeof (gpointer)*(method_count+start_slot));
> +		mono_loader_unlock ();
> +		memcpy(vtable, iunknown, sizeof(iunknown));
> +		if (start_slot == 7)
> +			memcpy(vtable+3, idispatch, sizeof(idispatch));

Put a space before the opening parenthesis of a function call.

> +static gboolean
> +cominterop_class_guid_equal (gpointer guid, MonoClass* klass)
> +{
> +	static MonoClass *GuidAttribute = NULL;
> +	MonoCustomAttrInfo *cinfo;
> +	MonoReflectionGuidAttribute *attr;
> +	MonoString *guid_string = mono_string_new (mono_domain_get (), mono_guid_to_string ((guint8*)guid));

If guid is a guint8*, change the type from gpointer to it wherever it is
used.

Thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better