Web Inspector: Doesn't show size of compressed content correctly

• Component: Web Inspector
• Status: RESOLVED FIXED

[Martin Häcker]

When inspecting  a page I'm developing in Safari, I noticed that on El Capitan 10.11.3 (15D21), the Network Panel doesn't show the correct sizes of downloaded requests.

(See attached screenshot).

I verified on the network level that compression is actually enabled and works successfully, but that information is not visible from the inspector.

There have already been quite a few bugs in the past that seem to have shown this exact behaviour, so there seems to be a regression test missing perhaps?

Examples of bugs previous bugs I found dealing with this issue: #31019, #19793, #35403, #56691

This happens both in the released Safari, as well as in the nightly as of r192768.

[Radar WebKit Bug Importer]

<rdar://problem/25006728>

[Martin Häcker]

Created attachment 273180 [details]
Screenshot showing the problem

[Timothy Hatcher]

<rdar://problem/21319035>

[Joseph Pecoraro]

*** Bug 170597 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

In Safari using NSURLSession (CFNetwork) we don't currently have a way to get the number of encoded bytes received by the network connection for the Resource Body. The existing API (-[NSURLSession countOfBytesReceived]) gives the number of decoded bytes, and that is what we are seeing right now in Web Inspector.

Eventually we will want access to the encoded size during the download so that we can accurately report the encoded/decoded size of the resource while the load is ongoing. That is tracked by Bug 158463.
<https://webkit.org/b/158463> Network: Correctly report encoded data length (transfer size) from CFNetwork to NetworkResourceLoader

Web Inspector is smart enough to fall-back to using the "Content-Length" header to display an encoded transfer size for resources, but that is not always available. For example see bug 170597 for a detailed account and examples:
<https://webkit.org/b/170597> Web Inspector: Compression inaccurately reported when Content-Length header omitted

--

We can use new CFNetwork SPI to get more complete metrics about network transfer sizes:

  - request headers size
  - request body size
  - response headers size
  - response body size (encoded)
  - response body size (decoded)

This gives us information we didn't have before (header sizes, request sizes). I'm going to use this bug to track using those metrics. That will allow Web Inspector to show accurate sizes once a resource has fully loaded.

[Joseph Pecoraro]

*** Bug 163295 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

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

The tests will fail on all EWS ports right now. I'm going to test on a 2nd machine tomorrow to see how I should handle older systems.

[Joseph Pecoraro]

Created attachment 307050 [details]
[PATCH] For Bots

[Build Bot]

Comment on attachment 307050 [details]
[PATCH] For Bots

Attachment 307050 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3531425

New failing tests:
http/tests/inspector/network/resource-sizes-network.html

[Build Bot]

Created attachment 307066 [details]
Archive of layout-test-results from ews103 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307050 [details]
[PATCH] For Bots

Attachment 307050 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3531397

New failing tests:
http/tests/inspector/network/resource-sizes-network.html

[Build Bot]

Created attachment 307067 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 307050 [details]
[PATCH] For Bots

Attachment 307050 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3531432

New failing tests:
http/tests/inspector/network/resource-sizes-network.html

[Build Bot]

Created attachment 307068 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Created attachment 307071 [details]
[PATCH] Proposed Fix

Alright! This should now be reviewable.

I think we should get a review from an Inspector person (Brian / Matt / Tim)? and a Networking person (Alex / Brady / Antti)?

[Matt Baker]

Comment on attachment 307071 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Models/Resource.js:489
> +        // for estimatedTransferSize.  So prefer the "Content-Length" property

Comment style: only need one space after period.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:532
> +        return !!(contentEncoding && /\b(?:gzip|deflate)\b/.test(contentEncoding));

Good catch, looks like we previously returned undefined or true.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:616
> +        this._estimatedResponseHeadersSize = String(this._statusCode).length + this._statusText.length + 12; // Extra length is for "HTTP/1.1 ", " ", and "\r\n".

What about:

const headerBaseSize = 12; // Length of "HTTP/1.1 ", " ", and "\r\n";
this._estimatedResponseHeadersSize = String(this._statusCode).length + this._statusText.length + headerBaseSize;

> Source/WebInspectorUI/UserInterface/Models/Resource.js:618
> +            this._estimatedResponseHeadersSize += name.length + this._responseHeaders[name].length + 4; // Extra length is for ": ", and "\r\n".

Ditto.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:720
> +        var previousSize = this._estimatedSize;

let

[Build Bot]

Comment on attachment 307071 [details]
[PATCH] Proposed Fix

Attachment 307071 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3531812

New failing tests:
http/tests/inspector/network/resource-sizes-network.html

[Build Bot]

Created attachment 307077 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Joseph Pecoraro]

Created attachment 307078 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 307080 [details]
[PATCH] Proposed Fix

Addressed Matt's comments.

[Timothy Hatcher]

Comment on attachment 307080 [details]
[PATCH] Proposed Fix

Inspector parts look good.

[Joseph Pecoraro]

Comment on attachment 307080 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Models/Resource.js:490
> +        // macOS provides the decoded transfer size instead of the encoded size
> +        // for estimatedTransferSize. So prefer the "Content-Length" property
> +        // on mac if it is available.

I can add the bug number tracking this:
<https://webkit.org/b/158463> Network: Correctly report encoded data length (transfer size) from CFNetwork to NetworkResourceLoader

[Build Bot]

Comment on attachment 307080 [details]
[PATCH] Proposed Fix

Attachment 307080 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3536339

New failing tests:
webrtc/multi-video.html

[Build Bot]

Created attachment 307174 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Alex Christensen]

Comment on attachment 307080 [details]
[PATCH] Proposed Fix

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

> LayoutTests/platform/mac/http/tests/inspector/network/resource-sizes-network-expected.txt:14
> +FAIL: networkEncodedSize should be exactly 2955 bytes.
> +    Expected: 2955
> +    Actual: NaN

What's this about?

> Source/WebCore/platform/spi/cf/CFNetworkSPI.h:128
> +@property (assign, readonly) NSInteger _requestHeaderBytesSent;

Is this SPI available on all platforms?

[Alex Christensen]

Comment on attachment 307080 [details]
[PATCH] Proposed Fix

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

I think we shouldn't check in tests with a FAIL expectation without some kind of explanation of what that means.  Also, I'm not sure the SPI exists everywhere.  Other than that, I think the network code looks good.

> Source/WebCore/platform/network/NetworkLoadMetrics.h:104
> +        requestHeaderBytesSent = std::nullopt;

Note: in another patch, we should replace the Seconds values that are -1 right now if there is no value with std::optional<Seconds>

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:625
> +    networkLoadMetrics.responseHeaderBytesReceived = 0;

Does the inspector have an indication that this resource was retrieved from the cache?  Does anyone care how many bytes were pulled from the cache?  It's obviously different from bytes received over the network, but maybe there should be a difference between a 0 byte resource and a resource from the cache.

[Joseph Pecoraro]

Comment on attachment 307080 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/platform/mac/http/tests/inspector/network/resource-sizes-network-expected.txt:14
>> +    Actual: NaN
> 
> What's this about?

This is a platform expectation for Mac because as mentioned in the ChangeLog:

    Older versions of macOS will not have the new metrics...

Older cocoa platforms will be using the "estimated*" methods, which are tested here. If you want I could hide NaN output, but I'd still want to include platform expectations for older macOSes so that their code pathes are still tested.

>> Source/WebCore/platform/spi/cf/CFNetworkSPI.h:128
>> +@property (assign, readonly) NSInteger _requestHeaderBytesSent;
> 
> Is this SPI available on all platforms?

No, just out of range of the Review Tools is this condition wrapping this class extension:

    #if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101300) || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 110000)

[Joseph Pecoraro]

Comment on attachment 307080 [details]
[PATCH] Proposed Fix

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

>> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:625
>> +    networkLoadMetrics.responseHeaderBytesReceived = 0;
> 
> Does the inspector have an indication that this resource was retrieved from the cache?  Does anyone care how many bytes were pulled from the cache?  It's obviously different from bytes received over the network, but maybe there should be a difference between a 0 byte resource and a resource from the cache.

Yes, the inspector will know that this resource was loaded through the DiskCache. (Likewise it identifies resources loaded through the MemoryCache).

Right now I'm more interested in showing the exact number of bytes that went over the network in Web Inspector. I'm not sure knowing how a disk cached resource was stored (possibly compressed or not) will be as interesting to web developers as that is an implementation detail of WebKit.

[Joseph Pecoraro]

Ping reviewers.

I answered Alex's questions and I don't think they required any changes to the existing patch.

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/215445/webkit>