Bug 168351 – [Resource Timing] Gather timing information with reliable responseEnd time

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Joseph Pecoraro]

[Resource Timing] Gather timing information with reliable responseEnd time

Currently:
  - WebContentProcess records pre-network times
  - NetworkProcess records network request times
  - WebContentProcess records responseEnd after receiving data from the NetworkProcess

However because the double switch, the "responseEnd" recorded in the WebContentProcess doesn't line up with the other times and may end up before the last NetworkProcess time (responseStart).

Ideally we just complete recording of network timestamps in the NetworkProcess.

Suggested:
  - WebContentProcess records pre-network times
  - NetworkProcess records network request times + responseEnd

This likely means moving NetworkLoadTiming off of ResourceResponse and instead sending it through a separate message, which is something we wanted to do anyways.

[Joseph Pecoraro]

This may therefore fix bug 161170.

[Joseph Pecoraro]

It is my understanding that on Cocoa platforms:

  -[NSURLSessionTaskDelegate URLSession:task:didFinishCollectingMetrics:]

is always guaranteed to be received before:

  -[NSURLSessionTaskDelegate URLSession:task:didCompleteWithError]

Moving to that, will allow Cocoa ports to:

  1. Move off of SPI to API (NSURLSessionTaskTransactionMetrics) for network load times
  2. Gain the "nextHopProtocol" value needed for Resource Timing Level 2
  3. Have a reliable responseEnd timestamp relative to other network timestamps

So that will be my goal for this radar. It is going to require a lot of plumbing.

[Joseph Pecoraro]

I believe this should fix bug 161088.

[Joseph Pecoraro]

I haven't quite figured out how I'm going to eliminate ResourceResponses's NetworkLoadTiming object. I'll have to how the ResourceHandle loading path works and ties into WebCore. It will need to be eliminated from many different places, and that task can be separate.

[Joseph Pecoraro]

Created attachment 301883 [details]
[PATCH] For Bots

[WebKit Commit Bot]

Attachment 301883 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/cocoa/NetworkLoadMetrics.mm:72:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:104:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:105:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:106:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:108:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 301885 [details]
[PATCH] For Bots

[WebKit Commit Bot]

Attachment 301885 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/cocoa/NetworkLoadMetrics.mm:72:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:104:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:105:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:106:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:108:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/resource-timing/test_resource_timing.html

[Build Bot]

Created attachment 301889 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

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

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/resource-timing/test_resource_timing.html

[Build Bot]

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

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

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location.html

[Build Bot]

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

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

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/resource-timing/test_resource_timing.html

[Build Bot]

Created attachment 301896 [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

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location.html

[Build Bot]

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

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

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/resource-timing/test_resource_timing.html

[Build Bot]

Created attachment 301904 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

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

[Joseph Pecoraro]

Ahh yes. El Capitan won't have protocol information because:

  (1) It did not have the NSURLSessionTask didFinishCollectingMetrics delegate
  (2) It did not have protocol information in the SPI it uses to get timing data

So I'll need platform specific results on El Capitan.

I don't understand the fetch issues on iOS yet.

[Joseph Pecoraro]

Created attachment 301984 [details]
[PATCH] For Bots

[WebKit Commit Bot]

Attachment 301984 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/cocoa/NetworkLoadMetrics.mm:72:  No space between ^ and block definition.  [whitespace/brackets] [4]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:104:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:105:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:106:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:108:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 5 in 54 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Oh great, the test itself is flakey because if the resource is served from the cache it will not have a protocol. That sucks. I can mark it as flakey. Ideally we could address this upstream in web-platform-tests but instead I'll just add my own test for protocol name.

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/resource-timing/test_resource_timing.html

[Build Bot]

Created attachment 301992 [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

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin.html
imported/w3c/web-platform-tests/resource-timing/test_resource_timing.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location.html

[Build Bot]

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

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

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/resource-timing/test_resource_timing.html

[Build Bot]

Created attachment 301995 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

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

[Joseph Pecoraro]

Created attachment 302037 [details]
[PATCH] Proposed Fix

Alrighty! I have a good feeling about this one.

[Joseph Pecoraro]

Created attachment 302040 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 302040 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/cocoa/NetworkLoadMetrics.mm:72:  No space between ^ and block definition.  [whitespace/brackets] [4]
WARNING: Not running on native Windows.
ERROR: LayoutTests/platform/win/TestExpectations:3787:  Path does not exist.  [test/expectations] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:104:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:105:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:106:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:108:  One space before end of line comments  [whitespace/comments] [5]
ERROR: LayoutTests/platform/mac/TestExpectations:1539:  Path does not exist.  [test/expectations] [5]
ERROR: LayoutTests/platform/gtk/TestExpectations:2812:  Path does not exist.  [test/expectations] [5]
Total errors found: 8 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location.html

[Build Bot]

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

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

[Joseph Pecoraro]

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

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

> Source/WebKit2/NetworkProcess/soup/NetworkDataTaskSoup.h:143
>  #if ENABLE(WEB_TIMING)
> -    double m_startTime { 0 };
> +    WebCore::NetworkLoadMetrics m_networkLoadMetrics;
> +    MonotonicTime m_startTime;
>  #endif

Hmm, one possible issue with this change on GTK here is that I'm not setting the values on the ResourceResponse which was what it was doing before. That will probably break NavigationTiming. It should probably be setting those values / copying them over before sending didReceiveResponse.

[Joseph Pecoraro]

Hmm, on my local machine I see the following asserts in iOS Simulator running these tests:

ASSERTION FAILED: response.httpStatusCode() < 300 || response.httpStatusCode() >= 400 || response.httpStatusCode() == 304 || !response.httpHeaderField(HTTPHeaderName::Location)
/Users/pecoraro/Code/safari/OpenSource/Source/WebCore/loader/SubresourceLoader.cpp(286) : virtual void WebCore::SubresourceLoader::didReceiveResponse(const WebCore::ResourceResponse &)
1   0x11160ae1d WTFCrash
2   0x115da33ed WebCore::SubresourceLoader::didReceiveResponse(WebCore::ResourceResponse const&)
3   0x108df0635 WebKit::WebResourceLoader::didReceiveResponse(WebCore::ResourceResponse const&, bool)
4   0x108df5654 void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool), std::__1::tuple<WebCore::ResourceResponse, bool>, 0ul, 1ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::ResourceResponse const&, bool), std::__1::tuple<WebCore::ResourceResponse, bool>&&, std::__1::integer_sequence<unsigned long, 0ul, 1ul>)

I don't see how my changes could have introduced this. Investigating.

[Joseph Pecoraro]

I see the following in the Simulator's Console:

    Feb 17 22:50:39 pecoraro com.apple.WebKit.Networking.Development[39610]: NSURLSessionTask finished with error - code: -1007
    Feb 17 22:50:39 pecoraro com.apple.WebKit.WebContent.Development[39611]: >>> httpStatusCode() - 301
    Feb 17 22:50:39 pecoraro com.apple.WebKit.WebContent.Development[39611]: >>> HTTPHeaderName::Location - ../resources/redirect.py?count=1&max_age=0&redirect_status=301&max_count=20&token=e36b436f-f1f5-447d-a2e1-023f92e87700&location=..%2Fresources%2Fredirect.py&count=17
    Feb 17 22:50:39 pecoraro com.apple.WebKit.WebContent.Development[39611]: ASSERTION FAILED: response.httpStatusCode() < 300 || response.httpStatusCode() >= 400 || response.httpStatusCode() == 304 || !response.httpHeaderField(HTTPHeaderName::Location)

It is super interesting that NSURLSessionTask had an error. So I wonder if that is related.

I don't think this failure is related to my changes though. Others have apparently been skipping these tests due to crashes:

    $ find-expectations fetch/api/redirect | grep simulator
    LayoutTests/platform/ios-simulator-wk1/TestExpectations:1405:imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location.html [ Crash Pass ]
    LayoutTests/platform/ios-simulator-wk1/TestExpectations:1874:imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count.html [ Crash Pass ]

But I'll continue to dig.

[Joseph Pecoraro]

https://developer.apple.com/reference/cfnetwork/cfnetworkerrors/kcfurlerrorhttptoomanyredirects?c_2

> kCFURLErrorHTTPTooManyRedirects = -1007

So for a test that is testing redirects, it seems this is not too unexpected.

--

Is this happening without my change?

[Joseph Pecoraro]

> Is this happening without my change?

Wow. When I revert my change I'm no longer seeing the asserts. So it clearly must be related to my change. But I don't see how... Hmm

[Joseph Pecoraro]

So just having an empty delegate:

> - (void)URLSession:(NSURLSession *)session task:(NSURLSessionTask *)task didFinishCollectingMetrics:(NSURLSessionTaskMetrics *)metrics { }

Is enough to cause iOS to hit the ASSERT. This seems like something I may need to work around. Just the existence is changing CFNetwork's behavior somehow. I'll have to work out exactly what changes.

[Joseph Pecoraro]

Okay, merely adding this new delegate causes CFNetwork to change how it sends the delegates in this error case.

    * If didFinishCollectingMetrics does not exist we only get a didCompleteWithError.
    * If didFinishCollectingMetrics does exist, we get a didReceiveResponse, didFinishCollectingMetrics, and didCompleteWithError.

I almost think the 2nd case is expected behavior, but I'm sure. I'll have to discuss this with others. It seems either way we will want to defend against this behavior since that is likely the case in Sierra.

[Joseph Pecoraro]

Created attachment 302180 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 302180 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/cocoa/NetworkLoadMetrics.mm:72:  No space between ^ and block definition.  [whitespace/brackets] [4]
WARNING: Not running on native Windows.
ERROR: LayoutTests/platform/win/TestExpectations:3787:  Path does not exist.  [test/expectations] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:104:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:105:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:106:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:108:  One space before end of line comments  [whitespace/comments] [5]
ERROR: LayoutTests/platform/mac/TestExpectations:1539:  Path does not exist.  [test/expectations] [5]
ERROR: LayoutTests/platform/gtk/TestExpectations:2828:  Path does not exist.  [test/expectations] [5]
Total errors found: 8 in 72 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-no-location.htm
http/tests/cache/disk-cache/disk-cache-307-status-code.html

[Build Bot]

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

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

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-no-location.htm
http/tests/cache/disk-cache/disk-cache-307-status-code.html

[Build Bot]

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

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

[Build Bot]

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

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

New failing tests:
http/tests/navigation/redirect302-metaredirect.html
imported/w3c/web-platform-tests/XMLHttpRequest/send-redirect-no-location.htm

[Build Bot]

Created attachment 302204 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Joseph Pecoraro]

Comment on attachment 302199 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

Alex and I talked about this. We're going to include the Timing information in didFinishLoading to cut down on the number of messages.

[Joseph Pecoraro]

Created attachment 302224 [details]
[PATCH] For Bots

[WebKit Commit Bot]

Attachment 302224 [details] did not pass style-queue:


ERROR: LayoutTests/platform/mac/TestExpectations:1539:  Path does not exist.  [test/expectations] [5]
ERROR: LayoutTests/platform/gtk/TestExpectations:2837:  Path does not exist.  [test/expectations] [5]
ERROR: Source/WebCore/platform/network/cocoa/NetworkLoadMetrics.mm:72:  No space between ^ and block definition.  [whitespace/brackets] [4]
WARNING: Not running on native Windows.
ERROR: LayoutTests/platform/win/TestExpectations:3787:  Path does not exist.  [test/expectations] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:104:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:105:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:106:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:108:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 8 in 109 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 302236 [details]
[PATCH] For Bots

This should fix the iOS Build, however already my failures are different from the bots so I'll need them to tell me why they are failing (different OS?)

[WebKit Commit Bot]

Attachment 302236 [details] did not pass style-queue:


ERROR: LayoutTests/platform/mac/TestExpectations:1539:  Path does not exist.  [test/expectations] [5]
ERROR: LayoutTests/platform/gtk/TestExpectations:2837:  Path does not exist.  [test/expectations] [5]
ERROR: Source/WebCore/platform/network/cocoa/NetworkLoadMetrics.mm:72:  No space between ^ and block definition.  [whitespace/brackets] [4]
WARNING: Not running on native Windows.
ERROR: LayoutTests/platform/win/TestExpectations:3787:  Path does not exist.  [test/expectations] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:104:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:105:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:106:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:108:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 8 in 109 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location.html

[Build Bot]

Created attachment 302243 [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 302236 [details]
[PATCH] For Bots

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin-worker.html

[Build Bot]

Created attachment 302244 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

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

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-worker.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location.html

[Build Bot]

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

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

[Build Bot]

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

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

New failing tests:
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location-worker.html

[Build Bot]

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

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

[Blaze Burg]

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

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

> Source/WTF/ChangeLog:12
> [phone]  Joseph Pecoraro  <[e-mail]>

Double changelog.

[Joseph Pecoraro]

Created attachment 302299 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 302299 [details] did not pass style-queue:


ERROR: LayoutTests/platform/mac/TestExpectations:1539:  Path does not exist.  [test/expectations] [5]
ERROR: LayoutTests/platform/gtk/TestExpectations:2828:  Path does not exist.  [test/expectations] [5]
ERROR: Source/WebCore/platform/network/cocoa/NetworkLoadMetrics.mm:72:  No space between ^ and block definition.  [whitespace/brackets] [4]
WARNING: Not running on native Windows.
ERROR: LayoutTests/platform/win/TestExpectations:3787:  Path does not exist.  [test/expectations] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:104:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:105:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:106:  One space before end of line comments  [whitespace/comments] [5]
ERROR: Source/WebCore/platform/network/NetworkLoadMetrics.h:108:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 8 in 104 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

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

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

> LayoutTests/platform/ios-simulator/TestExpectations:2880
> +imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin-worker.html [ Failure ]

What do these failures look like?  I think we should add platform expectations to see this and catch further regressions rather than just marking them as failing.

> Source/WebCore/loader/SubresourceLoader.cpp:288
> +    // cases of too many redirects from CFNetwork (<rdar://problem/30610988>).

Is there a way we could find out if the didReceiveResponse was after a kCFURLErrorHTTPTooManyRedirects and ignore it to work around this?

> Source/WebCore/platform/network/NetworkLoadMetrics.hSource/WebCore/platform/network/NetworkLoadTiming.h:70
> +        requestStart = Seconds(0);

I think it would be nicer if these used initializer lists.  m_networkLoadMetrics = { }; would then be used instead of .reset().

> Source/WebCore/platform/network/NetworkLoadMetrics.hSource/WebCore/platform/network/NetworkLoadTiming.h:108
> +    Seconds domainLookupStart;     // -1 if no DNS.
> +    Seconds domainLookupEnd;       // -1 if no DNS.
> +    Seconds connectStart;          // -1 if reused connection.
> +    Seconds secureConnectionStart; // -1 if no secure connection.
> +    Seconds connectEnd;            // -1 if reused connection.

Ideally these would be std::optional<Seconds> instead of the magic value of -1 if there is no value.

[Joseph Pecoraro]

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

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

>> LayoutTests/platform/ios-simulator/TestExpectations:2880
>> +imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-cross-origin-worker.html [ Failure ]
> 
> What do these failures look like?  I think we should add platform expectations to see this and catch further regressions rather than just marking them as failing.

A few PASS go to FAIL and expected rejections are not rejections. I spent a very long time trying to get results in the right place (once I add mac expectations then I need to re-add other port expectations, iOS, and mac-elcapitan). I think I should readdress this once CFNetwork looks at the radar. I think we'll need to address this at some point but it is already a weird case caused by too many redirects.

>> Source/WebCore/loader/SubresourceLoader.cpp:288
>> +    // cases of too many redirects from CFNetwork (<rdar://problem/30610988>).
> 
> Is there a way we could find out if the didReceiveResponse was after a kCFURLErrorHTTPTooManyRedirects and ignore it to work around this?

I looked for a way but did not find a way. This is really the first delegate we get and I didn't see a way to determine there is any at this point (it comes with the later delegate).

[Joseph Pecoraro]

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

[Chris Dumez]

I suspect this caused a lot of crashes on the bots:
ASSERTION FAILED: networkLoadMetrics.isComplete()
/Volumes/Data/WebKit/OpenSource/Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp(152) : void WebKit::WebResourceLoader::didFinishResourceLoad(const WebCore::NetworkLoadMetrics &)
1   0x107e6af9d WTFCrash
2   0x1021548fc WebKit::WebResourceLoader::didFinishResourceLoad(WebCore::NetworkLoadMetrics const&)
3   0x1021595e6 void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, 0ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>&&, std::__1::integer_sequence<unsigned long, 0ul>)
4   0x102159458 void IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, std::__1::integer_sequence<unsigned long, 0ul> >(std::__1::tuple<WebCore::NetworkLoadMetrics>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&))
5   0x102158732 void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)>(IPC::Decoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&))
6   0x102157e6c WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::Decoder&)
7   0x1019bf709 WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&)
8   0x1017442e3 IPC::Connection::dispatchMessage(IPC::Decoder&)
9   0x1017399a8 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)
10  0x1017448e0 IPC::Connection::dispatchOneMessage()
11  0x10175d4fd IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14::operator()()
12  0x10175d459 WTF::Function<void ()>::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_14>::call()
13  0x107e97a6e WTF::Function<void ()>::operator()() const
14  0x107eb3313 WTF::RunLoop::performWork()
15  0x107eb4b34 WTF::RunLoop::performWork(void*)
16  0x7fffab95a3c1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
17  0x7fffab93b2cd __CFRunLoopDoSources0
18  0x7fffab93a7c6 __CFRunLoopRun
19  0x7fffab93a1c4 CFRunLoopRunSpecific
20  0x7fffaae9bebc RunCurrentEventLoopInMode
21  0x7fffaae9bcf1 ReceiveNextEventCommon
22  0x7fffaae9bb26 _BlockUntilNextEventMatchingListInModeWithFilter
23  0x7fffa9436e24 _DPSNextEvent
24  0x7fffa9bb285e -[NSApplication(NSEvent) _nextEventMatchingEventMask:untilDate:inMode:dequeue:]
25  0x7fffa942b7ab -[NSApplication run]
26  0x7fffa93f61de NSApplicationMain
27  0x7fffc130f8c7 _xpc_objc_main
28  0x7fffc130e2e4 xpc_main
29  0x10166c132 main
30  0x7fffc10b6235 start
LEAK: 1 WebProcessPool

[Chris Dumez]

(In reply to comment #69)
> <https://trac.webkit.org/changeset/212944>

https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r212945%20(2249)/results.html

[Chris Dumez]

Reverted r212944 for reason:

Caused a lot of failures on the debug bots

Committed r212989: <http://trac.webkit.org/changeset/212989>

[Chris Dumez]

(In reply to comment #72)
> Reverted r212944 for reason:
> 
> Caused a lot of failures on the debug bots
> 
> Committed r212989: <http://trac.webkit.org/changeset/212989>

Also see failures in release at https://build.webkit.org/results/Apple%20Sierra%20Release%20WK1%20(Tests)/r212945%20(3950)/results.html

[Joseph Pecoraro]

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

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

> Source/WebKit2/WebProcess/Network/WebResourceLoader.cpp:153
> +#if USE(NETWORK_SESSION)
> +    ASSERT(networkLoadMetrics.isComplete());
> +#endif

This ASSERT is what was causing the crashes. It can just be removed. I used this to test the common code path was marking complete as expected. There are other code paths that can reach here that use empty metrics. It is fine to have empty metrics here (we may want to tweak their values later on though).

[Joseph Pecoraro]

> Also see failures in release at
> https://build.webkit.org/results/Apple%20Sierra%20Release%20WK1%20(Tests)/
> r212945%20(3950)/results.html

Ahh, yes these are WebKit1. That code path does not have the new code so this will need to be a Failure on WK1. Both fixes are simple so I'm going to Ireland this with the tiny changes.

Thanks for the swift action to keep the bots green Chris!

[Joseph Pecoraro]

> I'm going to Ireland this with the tiny changes.

I'm going to *reland* this with...

[Joseph Pecoraro]

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