Re: [PATCH v5 0/7] MR3189: Add windows.devices.geolocation.geolocator

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/windows.devices.geolocation.geolocator/main.c:
> +}
> -
>
> +static HRESULT WINAPI weak_reference_Resolve(IWeakReference *iface, REFIID iid, IInspectable **ref)
> +{
> - struct geolocator *impl = impl_from_IWeakReference(iface);
> - TRACE("iface %p, ref %p stub.\n", iface, ref);
> -
> - if (!impl->ref_public)
> - {
> -
>    *ref = 0;
> - }
> - else
> - {
> -
>    *ref = (IInspectable*)&impl->IGeolocator_iface;
> -
>    IInspectable_AddRef(*ref);
> - }

This will need to be done in an atomic way. You also probably need to respect the iid that is passed. I also suggest to rename `ref` parameter to out, as `ref` is often used for refcount values.

You may have a thread B currently releasing the last public reference on the object right after the current thread A checks this. This would result in the thread B seeing Release return 0, indicating the object is now invalid, while the thread A increasing the public ref again to 1, receiving a valid object, which is incorrect.

I think this will need to be done this way:

1) in Resolve:

```c
if ((ref = InterlockedIncrement(&impl->ref_public)) > 1)
    hr = IGeolocator_QueryInterface(&impl->IGeolocator_iface, iid, out);
else /* invalid object */
{
    *out = NULL;
    hr = S_OK;
}
InterlockedDecrement(&impl->ref_public);
```

Also now there's an issue as we increase the public refcount here, and we may end up releasing the last public ref. It could either also include the same check as in `geolocator_Release` and release a weak ref as well.

Or, perhaps better as it would be more consistent, change `geolocator_AddRef` / `geolocator_Release` to also always add/release a weak reference:

2) in geolocator_AddRef / geolocator_Release:

```c
static ULONG WINAPI geolocator_AddRef(IGeolocator *iface)
{
    struct geolocator *impl = impl_from_IGeolocator(iface);
    ULONG ref = InterlockedIncrement(&impl->ref_public);
    TRACE("iface %p increasing refcount to %lu.\n", iface, ref);
    IWeakReference_AddRef(&impl->IWeakReference_iface);
    return ref;
}

static ULONG WINAPI geolocator_Release(IGeolocator *iface)
{
    struct geolocator *impl = impl_from_IGeolocator(iface);
    ULONG ref = InterlockedDecrement(&impl->ref_public);
    TRACE("iface %p decreasing refcount to %lu.\n", iface, ref);
    IWeakReference_Release(&impl->IWeakReference_iface);
    return ref;
}
```