Bug 186312 – Network Preflights do not show in WebInspector after moving CORS checks to NetworkProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[youenn fablet]

Network Preflights do not show in WebInspector after moving CORS checks to NetworkProcess

[youenn fablet]

<rdar://problem/40495434>

[youenn fablet]

Created attachment 341982 [details]
Patch

[EWS Watchlist]

Comment on attachment 341982 [details]
Patch

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

New failing tests:
http/tests/inspector/network/har/har-page.html
http/tests/inspector/network/fetch-network-data.html

[EWS Watchlist]

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

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

[youenn fablet]

Created attachment 341994 [details]
Patch

[youenn fablet]

Created attachment 342616 [details]
Patch

[Blaze Burg]

Comment on attachment 342616 [details]
Patch

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

LGTM, but I'd like someone on your team to also take a peek at NetworkProcess changes.

> Source/WebCore/platform/network/NetworkLoadInformation.h:36
> +struct NetworkIntermediateLoadInformation {

'Intermediate' seems weird here, what about Provisional? How are redirects called in the rest of loading code?

> Source/WebCore/platform/network/NetworkLoadInformation.h:70
> +    encoder << type << request << response << metrics;

I believe we use separate lines to make this easier to diff in the future.

> LayoutTests/ChangeLog:15
> [phone]  Youenn Fablet  <[e-mail]>

Nit: double changelog

[youenn fablet]

Thanks for the review.

(In reply to Brian Burg from comment #7)
> Comment on attachment 342616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342616&action=review
> 
> LGTM, but I'd like someone on your team to also take a peek at
> NetworkProcess changes.
> 
> > Source/WebCore/platform/network/NetworkLoadInformation.h:36
> > +struct NetworkIntermediateLoadInformation {
> 
> 'Intermediate' seems weird here, what about Provisional? How are redirects
> called in the rest of loading code?

I don't know if there is a name coined for a single request/response exchange.
Maybe NetworkTransactionInformation would be more appropriate.


> > Source/WebCore/platform/network/NetworkLoadInformation.h:70
> > +    encoder << type << request << response << metrics;
> 
> I believe we use separate lines to make this easier to diff in the future.

OK

> > LayoutTests/ChangeLog:15
> > [phone]  Youenn Fablet  <[e-mail]>
> 
> Nit: double changelog

OK

[youenn fablet]

Created attachment 342676 [details]
Patch

[Alex Christensen]

Comment on attachment 342676 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:445
> +void NetworkLoadChecker::storeRedirectionIfNeeded(WebCore::ResourceRequest&& request, WebCore::ResourceResponse&& response)

It would improve performance to make this function take const references and only copy if needed.

[youenn fablet]

Created attachment 342691 [details]
Patch

[youenn fablet]

(In reply to Alex Christensen from comment #10)
> Comment on attachment 342676 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=342676&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:445
> > +void NetworkLoadChecker::storeRedirectionIfNeeded(WebCore::ResourceRequest&& request, WebCore::ResourceResponse&& response)
> 
> It would improve performance to make this function take const references and
> only copy if needed.


Right, done!

[Chris Dumez]

Comment on attachment 342691 [details]
Patch

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

r=me with changes.

> Source/WebCore/testing/Internals.cpp:4577
> +String Internals::ongoingLoadDescriptions()

Shouldn't this be named ongoingLoadsDescriptions() ?

> Source/WebCore/testing/Internals.cpp:4580
> +    builder.append("[");

'[' is more efficient than "[".

> Source/WebCore/testing/Internals.cpp:4581
> +    bool isStarting = true;

I think isFirst may be a bit clearer.

> Source/WebCore/testing/Internals.cpp:4582
> +    auto loads = platformStrategies()->loaderStrategy()->ongoingLoads();

This variable is unused, please drop this line. The loop below is fine.

> Source/WebCore/testing/Internals.cpp:4587
> +            builder.append(",");

',' not ","

> Source/WebCore/testing/Internals.cpp:4589
> +        builder.append("[");

ditto.

> Source/WebCore/testing/Internals.cpp:4592
> +            builder.append(makeString("[", (int)info.type, ",\"", info.request.url().string(), "\",\"", info.request.httpMethod(), "\",", info.response.httpStatusCode(), "]"));

Is is really worth calling makeString() just to append to a StringBuilder? I would just append every part individually to the StringBuilder.

> Source/WebCore/testing/Internals.cpp:4594
> +        builder.append("]");

']'

> Source/WebCore/testing/Internals.cpp:4596
> +    builder.append("]");

']'

> Source/WebCore/testing/Internals.h:700
> +    String ongoingLoadDescriptions();

const ?

> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:77
> +    WebCore::NetworkTransactionInformation m_loadInformation;

I personally think this could be an std::optional given this is only set when m_shouldCaptureExtraNetworkLoadMetrics is true.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:81
> +    void takeNetworkLoadInformationRequest(ResourceLoadIdentifier identifier, WebCore::ResourceRequest& request)

This is not really "taking".

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.h:91
> +    void takeNetworkLoadIntermediateInformation(ResourceLoadIdentifier identifier, Vector<WebCore::NetworkTransactionInformation>& information)

This is not really "taking".

> Source/WebKit/NetworkProcess/PingLoad.cpp:45
> +    , m_networkLoadChecker(makeUniqueRef<NetworkLoadChecker>(connection, m_parameters.webPageID, m_parameters.webFrameID, m_parameters.identifier, FetchOptions { m_parameters.options}, m_parameters.sessionID, WTFMove(m_parameters.originalRequestHeaders), URL { m_parameters.request.url() }, m_parameters.sourceOrigin.copyRef(), m_parameters.preflightPolicy, m_parameters.request.httpReferrer(), false))

false is not nice here. I think we should use an enum class instead of a boolean.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:103
> +        return WTF::map(m_webResourceLoaders, [](auto&& keyValue) -> uint64_t {

why auto&& and not auto& ?
Do we really need the "-> uint64_t" ?

> LayoutTests/http/wpt/fetch/resources/preflight.py:16
> +    time.sleep(5);

That's not 5 seconds, is it?

[youenn fablet]

Created attachment 343052 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 343052 [details]
Patch for landing

Clearing flags on attachment: 343052

Committed r232963: <https://trac.webkit.org/changeset/232963>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

(In reply to Chris Dumez from comment #13)
> Comment on attachment 342691 [details]
> Patch

Thanks, I updated accordingly.

> > LayoutTests/http/wpt/fetch/resources/preflight.py:16
> > +    time.sleep(5);
> 
> That's not 5 seconds, is it?

It is but the test is not waiting for the fetch load to terminate.