In [WebCoreResourceHandleAsDelegate connection:didReceiveDataArray:], we do for (NSData *data in dataArray) m_handle->client()->didReceiveData(m_handle, static_cast<const char*>([data bytes]), [data length], static_cast<int>([data length])); However, in the didReceiveData() callback, the client can cancel the load which would result in the delegate (self) being freed. This means we could crash in the subsequent iteration of the loop.
<rdar://problem/9203259>
Created attachment 93038 [details] Patch
Comment on attachment 93038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93038&action=review > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=60594 Please add a Radar link here, too. Also, there needs to be an explanation why there are no tests (e.g. "no bot tests CFNETWORK_DATA_ARRAY_CALLBACK"). > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:831 > + // The call to didReceiveData below could cancel a load, which would result in the delegate > + // (self) being released. Retain self so we can at least check the value of m_handle without > + // crashing and return safely in case the load got cancelled. Please remove the second sentence - it just states what the code below does. > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:832 > + RetainPtr<WebCoreResourceHandleAsDelegate> protectDelegate(self); protect(self) reads more like an English sentence than protectDelegate(self). > Source/WebCore/platform/network/mac/ResourceHandleMac.mm:835 > + if (m_handle && m_handle->client()) > + m_handle->client()->didReceiveData(m_handle, static_cast<const char*>([data bytes]), [data length], static_cast<int>([data length])); Is the check for m_handle->client() necessary? Let's not add it unless it's necessary, since it would be confusing readers otherwise. I'd have used an early return: if (!m_handle) break;
Comment on attachment 93038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93038&action=review >> Source/WebCore/ChangeLog:4 >> + https://bugs.webkit.org/show_bug.cgi?id=60594 > > Please add a Radar link here, too. > > Also, there needs to be an explanation why there are no tests (e.g. "no bot tests CFNETWORK_DATA_ARRAY_CALLBACK"). Done. >> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:831 >> + // crashing and return safely in case the load got cancelled. > > Please remove the second sentence - it just states what the code below does. Ok. >> Source/WebCore/platform/network/mac/ResourceHandleMac.mm:835 >> + m_handle->client()->didReceiveData(m_handle, static_cast<const char*>([data bytes]), [data length], static_cast<int>([data length])); > > Is the check for m_handle->client() necessary? Let's not add it unless it's necessary, since it would be confusing readers otherwise. > > I'd have used an early return: > > if (!m_handle) > break; Removed the check for m_handle->client() since its not necessary. And added the early return. Thanks for the review. I'll commit with the changes you suggested.
Committed r86200: <http://trac.webkit.org/changeset/86200>