Bug 192094 – HTTPS Upgrade: Consult dummy storage for HTTPS Upgrade, Apply If Appropriate

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Vivek Seth]

<rdar://problem/45851103>

[Vivek Seth]

Created attachment 355905 [details]
Patch

[Chris Dumez]

Comment on attachment 355905 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:197
> +    URL url = request.url();

We do not technically need to copy the URL here, only later if we upgrade. Therefore I would use
auto& url = .. here.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:199
> +    // Only upgrade http urls

WebKit comments need to end with a period.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:200
> +    if (url.protocol() == "http") {

url.protocolIs("http")

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:202
> +        Vector<String> upgradableHosts = {

You do not want to construct this vector every time this method is called, this would be expensive. This should be static:

static NeverDestroyed<HashSet<String>> upgradableHosts = std::initializer_list<String> {
    "www.bbc.com"_s, // (source: https://whynohttps.com)
    "www.speedtest.net"_s, // (source: https://whynohttps.com)
    "www.bea.gov"_s // (source: https://pulse.cio.gov/data/domains/https.csv)
};

Also, please move the static outside the if scope.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:207
> +        auto requestHost = url.host();

We do not really use this local variable, just use url.host() below.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:210
> +            url.setProtocol("https");

"https"_s is slightly more efficient.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:226
> +            ASSERT(request.url().protocol() == "https");

protocolIs("https")

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:143
> +

I do not think we need this blank line.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:145
> +

I do not think we need this blank line.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:198
> +#if ENABLE(HTTPS_UPGRADE)

I do not think this should be specific to HTTPS_UPGRADE. CSP / content extensions can also upgrade the URL. I think we should just use result.value() instead of originalRequest() in this block.

[Vivek Seth]

Created attachment 355916 [details]
Patch

[Chris Dumez]

Comment on attachment 355916 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:44
> +#include <wtf/Vector.h>

Not needed.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:207
> +    if (url.protocolIs("http"_s)) {

In webKit, we prefer early returns whenever possible. So this would be:
if (!url.protocolIs("http"))
    return false;

if (!upgradableHosts.get().contains(url.host().toString()))
    return false;

auto newURL = url;
newURL.setProtocol("https"_s);
request.setURL(newURL);
return true;

Note that I used url.protocolIs("http") instead of url.protocolIs("http"_s) as well. protocolIs() takes in a const char*, not a String.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:226
> +            ASSERT(request.url().protocolIs("https"_s));

no _s.

> Source/WebKit/NetworkProcess/NetworkLoadChecker.h:143
> +    bool applyHTTPSUpgradeIfNeeded(WebCore::ResourceRequest&);

can this be static? It does not look like it uses any members.

[Vivek Seth]

Created attachment 355930 [details]
Patch

[EWS Watchlist]

Comment on attachment 355930 [details]
Patch

Attachment 355930 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10191074

New failing tests:
webanimations/leak-document-with-web-animation.html

[EWS Watchlist]

Created attachment 355961 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Chris Dumez]

(In reply to Build Bot from comment #6)
> Comment on attachment 355930 [details]
> Patch
> 
> Attachment 355930 [details] did not pass win-ews (win):
> Output: https://webkit-queues.webkit.org/results/10191074
> 
> New failing tests:
> webanimations/leak-document-with-web-animation.html

You can disregard this failure, this is a flaky test.

[Chris Dumez]

Comment on attachment 355930 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 355930 [details]
Patch

Clearing flags on attachment: 355930

Committed r238659: <https://trac.webkit.org/changeset/238659>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alex Christensen]

Comment on attachment 355930 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:196
> +    // Use dummy list for now.

I'm not sure this dummy list should have been committed.

[Chris Dumez]

(In reply to Alex Christensen from comment #12)
> Comment on attachment 355930 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=355930&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:196
> > +    // Use dummy list for now.
> 
> I'm not sure this dummy list should have been committed.

Let's discuss this offline. My understanding is that this is what was agreed on during my last sync-up meeting but I could be wrong.