[Webkit-unassigned] [Bug 192375] HTTPS Upgrade: Figure out if/how to tell clients that the HTTPS upgrade happened

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=192375

--- Comment #6 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 356651
  --> https://bugs.webkit.org/attachment.cgi?id=356651
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:194
>  bool NetworkLoadChecker::applyHTTPSUpgradeIfNeeded(ResourceRequest& request)

Existing issue but applyHTTPSUpgradeIfNeeded is called before checking for CSP while https://fetch.spec.whatwg.org/#main-fetch is doing it after.
I would tend to fix this if we want to do that for iframes.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:197
> +        return false;

Should it be done for top level page only or iframes as well? Requester::Main applies to iframes as well.
If we only want for top level pages, I would tend to have a simpler mechanism and do that directly in NetworkResourceLoader.
That would remove the need to have this double callback approach.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:229
> +    // Only upgade if there is a way to notify parent.

s/upgade/upgrade

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:230
> +    if (m_didUpdateRequestURL) {

I would do this if check in applyHTTPSUpgradeIfNeeded.

The name m_didUpdateRequestURL seems like a boolean while it is a callback.
It might be good to change the name to m_didUpdateRequestURLCallback

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:250
> +    processContentExtensionRulesForLoad(WTFMove(request), [this, handler = WTFMove(handler), originalRequest](auto result) mutable {

originalRequest = WTFMove(originalRequest)

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:265
> +                this->continueCheckingRequest(WTFMove(request), WTFMove(handler));

Are we sure 'this' will be valid once this callback is called?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:123
> +        m_networkLoadChecker->setDidUpdateRequestURL([this](ResourceRequest& originalRequest, ResourceRequest& currentRequest, WTF::Function<void(void)>&& callback) {

We move below originalRequest and currentRequest while they are not r values.
This is dangerous, we should fix this.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.h:216
> +    WTF::Function<void(void)> m_didSendSimulatedRedirectCallback;

s/void(void)/void()/

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181205/39f105e8/attachment.html>