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.