Bug 158463 – Network: Correctly report encoded data length (transfer size) from CFNetwork to NetworkResourceLoader

• Status: NEW
• Component: New Bugs

[Alex Christensen]

Correctly report encoded data length from CFNetwork to NetworkResourceLoader

[Alex Christensen]

Created attachment 280676 [details]
Patch

[Alex Christensen]

Created attachment 280715 [details]
Patch

[Darin Adler]

Comment on attachment 280715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280715&action=review

> Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:263
> +        m_handle->client()->didReceiveBuffer(m_handle, SharedBuffer::wrapNSData(data), static_cast<int>(lengthReceived));

This chops a 64-bit integer and turns it into a 32-bit integer. Why is that OK?

> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:252
> +void NetworkLoad::didReceiveData(RefPtr<SharedBuffer>&& buffer, int64_t encodedDataLength)

How did you choose the type int64_t? Why signed rather than unsigned? Also, other places in the code use “[unsigned] long long” for this.

> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:255
> +    m_client.didReceiveBuffer(WTFMove(buffer), static_cast<int>(encodedDataLength));

This chops a 64-bit integer and turns it into a 32-bit integer. Why is that OK?

[Alex Christensen]

(In reply to comment #3)
> Comment on attachment 280715 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280715&action=review
> 
> > Source/WebCore/platform/network/mac/WebCoreResourceHandleAsOperationQueueDelegate.mm:263
> > +        m_handle->client()->didReceiveBuffer(m_handle, SharedBuffer::wrapNSData(data), static_cast<int>(lengthReceived));
> 
> This chops a 64-bit integer and turns it into a 32-bit integer. Why is that
> OK?
There is great inconsistency in how this is passed around.  It should be fixed.
> 
> > Source/WebKit2/NetworkProcess/NetworkLoad.cpp:252
> > +void NetworkLoad::didReceiveData(RefPtr<SharedBuffer>&& buffer, int64_t encodedDataLength)
> 
> How did you choose the type int64_t? Why signed rather than unsigned? Also,
> other places in the code use “[unsigned] long long” for this.
That's what is used in NSURLSession.h.
@property (readonly) int64_t countOfBytesReceived;
There's a comment saying it can be 0 or -1.

[Alex Christensen]

rdar://problem/21319035

[Alex Christensen]

Created attachment 280756 [details]
Patch

[Alex Christensen]

Created attachment 280769 [details]
Patch

[Alex Christensen]

Created attachment 280774 [details]
Patch

[Alex Christensen]

Created attachment 280784 [details]
Patch

[Alex Christensen]

Comment on attachment 280784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280784&action=review

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:265
> +        networkDataTask->didReceiveData(WebCore::SharedBuffer::wrapNSData(data), [dataTask countOfBytesReceived]);

[dataTask countOfBytesReceived] returns the decoded size, not the encoded size.  That's exactly the same as the sum of the previous data->size()s.  This gives us no additional information.

[Darin Adler]

Comment on attachment 280715 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280715&action=review

>>> Source/WebKit2/NetworkProcess/NetworkLoad.cpp:252
>>> +void NetworkLoad::didReceiveData(RefPtr<SharedBuffer>&& buffer, int64_t encodedDataLength)
>> 
>> How did you choose the type int64_t? Why signed rather than unsigned? Also, other places in the code use “[unsigned] long long” for this.
> 
> That's what is used in NSURLSession.h.
> @property (readonly) int64_t countOfBytesReceived;
> There's a comment saying it can be 0 or -1.

OK, but for future reference, that doesn’t make it the correct type for platform-independent code.

[Joseph Pecoraro]

<rdar://problem/27927293>

[Joseph Pecoraro]

> Comment on attachment 280784 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280784&action=review
> 
> > Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:265
> > +        networkDataTask->didReceiveData(WebCore::SharedBuffer::wrapNSData(data), [dataTask countOfBytesReceived]);
> 
> [dataTask countOfBytesReceived] returns the decoded size, not the encoded
> size.  That's exactly the same as the sum of the previous data->size()s. 
> This gives us no additional information.

There is a new SPI we should be able to use now to get the encoded size:
-[NSURLSessionTask _countOfBytesReceivedEncoded]

I'll update the patch.

[Joseph Pecoraro]

Comment on attachment 280784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=280784&action=review

> Source/WebKit/WebCoreSupport/WebResourceLoadScheduler.cpp:94
>  WebResourceLoadScheduler::~WebResourceLoadScheduler()
>  {
> +    // FIXME: Why isn't m_nonHTTPProtocolHost deleted?
>  }

It looks like this should delete the host, but I think the WebResourceLoadScheduler (owned by the PlatformStrategy) is never be destroyed. This could move to a separate issue.

[Joseph Pecoraro]

Created attachment 314544 [details]
[PATCH] Work in Progress

I don't think the new SPI is enough. But I'm attaching my work in progress so it doesn't get lost.

[Joseph Pecoraro]

Comment on attachment 314544 [details]
[PATCH] Work in Progress

View in context: https://bugs.webkit.org/attachment.cgi?id=314544&action=review

> Source/WebKit2/NetworkProcess/cocoa/NetworkSessionCocoa.mm:405
> +        if ([dataTask respondsToSelector:@selector(_countOfBytesReceivedEncoded)]) {
> +            WTFLogAlways(">>> didReceiveData (%u) (%u)", (unsigned)dataTask._countOfBytesReceivedEncoded, (unsigned)dataTask.countOfBytesReceived);
> +            bytesReceived = dataTask._countOfBytesReceivedEncoded ?: dataTask.countOfBytesReceived;
> +        }

This is not good enough.

  1. If the response is encoded (compressed) data, then _countOfBytesReceivedEncoded starts with zero and doesn't include headers...
  2. If the response is uncompressed data, then countOfBytesReceived starts with non-zero and includes the header size.

So without knowing if we are receiving compressed / uncompressed data we can't know which set of APIs to use.