Bug 65926 - ResourceLoader::didReceiveDataArray() does not handle m_shouldBufferData correctly
Summary: ResourceLoader::didReceiveDataArray() does not handle m_shouldBufferData corr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Pratik Solanki
URL:
Keywords:
Depends on:
Blocks: 61863
  Show dependency treegraph
 
Reported: 2011-08-09 10:08 PDT by Pratik Solanki
Modified: 2011-08-12 13:35 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.41 KB, patch)
2011-08-10 11:44 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch (1.96 KB, patch)
2011-08-12 00:00 PDT, Pratik Solanki
no flags Details | Formatted Diff | Diff
Patch (2.09 KB, patch)
2011-08-12 08:36 PDT, Pratik Solanki
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pratik Solanki 2011-08-09 10:08:45 PDT
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.
Comment 1 Pratik Solanki 2011-08-10 11:44:55 PDT
Created attachment 103510 [details]
Patch
Comment 2 Pratik Solanki 2011-08-11 22:19:14 PDT
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).
Comment 3 Pratik Solanki 2011-08-12 00:00:58 PDT
Created attachment 103745 [details]
Patch
Comment 4 Alexey Proskuryakov 2011-08-12 00:38:24 PDT
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.
Comment 5 Pratik Solanki 2011-08-12 08:36:20 PDT
Created attachment 103775 [details]
Patch
Comment 6 Alexey Proskuryakov 2011-08-12 09:08:24 PDT
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);
Comment 7 Pratik Solanki 2011-08-12 12:51:39 PDT
(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!
Comment 8 Pratik Solanki 2011-08-12 13:33:25 PDT
Committed r92985: <http://trac.webkit.org/changeset/92985>
Comment 9 Alexey Proskuryakov 2011-08-12 13:35:58 PDT
> 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.