[Mono-dev] COM Interop Remoting Patch

[Lluis Sanchez]

> Hello,
>      Attached is a patch to fix remoting involving COM Interop types.
> I had previsouly broken this unintentionally. COM Interop uses
> transparent proxies, and there was lots of places that didn't
> distinguish between a remoting transparent proxy and a COM Interop
> transparent proxy. To distinguish I check to see if the real proxy
> associated with the transparent proxy is a COM Interop proxy. 
> 
> The main cases I needed to handle was the casting of transparent proxy
> objects, method calls, and field access. For the casting of
> transparent proxies, I added a new field to the MonoRemoteClass for
> the COM Interop vtable. I was previously overwriting the
> default_vtable. For method calls, and probably the most important
> thing to notice, is that I added another check to
> mono_marshal_get_remoting_invoke_with_check to see if a transparent
> proxy is used for COM Interop. This does add some overhead to
> remoting; the overhead should always be emitting a method that simply
> loads all the arguments and calls the wrapped method on the underlying
> managed object the proxy wraps. No real COM Interop code or
> marshalling code should unless the type has a ComImport attribute.
> Unless COM is used the method should never be called, only emitted.
> 
> The field access was done via checks in mono_load_remote_field,
> mono_load_remote_field_new, mono_store_remote_field, and
> mono_store_remote_field_new. This avoids emitting any additional
> checks or methods for remoting wrappers. 
> 
> Please make any suggestions if my changes could be done in a better
> way; especially if my changes related to methods should be more like
> the ones for fields, or vice versa.

The patch looks in general ok to me. I only have a couple of comments:


> Index: mono/mono/metadata/object.c
> ===================================================================
> --- mono/mono/metadata/object.c (revision 75279)
> +++ mono/mono/metadata/object.c (working copy)


> @@ -2589,6 +2598,15 @@
>                 return obj;
>         } else {
>                 /* obj must be already unboxed if needed */
> +               if (obj && mono_object_class(obj) ==
> mono_defaults.transparent_proxy_class) {
> +                       /* Get wrapper method if object is a
> transparent proxy for com interop. 
> +                        * There was no check here for a remoting
> transparent proxy, so I assume that case does
> +                        * not happen... 
> +                        */

This branch of the condition is executed when calling a method which is
not a constructor. I think that it does happen for remote methods, so it
would be good to really check it. Maybe you are adding this com check in
the wrong place.

> +                       MonoTransparentProxy* transparent_proxy =
> (MonoTransparentProxy*)obj;
> +                       if
> (transparent_proxy->rp->object.vtable->klass ==
> mono_defaults.com_interop_proxy_class)
> +                               method =
> mono_marshal_get_cominterop_invoke (method->slot == -1 ? method :
> method->klass->vtable [method->slot]);
> +               }
>                 return mono_runtime_invoke (method, obj, pa, exc);
>         }
>  }
> @@ -4103,8 +4121,12 @@
>         }
>  
>         if (target && target->vtable->klass ==
> mono_defaults.transparent_proxy_class) {
> +               MonoTransparentProxy* transparent_proxy =
> (MonoTransparentProxy*)target;
>                 g_assert (method);
> -               method = mono_marshal_get_remoting_invoke (method);
> +               if (transparent_proxy->rp->object.vtable->klass ==
> mono_defaults.com_interop_proxy_class)
> +                       method = mono_marshal_get_cominterop_invoke
> (method);
> +               else
> +                       method = mono_marshal_get_remoting_invoke
> (method);

Since this check is done in several places, maybe it would be good to
introduce a new method which takes the proxy and the method as
parameters, and returns the correct wrapper method depending on the
proxy type.

>                 delegate->method_ptr = mono_compile_method (method);
>                 MONO_OBJECT_SETREF (delegate, target, target);
>         } else if (mono_method_signature (method)->hasthis &&
> method->klass->valuetype) {
> @@ -4284,6 +4306,12 @@
>         g_assert (this->vtable->klass ==
> mono_defaults.transparent_proxy_class);
>         g_assert (res != NULL);
>  
> +       if (tp->rp->object.vtable->klass ==
> mono_defaults.com_interop_proxy_class) {

Would it make sense to define a macro for checking if a transparent
proxy is a com object? it is being used in several places. 

Thanks,
Lluis.