[webkit-reviews] review granted: [Bug 65926] ResourceLoader::didReceiveDataArray() does not handle m_shouldBufferData correctly : [Attachment 103775] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Pratik Solanki
<psolanki at apple.com>'s request for review:
Bug 65926: ResourceLoader::didReceiveDataArray() does not handle
m_shouldBufferData correctly
https://bugs.webkit.org/show_bug.cgi?id=65926

Attachment 103775: Patch
https://bugs.webkit.org/attachment.cgi?id=103775&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103775&action=review


This patch looks fine, but I had a look at SharedBuffer code, and that doesn't
look correct to me.

1. SharedBuffer(CFDataRef) doesn't set m_size, which may or may not be by
design, but is extremely confusing. If this m_size data member only counts
sizes of some data representations, the variable should be called
appropriately. And then SharedBuffer::append(CFDataRef) increments m_size?!
2. SharedBuffer::maybeTransferPlatformData() doesn't set m_size after copying
in CFData.
3. SharedBuffer::clearPlatformData() doesn't clear m_size.
4. SharedBuffer::platformDataSize() only looks at the first chunk, and never at
m_dataArray.
5. SharedBuffer::maybeTransferPlatformData() doesn't transfer m_dataArray.

> Source/WebCore/loader/mac/ResourceLoaderMac.mm:87
> +	       if (!m_resourceData)
> +		   m_resourceData = SharedBuffer::create();
> +	       m_resourceData->append(data);

You could also copy how it's done in addData():

	if (!m_resourceData)
	    m_resourceData = SharedBuffer::create(data);
	else
	    m_resourceData->append(data);