[PATCH v2 1/3] uiautomationcore: Implement UiaGetReservedNotSupportedValue.

[Connor McAdams]

On Thu, Oct 28, 2021 at 09:02:40PM +0300, Nikolay Sivov wrote:
> 
> 
> On 10/28/21 6:17 PM, Connor McAdams wrote:
> > Signed-off-by: Connor McAdams <cmcadams at codeweavers.com>
> > ---
> >  dlls/uiautomationcore/Makefile.in |   1 +
> >  dlls/uiautomationcore/uia_main.c  | 147 +++++++++++++++++++++++++++++-
> >  2 files changed, 146 insertions(+), 2 deletions(-)
> >
> > diff --git a/dlls/uiautomationcore/Makefile.in b/dlls/uiautomationcore/Makefile.in
> > index 71ea7b99c94..5a72ea144c4 100644
> > --- a/dlls/uiautomationcore/Makefile.in
> > +++ b/dlls/uiautomationcore/Makefile.in
> > @@ -1,5 +1,6 @@
> >  MODULE = uiautomationcore.dll
> >  IMPORTLIB = uiautomationcore
> > +IMPORTS   = uuid ole32
> >  
> >  EXTRADLLFLAGS = -Wb,--prefer-native
> >  
> > diff --git a/dlls/uiautomationcore/uia_main.c b/dlls/uiautomationcore/uia_main.c
> > index 2dada95af80..51c220c6041 100644
> > --- a/dlls/uiautomationcore/uia_main.c
> > +++ b/dlls/uiautomationcore/uia_main.c
> > @@ -16,12 +16,150 @@
> >   * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> >   */
> >  
> > +#define COBJMACROS
> > +
> >  #include "uiautomation.h"
> >  
> >  #include "wine/debug.h"
> > +#include "wine/heap.h"
> >  
> >  WINE_DEFAULT_DEBUG_CHANNEL(uiautomation);
> >  
> > +struct uia_rsrv_ftmarshaler
> > +{
> > +    IUnknown IUnknown_iface;
> > +    LONG refcount;
> > +
> > +    IUnknown *inner;
> > +    IUnknown *rsrv_val;
> > +};
> 
> I think naming is confusing. 'inner' here really means marshaler,
> rsrv_val is the object marshaler is associated with, and the whole thing
> is not an ftmarshaler, but something like a host or a wrapper.
>

Yeah, I agree the naming is a bit confusing. I was having trouble with
that, but I'll use the name you've suggested in the next version.

> > +
> > +static struct uia_rsrv_ftmarshaler *uia_rsrv_ns_val_ftmarshaler = NULL;
> I thought you said that new instances are created every time?
> > +
> > +static struct uia_rsrv_ftmarshaler *impl_uia_rsrv_ftmarshaler_from_IUnknown(IUnknown *iface)
> > +{
> > +    return CONTAINING_RECORD(iface, struct uia_rsrv_ftmarshaler, IUnknown_iface);
> > +}
> > +
> > +static HRESULT WINAPI uia_rsrv_ftmarshaler_QueryInterface(IUnknown *iface,
> > +        REFIID riid, void **ppv)
> > +{
> > +    struct uia_rsrv_ftmarshaler *marshaler = impl_uia_rsrv_ftmarshaler_from_IUnknown(iface);
> > +
> > +    return IUnknown_QueryInterface(marshaler->rsrv_val, riid, ppv);
> > +}
> > +
> > +static ULONG WINAPI uia_rsrv_ftmarshaler_AddRef(IUnknown *iface)
> > +{
> > +    struct uia_rsrv_ftmarshaler *marshaler = impl_uia_rsrv_ftmarshaler_from_IUnknown(iface);
> > +    ULONG refcount = InterlockedIncrement(&marshaler->refcount);
> > +
> > +    TRACE("%p, refcount %d\n", iface, refcount);
> > +
> > +    return refcount;
> > +}
> > +
> > +static ULONG WINAPI uia_rsrv_ftmarshaler_Release(IUnknown *iface)
> > +{
> > +    struct uia_rsrv_ftmarshaler *marshaler = impl_uia_rsrv_ftmarshaler_from_IUnknown(iface);
> > +    ULONG refcount = InterlockedDecrement(&marshaler->refcount);
> > +
> > +    TRACE("%p, refcount %d\n", iface, refcount);
> > +    if (!refcount)
> > +    {
> > +        if (marshaler == uia_rsrv_ns_val_ftmarshaler)
> > +            uia_rsrv_ns_val_ftmarshaler = NULL;
> > +
> > +        IUnknown_Release(marshaler->inner);
> > +        heap_free(marshaler);
> > +    }
> > +
> > +    return refcount;
> > +}
> > +
> > +static const IUnknownVtbl uia_rsrv_ftmarshaler_vtbl = {
> > +    uia_rsrv_ftmarshaler_QueryInterface,
> > +    uia_rsrv_ftmarshaler_AddRef,
> > +    uia_rsrv_ftmarshaler_Release,
> > +};
> > +
> > +/*
> > + * When passing the ReservedNotSupportedValue/ReservedMixedAttributeValue
> > + * interface pointers across apartments within the same process, create a free
> > + * threaded marshaler so that the pointer value is preserved.
> > + */
> > +static HRESULT uia_rsrv_val_create_ftmarshaler(IUnknown *reserved, struct uia_rsrv_ftmarshaler **marshaler)
> > +{
> > +    struct uia_rsrv_ftmarshaler *object;
> > +    HRESULT hr;
> > +
> > +    TRACE("%p, %p\n", reserved, marshaler);
> > +
> > +    object = heap_alloc(sizeof(*object));
> > +    if (!object)
> > +        return E_OUTOFMEMORY;
> > +
> > +    object->IUnknown_iface.lpVtbl = &uia_rsrv_ftmarshaler_vtbl;
> > +    object->refcount = 1;
> > +
> > +    hr = CoCreateFreeThreadedMarshaler(&object->IUnknown_iface, &object->inner);
> > +    if (FAILED(hr = CoCreateFreeThreadedMarshaler(&object->IUnknown_iface, &object->inner)))
> > +    {
> > +        heap_free(object);
> > +        return hr;
> > +    }
> This one is obviously copy-paste typo.

Oof, yeah, good catch.

> > +
> > +    *marshaler = object;
> > +
> > +    return hr;
> > +}
> > +
> > +/*
> > + * UiaReservedNotSupported object.
> > + */
> > +static HRESULT WINAPI uia_reserved_ns_QueryInterface(IUnknown *iface,
> > +        REFIID riid, void **ppv)
> > +{
> > +    *ppv = NULL;
> > +    if (IsEqualIID(riid, &IID_IUnknown))
> > +        *ppv = iface;
> > +    else if (IsEqualIID(riid, &IID_IMarshal))
> > +    {
> > +        if (!uia_rsrv_ns_val_ftmarshaler)
> > +        {
> > +            if (FAILED(uia_rsrv_val_create_ftmarshaler(iface, &uia_rsrv_ns_val_ftmarshaler)))
> > +                return E_NOINTERFACE;
> > +        }
> > +
> > +        if (FAILED(IUnknown_QueryInterface(uia_rsrv_ns_val_ftmarshaler->inner, riid, ppv)))
> > +            return E_NOINTERFACE;
> > +
> > +        IUnknown_Release(&uia_rsrv_ns_val_ftmarshaler->IUnknown_iface);
> > +    }
> > +    else
> > +        return E_NOINTERFACE;
> > +
> > +    return S_OK;
> > +}
> It's not safe to handle shared pointer like that, when you know you're
> going to run in multithreaded environment.
> 
> I think creating that helper wrapper/host object, and then creating
> marshaler on every call should be fine.

Ah, okay. In our last discussion you had asked if it was really that
important that QI returns a new instance each time, so I assumed you
wanted a shared instance across each call. I can go back to creating a
unique one each time though like I was doing before.

> > +
> > +static ULONG WINAPI uia_reserved_ns_AddRef(IUnknown *iface)
> > +{
> > +    return 1;
> > +}
> > +
> > +static ULONG WINAPI uia_reserved_ns_Release(IUnknown *iface)
> > +{
> > +    return 1;
> > +}
> That probably doesn't matter, but I'll mentioned it anyway, occasionally
> for such singleton objects they still maintain refcount field, that
> doesn't do anything, expect changing its value.
> > +
> > +static const IUnknownVtbl uia_reserved_ns_vtbl = {
> > +    uia_reserved_ns_QueryInterface,
> > +    uia_reserved_ns_AddRef,
> > +    uia_reserved_ns_Release,
> > +};
> > +
> > +static IUnknown uia_reserved_ns_iface = {&uia_reserved_ns_vtbl};
> > +
> >  /***********************************************************************
> >   *          UiaClientsAreListening (uiautomationcore.@)
> >   */
> > @@ -46,8 +184,13 @@ HRESULT WINAPI UiaGetReservedMixedAttributeValue(IUnknown **value)
> >   */
> >  HRESULT WINAPI UiaGetReservedNotSupportedValue(IUnknown **value)
> >  {
> > -    FIXME("(%p) stub!\n", value);
> > -    *value = NULL;
> > +    TRACE("(%p)\n", value);
> > +
> > +    if (!value)
> > +        return E_INVALIDARG;
> > +
> > +    *value = &uia_reserved_ns_iface;
> > +
> >      return S_OK;
> >  }
> >  
> 
> 

Thanks for the review.