Bug 25294 – All WebKit/win classes should return COMPtrs from their static constructor members

• Status: NEW
• Component: WebKit Misc.

[Adam Roben (:aroben)]

All classes in WebKit/win should return COMPtrs from their static constructor members (e.g., createInstance()). This will help prevent memory leaks/crashes caused by incorrect ref-count management.

[Adam Roben (:aroben)]

<rdar://problem/6803127>

[Adam Roben (:aroben)]

Created attachment 29616 [details]
patch for MemoryStream

[Darin Adler]

Comment on attachment 29616 [details]
patch for MemoryStream

>  HRESULT STDMETHODCALLTYPE MemoryStream::Clone( 
>      /* [out] */ IStream** ppstm)
>  {
> -    *ppstm = MemoryStream::createInstance(m_buffer);
> +    *ppstm = MemoryStream::createInstance(m_buffer).releaseRef();
> +    // FIXME: MSDN says we should be returning STG_E_INSUFFICIENT_MEMORY instead of E_OUTOFMEMORY here.
>      return (*ppstm) ? S_OK : E_OUTOFMEMORY;

Why not copyRefTo here?

> -    COMPtr<IStream> stream(AdoptCOM, MemoryStream::createInstance(buffer.release()));
> +    COMPtr<MemoryStream> stream = MemoryStream::createInstance(buffer.release());
>      m_view->didReceiveData(stream.get());

Would this read better without the local variable?

> -    COMPtr<MemoryStream> result(AdoptCOM, MemoryStream::createInstance(SharedBuffer::wrapCFData(historyData.get())));
> +    COMPtr<MemoryStream> result = MemoryStream::createInstance(SharedBuffer::wrapCFData(historyData.get()));
>      return result.copyRefTo(stream);

Would this read better without the local variable?

r=me

[Adam Roben (:aroben)]

(In reply to comment #3)
> (From update of attachment 29616 [details] [review])
> >  HRESULT STDMETHODCALLTYPE MemoryStream::Clone( 
> >      /* [out] */ IStream** ppstm)
> >  {
> > -    *ppstm = MemoryStream::createInstance(m_buffer);
> > +    *ppstm = MemoryStream::createInstance(m_buffer).releaseRef();
> > +    // FIXME: MSDN says we should be returning STG_E_INSUFFICIENT_MEMORY instead of E_OUTOFMEMORY here.
> >      return (*ppstm) ? S_OK : E_OUTOFMEMORY;
> 
> Why not copyRefTo here?

I originally thought that copyRefTo's behavior was incompatible with the existing behavior of this function. But now I see that it isn't. I will change to use copyRefTo.

> > -    COMPtr<IStream> stream(AdoptCOM, MemoryStream::createInstance(buffer.release()));
> > +    COMPtr<MemoryStream> stream = MemoryStream::createInstance(buffer.release());
> >      m_view->didReceiveData(stream.get());
> 
> Would this read better without the local variable?

Probably. I'll remove the variable.

> > -    COMPtr<MemoryStream> result(AdoptCOM, MemoryStream::createInstance(SharedBuffer::wrapCFData(historyData.get())));
> > +    COMPtr<MemoryStream> result = MemoryStream::createInstance(SharedBuffer::wrapCFData(historyData.get()));
> >      return result.copyRefTo(stream);
> 
> Would this read better without the local variable?

Maybe. I'll remove the variable and see how it looks.

Thanks for the review!

[Adam Roben (:aroben)]

Comment on attachment 29616 [details]
patch for MemoryStream

Landed as r42692

[Adam Roben (:aroben)]

Created attachment 42574 [details]
Make CFDictionaryPropertyBag::createInstance return a COMPtr

[Adam Roben (:aroben)]

Comment on attachment 42574 [details]
Make CFDictionaryPropertyBag::createInstance return a COMPtr

Landed as r50567 <http://trac.webkit.org/changeset/50567>