Bug 65926

Created attachment 103510 [details] Patch

I was going to check this in and then I realized that the current code was avoiding copies by calling SharedBuffer::append(CFDataRef). If I change it to call addData() then we end up calling SharedBuffer::append(const char*, int) which would end up copying the char* buffer passed in. I am going to rework this so I can continue to call the SharedBuffer::append(CFDataRef).

Created attachment 103745 [details] Patch

Comment on attachment 103745 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103745&action=review > Source/WebCore/loader/mac/ResourceLoaderMac.mm:80 > if (!m_resourceData) > m_resourceData = SharedBuffer::create(); I don't think that we are supposed to create m_resourceData unless we're buffering.

Created attachment 103775 [details] Patch

Comment on attachment 103775 [details] Patch 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);

(In reply to comment #6) > (From update of attachment 103775 [details]) > 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. I think the confusions stem from the fact that SharedBuffer takes on different roles in different scenarios. In one of the reviews, David Kilzer had suggested we refactor SharedBuffer but I don't think I filed bug for it. The data array part of SharedBuffer is different from the CFDataRef part. The latter is used when you create a SharedBuffer that is a wrapper around a single CFDataRef, whereas the data array part behaves more like the version that takes char* buffers. > 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?! I do not expect SharedBuffer::append(CFDataRef) to be called on a SharedBuffer constructed via SharedBuffer(CFDataRef). But yeah, if that were to happen things won't work correctly. We have 2 fields in SharedBuffer 1. CFDataRef m_cfData - This is the "platform data" 2. Vector<RetainPtr<CFDataRef> > m_dataArray - This is the data array. > 2. SharedBuffer::maybeTransferPlatformData() doesn't set m_size after copying in CFData. I think it should for correctness. Doesn't look like append() will work correctly. It probably works right now because we don't call append(const char*, int) on a SharedBuffer that was created with a CFDataRef (i.e. via wrapCFData()). > 3. SharedBuffer::clearPlatformData() doesn't clear m_size. But that seems to be only called from SharedBuffer::clear() which sets m_size to 0. > 4. SharedBuffer::platformDataSize() only looks at the first chunk, and never at m_dataArray. Right. the platformData functions only look at the CFDataRef. > 5. SharedBuffer::maybeTransferPlatformData() doesn't transfer m_dataArray. Yeah, it probably should. > > 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); I'll keep it the way I have it since, the CFDataRef constructor won't work with CFDataArrayRef code. I think SharedBuffer class is calling for a cleanup!

Committed r92985: <http://trac.webkit.org/changeset/92985>

> Right. the platformData functions only look at the CFDataRef. This means that SharedBuffer::size() will always return a wrong size when m_dataArray is non-empty. Even though the code has been tested in practice, I'm not convinced that it actually always works without losing parts of response data today.