Summary: | [SOUP] Add support for preconnect | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Miguel Gomez <magomez> | ||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | annulen, bugs-noreply, cdumez, cgarcia, csaavedra, ews-watchlist, gyuyoung.kim, pgriffis, ryuan.choi, sergio, svillar, zan | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=224955 | ||||||||
Attachments: |
|
Description
Miguel Gomez
2017-10-05 05:44:32 PDT
Related failure, since introduced in r229308: http/tests/preconnect/link-header-rel-preconnect-http.html https://trac.webkit.org/changeset/229308/webkit 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> |