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.
<rdar://problem/6803127>
Created attachment 29616 [details] patch for MemoryStream
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
(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!
Comment on attachment 29616 [details] patch for MemoryStream Landed as r42692
Created attachment 42574 [details] Make CFDictionaryPropertyBag::createInstance return a COMPtr
Comment on attachment 42574 [details] Make CFDictionaryPropertyBag::createInstance return a COMPtr Landed as r50567 <http://trac.webkit.org/changeset/50567>