On Mon Jul 3 09:42:28 2023 +0000, Rémi Bernon wrote:
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:
- in Resolve:
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:
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; }
I'm a bit tired and this is kinda complicated, so please don't mind if I just rehash the issues real quick to avoid misunderstanding.
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.
For clarification, you mean A passing the check `!impl->ref_public` in `Resolve`, then B calling `Release` and getting a 0, while A gets a valid Object. We need to have both calls either fail or succeed. Correct?
Second problem is, when incrementing the count in `Resolve` we could miss that `if (!ref)` in `geolocator_Release`. Right? So we could just always add/release the weak reference in the geolocator.
If I understood all that correctly, that seems reasonable to me. Thanks!