Re: [PATCH 2/2] ole32: Fix refcounting of IObjContext per-thread instance

[Huw Davies]

On Mon, Mar 14, 2016 at 08:10:00AM +0300, Nikolay Sivov wrote:
> Signed-off-by: Nikolay Sivov <nsivov(a)codeweavers.com>
> ---
>  dlls/ole32/compobj.c       | 57 +++++++++++++++++++---------------------------
>  dlls/ole32/tests/compobj.c | 46 ++++++++++++++++++++++---------------
>  2 files changed, 51 insertions(+), 52 deletions(-)
>
> diff --git a/dlls/ole32/compobj.c b/dlls/ole32/compobj.c
> index b095aeb..1081f98 100644
> --- a/dlls/ole32/compobj.c
> +++ b/dlls/ole32/compobj.c
> @@ -4683,10 +4683,13 @@ static ULONG Context_AddRef(Context *This)
>
> static ULONG Context_Release(Context *This)
>  {
> -    ULONG refs = InterlockedDecrement(&This->refs);
> -    if (!refs)
> +    if (!This->refs)
> +    {

Since this is non-standard it needs a comment.  Probably just
referring to the constructor below.
> HeapFree(GetProcessHeap(), 0, This);
> -    return refs;
> +        return 0;
> +    }
> +
> +    return InterlockedDecrement(&This->refs);
>  }
>
> static HRESULT WINAPI Context_CTI_QueryInterface(IComThreadingInfo *iface, REFIID riid, LPVOID *ppv)
> @@ -4924,39 +4927,19 @@ static const IObjContextVtbl Context_Object_Vtbl =
>   */
>  HRESULT WINAPI CoGetObjectContext(REFIID riid, void **ppv)
>  {
> -    APARTMENT *apt = COM_CurrentApt();
> -    Context *context;
> +    IObjContext *context;
>      HRESULT hr;
>
> TRACE("(%s, %p)\n", debugstr_guid(riid), ppv);
>
> *ppv = NULL;
> -    if (!apt)
> -    {
> -        if (!(apt = apartment_find_multi_threaded()))
> -        {
> -            ERR("apartment not initialised\n");
> -            return CO_E_NOTINITIALIZED;
> -        }
> -        apartment_release(apt);
> -    }
> -
> -    context = HeapAlloc(GetProcessHeap(), 0, sizeof(*context));
> -    if (!context)
> -        return E_OUTOFMEMORY;
> -
> -    context->IComThreadingInfo_iface.lpVtbl = &Context_Threading_Vtbl;
> -    context->IContextCallback_iface.lpVtbl = &Context_Callback_Vtbl;
> -    context->IObjContext_iface.lpVtbl = &Context_Object_Vtbl;
> -    context->refs = 1;
> -
> -    hr = IComThreadingInfo_QueryInterface(&context->IComThreadingInfo_iface, riid, ppv);
> -    IComThreadingInfo_Release(&context->IComThreadingInfo_iface);
> +    hr = CoGetContextToken((ULONG_PTR*)&context);
> +    if (FAILED(hr))
> +        return hr;
>
> -    return hr;
> +    return IObjContext_QueryInterface(context, riid, ppv);
>  }
>
> -
>  /***********************************************************************
>   *           CoGetContextToken [OLE32.@]
>   */
> @@ -4985,16 +4968,22 @@ HRESULT WINAPI CoGetContextToken( ULONG_PTR *token )
>
> if (!info->context_token)
>      {
> -        HRESULT hr;
> -        IObjContext *ctx;
> +        Context *context;
> +
> +        context = HeapAlloc(GetProcessHeap(), 0, sizeof(*context));
> +        if (!context)
> +            return E_OUTOFMEMORY;
> +
> +        context->IComThreadingInfo_iface.lpVtbl = &Context_Threading_Vtbl;
> +        context->IContextCallback_iface.lpVtbl = &Context_Callback_Vtbl;
> +        context->IObjContext_iface.lpVtbl = &Context_Object_Vtbl;

Add a comment here stating that context_token does not take a ref.
> +        context->refs = 0;
>
> -        hr = CoGetObjectContext(&IID_IObjContext, (void **)&ctx);
> -        if (FAILED(hr)) return hr;
> -        info->context_token = ctx;
> +        info->context_token = &context->IObjContext_iface;
>      }
>
> *token = (ULONG_PTR)info->context_token;
> -    TRACE("apt->context_token=%p\n", info->context_token);
> +    TRACE("context_token=%p\n", info->context_token);
>
> return S_OK;
>  }

I'm wondering whether this patch could be split since it's doing
rather more than fixing a ref counting issue.  Could you move the
constructor to CoGetContextToken first and then fix the refs, or would
that mess up the tests?

Huw.

[PATCH 2/2] ole32: Fix refcounting of IObjContext per-thread instance

[Nikolay Sivov]

On 14.03.2016 13:01, Huw Davies wrote:
> I'm wondering whether this patch could be split since it's doing
> rather more than fixing a ref counting issue.  Could you move the
> constructor to CoGetContextToken first and then fix the refs, or would
> that mess up the tests?

That's exactly what I did initially, it was 3 patches series. And yes,
it breaks tests if separated, which means it's regressing even if for a
brief moment to be fixed by following commit, so I decided to merge them.

One thing I'm a bit worried about is that it's possible for client to
release this shared instance in a first place (and that's how it works
on windows, e.g. if you release pointer returned by CoGetContextToken it
will crash on subsequent method calls. Do we want to replicate that or
maybe it's better to make it more reliable and never destroy on Release,
but only in COM_TlsDestroy()?
> Huw.

[PATCH 2/2] ole32: Fix refcounting of IObjContext per-thread instance

[Huw Davies]

On Mon, Mar 14, 2016 at 01:27:08PM +0300, Nikolay Sivov wrote:
> On 14.03.2016 13:01, Huw Davies wrote:
> > I'm wondering whether this patch could be split since it's doing
> > rather more than fixing a ref counting issue.  Could you move the
> > constructor to CoGetContextToken first and then fix the refs, or would
> > that mess up the tests?
>
> That's exactly what I did initially, it was 3 patches series. And yes,
> it breaks tests if separated, which means it's regressing even if for a
> brief moment to be fixed by following commit, so I decided to merge them.

Ok.
> One thing I'm a bit worried about is that it's possible for client to
> release this shared instance in a first place (and that's how it works
> on windows, e.g. if you release pointer returned by CoGetContextToken it
> will crash on subsequent method calls. Do we want to replicate that or
> maybe it's better to make it more reliable and never destroy on Release,
> but only in COM_TlsDestroy()?

I think we can leave it as you have it.  If we find an app for which
that causes problems, we can change it then.

Huw.