http/tests/preconnect/link-rel-preconnect-http.html [ Timeout ] http/tests/preconnect/link-rel-preconnect-https.html [ Timeout ] fast/dom/HTMLLinkElement/preconnect-support.html [ Failure ]
Related failure, since introduced in r229308: http/tests/preconnect/link-header-rel-preconnect-http.html https://trac.webkit.org/changeset/229308/webkit
See https://w3c.github.io/resource-hints/#preconnect
Created attachment 408225 [details] WIP patch This is WIP only because it requires new libsoup API, see https://gitlab.gnome.org/GNOME/libsoup/-/merge_requests/136
Comment on attachment 408225 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=408225&action=review > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:482 > + auto error = SoupNetworkSession::checkTLSErrors(m_currentRequest.url(), certificate, tlsErrors); Why this?
Comment on attachment 408225 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=408225&action=review >> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:482 >> + auto error = SoupNetworkSession::checkTLSErrors(m_currentRequest.url(), certificate, tlsErrors); > > Why this? Because in case of preconnect we don't have a m_soupRequest. I can check m_shouldPreconnectOnly, but using m_currentRequest is just simpler and should work in both cases.
Created attachment 426541 [details] Patch This is now possible with soup3
Comment on attachment 408225 [details] WIP patch View in context: https://bugs.webkit.org/attachment.cgi?id=408225&action=review > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:247 > + soup_session_preconnect_async(soupSession, soupURI.get(), m_cancellable.get(), This would require guarding with ENABLE(SERVER_PRECONNECT), actually the whole if block (otherwise weird things could happen). I haven't checked the caller but I'll assume that parameters.shouldPreconnectOnly is properly guarded client side by ENABLE(SERVER_PRECONNECT) and could never be ::Yes if not enabled. > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:250 > + if (task.state() == State::Canceling || task.state() == State::Completed || !task.m_client) Consider refactoring the condition to a method as it'll be used in preconnectCallback too. > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:341 > +} What about using scopes for the clearRequest()? Something like auto clearRequestOnExit = makeScopeExit([task]() { task->clearRequest(); } My only doubt is which is executed first the RefPtr::deref of protectedThis or the scope exit. Maybe it's safer to do it as you did then...
(In reply to Sergio Villar Senin from comment #7) > Comment on attachment 408225 [details] > WIP patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=408225&action=review > > > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:247 > > + soup_session_preconnect_async(soupSession, soupURI.get(), m_cancellable.get(), > > This would require guarding with ENABLE(SERVER_PRECONNECT), actually the > whole if block (otherwise weird things could happen). > > I haven't checked the caller but I'll assume that > parameters.shouldPreconnectOnly is properly guarded client side by > ENABLE(SERVER_PRECONNECT) and could never be ::Yes if not enabled. > > > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:250 > > + if (task.state() == State::Canceling || task.state() == State::Completed || !task.m_client) > > Consider refactoring the condition to a method as it'll be used in > preconnectCallback too. > > > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:341 > > +} > > What about using scopes for the clearRequest()? Something like > > auto clearRequestOnExit = makeScopeExit([task]() { task->clearRequest(); } > > My only doubt is which is executed first the RefPtr::deref of protectedThis > or the scope exit. Maybe it's safer to do it as you did then... Sorry I was reviewing the old one. Forget about this.
Comment on attachment 426541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426541&action=review > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:256 > +#if !USE(SOUP2) What about && ENABLE(SERVER_PRECONNECT) ? > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:463 > +#if !USE(SOUP2) Ditto > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:478 > +} I'd suggest using makeScopeExit for the clearRequest() call. However I am not sure whether the RefPtr::deref is executed after it so we can leave it as it just in case.
Comment on attachment 426541 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426541&action=review >> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:256 >> +#if !USE(SOUP2) > > What about && ENABLE(SERVER_PRECONNECT) ? SERVER_PRECONNECT is enabled unconditionally when building with soup3 >> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:478 >> +} > > I'd suggest using makeScopeExit for the clearRequest() call. However I am not sure whether the RefPtr::deref is executed after it so we can leave it as it just in case. clearRequest should happen before the dispatchDidCompleteWithError.
Committed r276432 (236895@main): <https://commits.webkit.org/236895@main>