Bug 177934 – [SOUP] Add support for preconnect

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Miguel Gomez]

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 ]

[Zan Dobersek]

Related failure, since introduced in r229308:
http/tests/preconnect/link-header-rel-preconnect-http.html

https://trac.webkit.org/changeset/229308/webkit

[Carlos Garcia Campos]

See https://w3c.github.io/resource-hints/#preconnect

[Carlos Garcia Campos]

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

[Sergio Villar Senin]

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?

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 426541 [details]
Patch

This is now possible with soup3

[Sergio Villar Senin]

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...

[Sergio Villar Senin]

(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.

[Sergio Villar Senin]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Committed r276432 (236895@main): <https://commits.webkit.org/236895@main>