Set User-Agent in preconnect requests

• Component: Page Loading
• Status: RESOLVED FIXED

[Ben Nham]

When using an HTTPS proxy, CFNetwork will not reuse a preconnected socket if the User-Agent on the preconnect request doesn't match the User-Agent of the actual request (<rdar://problem/59434166>). To work around this, we should set the User-Agent on preconnect requests.

In addition, this patch moves the preconnect request from WebProcess::loadRequest in the WebProcess to WebProcessProxy::loadRequest in the UIProcess. This is because there can be long sync IPCs that last >100 ms that occur before WebProcess::loadRequest even starts, e.g. https://bugs.webkit.org/show_bug.cgi?id=203165.

By making both changes, we see a ~2% improvement in PLT5 times.

[Radar WebKit Bug Importer]

<rdar://problem/59462497>

[Ben Nham]

Created attachment 390781 [details]
Patch

[Ben Nham]

Comment on attachment 390781 [details]
Patch

Looking into API test failures.

[Ben Nham]

Created attachment 390956 [details]
Patch

[Ben Nham]

Comment on attachment 390956 [details]
Patch

Changed patch to match existing behavior of not preconnecting to localhost and respecting the preconnect flag on WebsiteDataStoreConfiguration.

[Chris Dumez]

Comment on attachment 390956 [details]
Patch

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:1509
> +void WebProcessPool::preconnectToServer(PAL::SessionID sessionID, const URL& url, const String& userAgent)

I don't think we need this method on the WebProcessPool. Seems like the WebPageProxy could call a method on the networkprocessproxy directly.

> Source/WebKit/UIProcess/WebProcessProxy.cpp:617
> +void WebProcessProxy::preconnectTo(const URL& url, const String& userAgent)

I don't think it makes much sense to have a preconnect method on the WebProcessProxy. It should likely go on the NetworkProcessProxy.

[Chris Dumez]

Comment on attachment 390956 [details]
Patch

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

> Source/WebKit/ChangeLog:18
> +        Reviewed by NOBODY (OOPS!).

FYI, this usually comes *before* the change log description.

[Ben Nham]

Created attachment 391091 [details]
Patch

[Chris Dumez]

Comment on attachment 391091 [details]
Patch

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

r=me

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1307
> +    if (!url.isValid() || !url.protocolIsInHTTPFamily() || SecurityOrigin::isLocalHostOrLoopbackIPAddress(url.host()))

Why do we need this check here? You're already doing them at the call site.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4236
> +        if (!networkProcess->canSendMessage())

Should not be needed.

[Ben Nham]

Created attachment 391095 [details]
Patch

[Ben Nham]

Comment on attachment 391095 [details]
Patch

Addressed comments and requested cq in the latest version of the patch.

(In reply to Chris Dumez from comment #9)
> Comment on attachment 391091 [details]
> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1307
> > +    if (!url.isValid() || !url.protocolIsInHTTPFamily() || SecurityOrigin::isLocalHostOrLoopbackIPAddress(url.host()))
> 
> Why do we need this check here? You're already doing them at the call site.

Are we allowed to trust the IPC channel? That's the only reason I was doing the parameter sanity checks on both the client side in NetworkProcessProxy and on the server side in NetworkProcess.

[WebKit Commit Bot]

Comment on attachment 391095 [details]
Patch

Clearing flags on attachment: 391095

Committed r256912: <https://trac.webkit.org/changeset/256912>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/256912/webkit

broke 2 API tests on Mac Debug: 
TestWebKitAPI.WebKit.DownloadNavigationResponseFromMemoryCache
TestWebKitAPI.ContentFiltering.LazilyLoadPlatformFrameworks

stdio:
https://build.webkit.org/builders/Apple-Catalina-Debug-WK2-Tests/builds/2404/steps/run-api-tests/logs/stdio

I was able to reproduce this test locally by just running these two tests.

[Ben Nham]

Those are debug asserts that are firing because I forgot to avoid preconnecting to custom URL schemes, which should be fixed in https://bugs.webkit.org/show_bug.cgi?id=207964.