Move HTTP_UPGRADE code behind a runtime flag, off by default and drop the build time flag.
Created attachment 357821 [details] Patch
Created attachment 357823 [details] Patch
Created attachment 357826 [details] Patch
Created attachment 357827 [details] Patch
make: *** No rule to make target `HTTPSUpgradeList.txt', needed by `HTTPSUpgradeList.db'. Stop. make: *** Waiting for unfinished jobs.... Command /bin/sh failed with exit code 2 :/
Comment on attachment 357827 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357827&action=review > Source/WebKit/DerivedSources.make:320 > HTTPSUpgradeList.db : HTTPSUpgradeList.txt $(WebKit2)/Scripts/generate-https-upgrade-database.sh Should this be in WebKitDerivedSourcesAdditions.make ?
(In reply to Chris Dumez from comment #6) > Comment on attachment 357827 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357827&action=review > > > Source/WebKit/DerivedSources.make:320 > > HTTPSUpgradeList.db : HTTPSUpgradeList.txt $(WebKit2)/Scripts/generate-https-upgrade-database.sh > > Should this be in WebKitDerivedSourcesAdditions.make ? Yes.
Created attachment 357830 [details] Patch
Comment on attachment 357830 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357830&action=review > Source/WebKit/Shared/HTTPSUpgrade/HTTPSUpgradeList.txt:1 > +www.bbc.com Should we use a larger list of domains from the HSTS list?
Created attachment 357833 [details] Patch
Comment on attachment 357833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357833&action=review > Source/WebKit/DerivedSources.make:319 > +VPATH += $(WebKit2)/Shared/HTTPSUpgrade/ I thought we were adding this to WebKitDerivedSourcesAdditions.make. Did that not work?
> /Volumes/Data/EWS/WebKit/Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:95> :13: error: unused variable 'bindTextResult' [-Werror,-Wunused-variable] > int bindTextResult = m_statement->bindText(1, WTFMove(host)); > ^ > /Volumes/Data/EWS/WebKit/Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:10> 1:13: error: unused variable 'resetResult' [-Werror,-Wunused-variable] > int resetResult = m_statement->reset(); > ^ > 2 errors generated. This was my mistake, ASSERTS are compiled out on release builds. I didn't realize unused variables would cause errors. Here is one solution: void NetworkHTTPSUpgradeChecker::query(String&& host, CompletionHandler<void(bool)>&& callback) { ASSERT(isMainThread()); if (!this->m_isSetupComplete) { RELEASE_LOG_IF_ALLOWED("query - database setup is not complete, ignoring query."); callback(false); return; } m_workQueue->dispatch([this, host = WTFMove(host), callback = WTFMove(callback)] () mutable { auto runCallbackOnMainThread = [callback = WTFMove(callback)] (bool foundHost) mutable { WTF::RunLoop::main().dispatch([foundHost, callback = WTFMove(callback)] () mutable { callback(foundHost); }); }; if (this->m_statement->bindText(1, WTFMove(host)) != SQLITE_OK) { ASSERT(false); runCallbackOnMainThread(false); return; } int stepResult = this->m_statement->step(); if (stepResult != SQLITE_ROW && stepResult != SQLITE_DONE) { ASSERT(false); runCallbackOnMainThread(false); return; } if (this->m_statement->reset() != SQLITE_OK) { ASSERT(false); runCallbackOnMainThread(false); return; } bool foundHost = (stepResult == SQLITE_ROW); RELEASE_LOG_IF_ALLOWED("query - Ran successfully. Result = %s", (foundHost ? "true" : "false")); runCallbackOnMainThread(foundHost); }); }
Created attachment 357841 [details] Patch
(In reply to Vivek Seth from comment #11) > Comment on attachment 357833 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=357833&action=review > > > Source/WebKit/DerivedSources.make:319 > > +VPATH += $(WebKit2)/Shared/HTTPSUpgrade/ > > I thought we were adding this to WebKitDerivedSourcesAdditions.make. Did > that not work? I haven't tried, I thought it'd be better to have a minimal db in OpenSource.
Comment on attachment 357833 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357833&action=review >>> Source/WebKit/DerivedSources.make:319 >>> +VPATH += $(WebKit2)/Shared/HTTPSUpgrade/ >> >> I thought we were adding this to WebKitDerivedSourcesAdditions.make. Did that not work? > > I haven't tried, I thought it'd be better to have a minimal db in OpenSource. Yeah that would be good.
Comment on attachment 357841 [details] Patch Looks good to me!
Comment on attachment 357841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357841&action=review > Source/WebKit/NetworkProcess/PingLoad.cpp:46 > + , m_networkLoadChecker(makeUniqueRef<NetworkLoadChecker>(FetchOptions { m_parameters.options}, m_parameters.sessionID, m_parameters.webPageID, m_parameters.webFrameID, WTFMove(m_parameters.originalRequestHeaders), URL { m_parameters.request.url() }, m_parameters.sourceOrigin.copyRef(), m_parameters.preflightPolicy, false, m_parameters.request.httpReferrer())) Not very clear what false means here. Maybe isHTTPSUpgradeEnabled should be the last parameter and set to false by default, like requestLoadType.
Comment on attachment 357841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=357841&action=review > Source/WebKit/DerivedSources.make:319 > -ifeq ($(ENABLE_HTTPS_UPGRADE),ENABLE_HTTPS_UPGRADE) > +VPATH += $(WebKit2)/Shared/HTTPSUpgrade/ Nice > Source/WebKit/Shared/HTTPSUpgrade/HTTPSUpgradeList.txt:3 > +www.bbc.com > +www.speedtest.net > +www.bea.gov webkit.org? > Source/WebKit/Shared/WebPreferences.yaml:42 > + humanReadableDescription: "Automatic HTTPS upgrade for sites that support it" For the sites that we know about, at least :)
Created attachment 357886 [details] Patch
Created attachment 357888 [details] Patch
Comment on attachment 357888 [details] Patch Clearing flags on attachment: 357888 Committed r239474: <https://trac.webkit.org/changeset/239474>
All reviewed patches have been landed. Closing bug.
<rdar://problem/46887131>