Re: No test on the return of pOleObject->Close in embedserv/source/inprocserv/inprocembobj.cxx

On 12/22/2012 12:12 AM, julien2412 wrote:
> Cppcheck reported this:
> [source/inprocserv/inprocembobj.cxx:783] ->
> [source/inprocserv/inprocembobj.cxx:784]: (performance) Variable 'hr' is
> reassigned a value before the old one has been used
>
> Here are the lines:
>      778         HRESULT hr = m_pDefHandler->QueryInterface( IID_IOleObject,
> (void**)&pOleObject );
>      779
>      780         ULONGGuard aGuard( &m_nCallsOnStack ); // avoid reentrance
> problem
>      781         if ( SUCCEEDED( hr ) && pOleObject )
>      782         {
>      783             hr = pOleObject->Close( dwSaveOption );
>      784             hr = CoDisconnectObject(
> (IUnknown*)(IPersistStorage*)this, 0 );
>      785         }
>
> (see
> http://opengrok.libreoffice.org/xref/core/embedserv/source/inprocserv/inprocembobj.cxx#778)
> Moreover, the result of CoDisconnectObject isn't tested too.
>
> I thought about adding after 783 and 784 (so after both lines) this:
> if( !SUCCEEDED( hr ) ) return hr;
>
> What do you think?

I really know zero about this, but I would /assume/ that (a) one wants 
to indeed call CoDisconnectObject even if pOleObject->Close fails, and 
(b) the return value of InprocEmbedDocument_Impl::Close should reflect 
any failures returned from pOleObject->Close and CoDisconnectObject. 
Something like

  HRESULT ret = S_OK;
  if ( m_pDefHandler ...
     ...
     hr = pOleObject->Close(...);
     if (!SUCCEEDED(hr))
       ret = hr;
     hr = CoDisconnectObject(...);
     if (!(SUCCEEDED(hr) && SUCCEEDED(ret)))
       ret = hr;
    }
    ...
  return ret;

Stephan