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

[Nikolay Sivov]

On 10/28/21 6:17 PM, Connor McAdams wrote:
> Signed-off-by: Connor McAdams
>
> 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.
> -
>
> +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.
> -
> - *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.
> -
>
> +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;
>
> }