Bug 155112 - Web Inspector: Doesn't show size of compressed content correctly
Summary: Web Inspector: Doesn't show size of compressed content correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
: 163295 170597 (view as bug list)
Depends on:
Blocks: 28812 32026
  Show dependency treegraph
 
Reported: 2016-03-07 07:51 PST by Martin Häcker
Modified: 2017-04-17 18:31 PDT (History)
18 users (show)

See Also:


Attachments
Screenshot showing the problem (210.17 KB, image/png)
2016-03-07 07:52 PST, Martin Häcker
no flags Details
[PATCH] Work in Progress (56.44 KB, patch)
2017-04-12 19:53 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] For Bots (65.92 KB, patch)
2017-04-13 17:06 PDT, Joseph Pecoraro
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1007.00 KB, application/zip)
2017-04-13 18:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.69 MB, application/zip)
2017-04-13 18:25 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (1.28 MB, application/zip)
2017-04-13 18:31 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (73.71 KB, patch)
2017-04-13 18:52 PDT, Joseph Pecoraro
joepeck: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.00 MB, application/zip)
2017-04-13 20:01 PDT, Build Bot
no flags Details
[PATCH] Proposed Fix (73.72 KB, patch)
2017-04-13 20:03 PDT, Joseph Pecoraro
joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (73.78 KB, patch)
2017-04-13 20:07 PDT, Joseph Pecoraro
achristensen: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (9.02 MB, application/zip)
2017-04-14 17:59 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Häcker 2016-03-07 07:51:45 PST
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.
Comment 1 Radar WebKit Bug Importer 2016-03-07 07:51:57 PST
<rdar://problem/25006728>
Comment 2 Martin Häcker 2016-03-07 07:52:19 PST
Created attachment 273180 [details]
Screenshot showing the problem
Comment 3 Timothy Hatcher 2016-03-07 09:08:37 PST
<rdar://problem/21319035>
Comment 4 Joseph Pecoraro 2017-04-12 18:46:32 PDT
*** Bug 170597 has been marked as a duplicate of this bug. ***
Comment 5 Joseph Pecoraro 2017-04-12 18:54:33 PDT
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.
Comment 6 Joseph Pecoraro 2017-04-12 19:14:45 PDT
*** Bug 163295 has been marked as a duplicate of this bug. ***
Comment 7 Joseph Pecoraro 2017-04-12 19:53:02 PDT
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.
Comment 8 Joseph Pecoraro 2017-04-13 17:06:21 PDT
Created attachment 307050 [details]
[PATCH] For Bots
Comment 9 Build Bot 2017-04-13 18:25:05 PDT
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
Comment 10 Build Bot 2017-04-13 18:25:06 PDT
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
Comment 11 Build Bot 2017-04-13 18:25:07 PDT
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
Comment 12 Build Bot 2017-04-13 18:25:08 PDT
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
Comment 13 Build Bot 2017-04-13 18:30:59 PDT
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
Comment 14 Build Bot 2017-04-13 18:31:00 PDT
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
Comment 15 Joseph Pecoraro 2017-04-13 18:52:26 PDT
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)?
Comment 16 Matt Baker 2017-04-13 19:41:49 PDT
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
Comment 17 Build Bot 2017-04-13 20:01:18 PDT
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
Comment 18 Build Bot 2017-04-13 20:01:20 PDT
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
Comment 19 Joseph Pecoraro 2017-04-13 20:03:58 PDT
Created attachment 307078 [details]
[PATCH] Proposed Fix
Comment 20 Joseph Pecoraro 2017-04-13 20:07:49 PDT
Created attachment 307080 [details]
[PATCH] Proposed Fix

Addressed Matt's comments.
Comment 21 Timothy Hatcher 2017-04-13 21:14:42 PDT
Comment on attachment 307080 [details]
[PATCH] Proposed Fix

Inspector parts look good.
Comment 22 Joseph Pecoraro 2017-04-14 13:59:55 PDT
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
Comment 23 Build Bot 2017-04-14 17:59:44 PDT
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
Comment 24 Build Bot 2017-04-14 17:59:45 PDT
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
Comment 25 Alex Christensen 2017-04-14 23:44:14 PDT
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?
Comment 26 Alex Christensen 2017-04-15 10:34:17 PDT
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.
Comment 27 Joseph Pecoraro 2017-04-15 16:59:00 PDT
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)
Comment 28 Joseph Pecoraro 2017-04-15 17:01:48 PDT
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.
Comment 29 Joseph Pecoraro 2017-04-17 15:53:51 PDT
Ping reviewers.

I answered Alex's questions and I don't think they required any changes to the existing patch.
Comment 30 Joseph Pecoraro 2017-04-17 18:31:02 PDT
<https://trac.webkit.org/changeset/215445/webkit>