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.
<rdar://problem/59462497>
Created attachment 390781 [details] Patch
Comment on attachment 390781 [details] Patch Looking into API test failures.
Created attachment 390956 [details] Patch
Comment on attachment 390956 [details] Patch Changed patch to match existing behavior of not preconnecting to localhost and respecting the preconnect flag on WebsiteDataStoreConfiguration.
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.
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.
Created attachment 391091 [details] Patch
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.
Created attachment 391095 [details] Patch
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.
Comment on attachment 391095 [details] Patch Clearing flags on attachment: 391095 Committed r256912: <https://trac.webkit.org/changeset/256912>
All reviewed patches have been landed. Closing bug.
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.
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.