Summary: | HTTPS Upgrade: Consult dummy storage for HTTPS Upgrade, Apply If Appropriate | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vivek Seth <v_seth> | ||||||||||
Component: | Page Loading | Assignee: | 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
Vivek Seth
2018-11-28 11:52:02 PST
Created attachment 355905 [details]
Patch
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. Created attachment 355916 [details]
Patch
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. Created attachment 355930 [details]
Patch
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 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
(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 on attachment 355930 [details]
Patch
r=me
Comment on attachment 355930 [details] Patch Clearing flags on attachment: 355930 Committed r238659: <https://trac.webkit.org/changeset/238659> All reviewed patches have been landed. Closing bug. 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. (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. |