WKWebView._negotiatedLegacyTLS should be correct after back/forward navigations

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

WKWebView._negotiatedLegacyTLS should be correct after back/forward navigations

[Alex Christensen]

Created attachment 393396 [details]
Patch

[Alex Christensen]

Created attachment 393409 [details]
Patch

[youenn fablet]

Comment on attachment 393409 [details]
Patch

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

> Source/WebCore/history/CachedFrame.cpp:322
> +        if (cachedFrame && cachedFrame->usedLegacyTLS() == UsedLegacyTLS::Yes)

s/cachedFrame &&// as probably we cannot have null child frames.
We could use a Vector<UniqueRef> to make it clearer.

> Source/WebKit/NetworkProcess/NetworkDataTask.cpp:114
> +    response.setSource(ResourceResponse::Source::Network);

Why do we need this change?
We are still setting the source for redirections in NetworkLoad.
It seems better to keep everything in one place.

> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:136
> +    if (response.usedLegacyTLS()) {

You probably also want to check the source, like if (response.usedLegacyTLS() && response.source() == Network).
If it is from the network cache or service worker, we probably do not want to take that into account, as I understand the current implementation.

If we wanted to take into account appcache/memorycache, the check should be done inside DocumentLoader.
Service worker case is interesting since if a document is using it, it will never have usedLegacyTLS currently.
Maybe we should fix this.

[Alex Christensen]

Comment on attachment 393409 [details]
Patch

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

>> Source/WebCore/history/CachedFrame.cpp:322
>> +        if (cachedFrame && cachedFrame->usedLegacyTLS() == UsedLegacyTLS::Yes)
> 
> s/cachedFrame &&// as probably we cannot have null child frames.
> We could use a Vector<UniqueRef> to make it clearer.

Good idea.  Done.

>> Source/WebKit/NetworkProcess/NetworkDataTask.cpp:114
>> +    response.setSource(ResourceResponse::Source::Network);
> 
> Why do we need this change?
> We are still setting the source for redirections in NetworkLoad.
> It seems better to keep everything in one place.

NetworkLoad is an unneeded abstraction that was originally representing either an NSURLSession load or an NSURLConnection load.  Since we don't use NSURLConnection in modern WebKit anymore, we should move everything from it and remove it.  As long as I'm changing the didReceiveResponse code and putting things here, This may as well go as well.

>> Source/WebKit/WebProcess/Network/WebResourceLoader.cpp:136
>> +    if (response.usedLegacyTLS()) {
> 
> You probably also want to check the source, like if (response.usedLegacyTLS() && response.source() == Network).
> If it is from the network cache or service worker, we probably do not want to take that into account, as I understand the current implementation.
> 
> If we wanted to take into account appcache/memorycache, the check should be done inside DocumentLoader.
> Service worker case is interesting since if a document is using it, it will never have usedLegacyTLS currently.
> Maybe we should fix this.

Good idea, but moving it to DocumentLoader doesn't fix this.  Moving it to ResourceLoader does.  That looks like a better place for it.

[Alex Christensen]

Created attachment 393506 [details]
Patch

[youenn fablet]

Comment on attachment 393506 [details]
Patch

lgtm
As of setSource, it does not look great to have it in two places. I would leave that for a future refactoring that would add a platform independent redirection handler in NetworkDataTask so that setSource is no longer called in NetworkLoad.
Also, I am not sure NetworkTask is the right place. Source is only useful for WebProcess so this seems more a NetworkResourceLoader thing

[WebKit Commit Bot]

Comment on attachment 393506 [details]
Patch

Clearing flags on attachment: 393506

Committed r258458: <https://trac.webkit.org/changeset/258458>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60443735>