Bug 65926 – ResourceLoader::didReceiveDataArray() does not handle m_shouldBufferData correctly

• Component: Platform
• Status: RESOLVED FIXED

[Pratik Solanki]

This was noticed in <https://bugs.webkit.org/show_bug.cgi?id=61863#c11> by Alexey. ResourceLoader::didReceiveDataArray() does an early bailout if m_shouldBufferData is set to false. It does not call through to the client and pass the data like didReceiveData() does.

[Pratik Solanki]

Created attachment 103510 [details]
Patch

[Pratik Solanki]

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).

[Pratik Solanki]

Created attachment 103745 [details]
Patch

[Alexey Proskuryakov]

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.

[Pratik Solanki]

Created attachment 103775 [details]
Patch

[Alexey Proskuryakov]

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);

[Pratik Solanki]

(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!

[Pratik Solanki]

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

[Alexey Proskuryakov]

> 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.