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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 4 14:36:12 PST 2018


Chris Dumez <cdumez at apple.com> changed:

           What    |Removed                     |Added
 Attachment #356527|review?                     |review-
              Flags|                            |

--- Comment #4 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 356527
  --> https://bugs.webkit.org/attachment.cgi?id=356527

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

> Source/WebKit/NetworkProcess/NetworkLoad.cpp:118
> +    if (!redirectCompletionHandler)

Why is this needed?

> Source/WebKit/NetworkProcess/NetworkLoad.h:69
> +    void willPerformHTTPRedirection(WebCore::ResourceResponse&&, WebCore::ResourceRequest&&, RedirectCompletionHandler&&) final;

Why is this needed?

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:58
> +    : didUpgradeCallback()

Not needed

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:232
> +    ResourceRequest oldRequest = request;

We normally call this "originalRequest"

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:235
> +    if (didUpgradeCallback && request.requester() == ResourceRequest::Requester::Main) {

Seems like the "request.requester() == ResourceRequest::Requester::Main" check could be inside applyHTTPSUpgradeIfNeeded().

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:258
> +    , oldRequest, request, didUpgradeRequest

Why are you capturing the request here? Also this is wrong since request is WTFMove()'d out on the same line.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:272

I do not think this part needs to be HTTPS_UPGRADE specific. I think we should simulate a redirect for all upgrades so that the Safari URL bar is up-to-date.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:273
> +        if (didUpgradeCallback && didUpgradeRequest) {

didUpgradeRequest can be replaced by checking if originalRequest's URL and result->value()'s request are different.

We may want to rename didUpgradeCallback() to didUpdateRequestURL()

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:274
> +            didUpgradeCallback(oldRequest, request, [this, request = WTFMove(result.value().request), handler = WTFMove(handler)]() mutable {

Why is this using request and not result->request?

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:88
> +    WTF::Function<void(WebCore::ResourceRequest, WebCore::ResourceRequest, WTF::Function<void(void)>)> didUpgradeCallback;

Why is this public? This should be a private data member.

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

Needs to be a proper setter. Setting a member like this is bad practice. Also parameters should probably not be passed by value:
[this](const ResourceRequest& originalRequest, const ResourceRequest& currentRequest, WTF::Function<void(void)>&& callback) {

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:125
> +            ResourceResponse redirectResponse { };

{ } is not needed.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:128
> +            redirectResponse.setHTTPVersion("HTTP/1.1");


> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:129
> +            redirectResponse.setHTTPHeaderField(String("Location"), currentRequest.url().string());


> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:130
> +            redirectResponse.setHTTPHeaderField(String("Cache-Control"), String("no-store"));

String("no-store") -> "no-store"_s

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:135
> +            callback();

Seems wrong to call the callback before continueWillSendRequest() has been called. 

Network process sends WillSendRequest IPC to the WebProcess with the proposed request
WebProcess may change the request
WebProcess sends ContinueWillSendRequest IPC back to the network process with the final URL.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:364
> +    m_networkLoadChecker->didUpgradeCallback = nullptr;

Why is this needed?

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/20181204/6a96086f/attachment-0001.html>

More information about the webkit-unassigned mailing list