Bug 192094

Summary: HTTPS Upgrade: Consult dummy storage for HTTPS Upgrade, Apply If Appropriate
Product: WebKit Reporter: Vivek Seth <v_seth>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, cdumez, commit-queue, ews-watchlist, ggaren, rhoule, v_seth
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews206 for win-future none

Description Vivek Seth 2018-11-28 11:52:02 PST
<rdar://problem/45851103>
Comment 1 Vivek Seth 2018-11-28 12:37:32 PST
Created attachment 355905 [details]
Patch
Comment 2 Chris Dumez 2018-11-28 13:05:47 PST
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.
Comment 3 Vivek Seth 2018-11-28 14:17:42 PST
Created attachment 355916 [details]
Patch
Comment 4 Chris Dumez 2018-11-28 14:36:24 PST
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.
Comment 5 Vivek Seth 2018-11-28 15:32:01 PST
Created attachment 355930 [details]
Patch
Comment 6 EWS Watchlist 2018-11-28 19:31:56 PST
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
Comment 7 EWS Watchlist 2018-11-28 19:32:07 PST
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
Comment 8 Chris Dumez 2018-11-28 19:45:55 PST
(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.
Comment 9 Chris Dumez 2018-11-28 19:47:36 PST
Comment on attachment 355930 [details]
Patch

r=me
Comment 10 WebKit Commit Bot 2018-11-28 20:14:09 PST
Comment on attachment 355930 [details]
Patch

Clearing flags on attachment: 355930

Committed r238659: <https://trac.webkit.org/changeset/238659>
Comment 11 WebKit Commit Bot 2018-11-28 20:14:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Alex Christensen 2018-11-28 23:17:57 PST
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.
Comment 13 Chris Dumez 2018-11-29 08:40:24 PST
(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.