Implement Curl specific proxy and TLS related settings on WebKit NetworkProcess.
Created attachment 348321 [details] PATCH
Attachment 348321 [details] did not pass style-queue: ERROR: Source/WebKit/PlatformWin.cmake:67: Alphabetical sorting problem. "UIProcess/API/C/curl/WKContextCurl.cpp" should be before "UIProcess/win/WebView.cpp". [list/order] [5] ERROR: Source/WebKit/UIProcess/API/C/curl/WKContextCurl.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/win/WebProcessPoolWin.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebKit/NetworkProcess/curl/NetworkProcessCurl.cpp:113: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 4 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348333 [details] PATCH
Created attachment 348334 [details] PATCH
Attachment 348334 [details] did not pass style-queue: ERROR: Source/WebKit/PlatformWin.cmake:69: Alphabetical sorting problem. "UIProcess/curl/WebProcessPoolCurl.cpp" should be before "UIProcess/win/WebView.cpp". [list/order] [5] ERROR: Source/WebKit/UIProcess/API/C/curl/WKContextCurl.cpp:30: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit/UIProcess/win/WebProcessPoolWin.cpp:33: Bad include order. Mixing system and custom headers. [build/include_order] [4] ERROR: Source/WebKit/NetworkProcess/curl/NetworkProcessCurl.cpp:113: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 4 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 348339 [details] PATCH
Comment on attachment 348339 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=348339&action=review > Source/WebKit/NetworkProcess/win/NetworkProcessWin.cpp:38 > + platformInitializeNetworkProcessCurl(parameters); I don't think we should be going against the other ports in terms of how the NetworkProcess is being laid out.
(In reply to Don Olmstead from comment #7) > Comment on attachment 348339 [details] > PATCH > > View in context: > https://bugs.webkit.org/attachment.cgi?id=348339&action=review > > > Source/WebKit/NetworkProcess/win/NetworkProcessWin.cpp:38 > > + platformInitializeNetworkProcessCurl(parameters); > > I don't think we should be going against the other ports in terms of how the > NetworkProcess is being laid out. Our private port also uses Curl and it requires some special initialization. It's a little bit weird because There's no special extra initialization on Windows side, but the separation is needed for our platform.
Comment on attachment 348339 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=348339&action=review I think you should make this change testable, for example, by adding proxy and TLS setting UI for MiniBrowser. > Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:30 > +#include "NetworkProcess.h" You don't need to add this line. > Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:35 > +#include <WebCore/URL.h> You should not include URL.h because CurlProxySettings.h includes URL.h. > Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:134 > + ASSERT(!settings.isEmpty()); IIUC, this assertion can fail if the client calls WKContextSetNetworkProxySettings with kWKNetworkProxyModeCustom and an empty proxyUrl. > Source/WebKit/UIProcess/API/C/curl/WKContextCurl.cpp:30 > +#include "WebProcessPool.h" Remove this blank line. https://webkit.org/code-style-guidelines/#include-system > Source/WebKit/UIProcess/API/C/curl/WKContextCurl.h:44 > +WK_EXPORT WKStringRef WKContextGetSslCACertPath(WKContextRef); You are using toCopiedAPI to return a copy of string. You should name an API Copy instead of Get if you return a copy of string in WK API. For example, WKPageCopyTitle. And, You should name SSL instead of Ssl, for exameple WKContextSetAllowsAnySSLCertificateForWebSocketTesting. WKContextGetSslCACertPath -> WKContextCopySSLCACertPath > Source/WebKit/UIProcess/win/WebProcessPoolWin.cpp:33 > + Remove this blank line. https://webkit.org/code-style-guidelines/#include-system
Thanks for review!
Comment on attachment 348339 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=348339&action=review > Source/WebKit/UIProcess/API/C/curl/WKContextCurl.cpp:41 > + case WKNetworkProxyMode::kWKNetworkProxyModeDefault : No space before : > Source/WebKit/UIProcess/API/C/curl/WKContextCurl.h:43 > +WK_EXPORT void WKContextSetSslCACertPath(WKContextRef, WKStringRef caCertPath); I have two problems with this: 1) This should be per WKWebsiteDataStoreRef, which corresponds with a NetworkSession. It may be tempting to add things to WKContext, but we're in the process of moving as much as we can away from WKContext. 2) I'm not sure this is a good design to expose these things via the API. I think instead the cacert data should just be included in the bundle of files and resources that constitute WebKit. There should definitely be a good default so that API users don't have to use this in order to get something working.
Comment on attachment 348339 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=348339&action=review >> Source/WebKit/NetworkProcess/curl/NetworkDataTaskCurl.cpp:30 >> +#include "NetworkProcess.h" > > You don't need to add this line. Got it. >> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:35 >> +#include <WebCore/URL.h> > > You should not include URL.h because CurlProxySettings.h includes URL.h. Got it. >> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:134 >> + ASSERT(!settings.isEmpty()); > > IIUC, this assertion can fail if the client calls WKContextSetNetworkProxySettings with kWKNetworkProxyModeCustom and an empty proxyUrl. Got it. >> Source/WebKit/UIProcess/API/C/curl/WKContextCurl.cpp:30 >> +#include "WebProcessPool.h" > > Remove this blank line. > https://webkit.org/code-style-guidelines/#include-system Got it. >> Source/WebKit/UIProcess/API/C/curl/WKContextCurl.cpp:41 >> + case WKNetworkProxyMode::kWKNetworkProxyModeDefault : > > No space before : Got it. >> Source/WebKit/UIProcess/API/C/curl/WKContextCurl.h:43 >> +WK_EXPORT void WKContextSetSslCACertPath(WKContextRef, WKStringRef caCertPath); > > I have two problems with this: > 1) This should be per WKWebsiteDataStoreRef, which corresponds with a NetworkSession. It may be tempting to add things to WKContext, but we're in the process of moving as much as we can away from WKContext. > 2) I'm not sure this is a good design to expose these things via the API. I think instead the cacert data should just be included in the bundle of files and resources that constitute WebKit. There should definitely be a good default so that API users don't have to use this in order to get something working. 1) I see. I try moving them to WKWebsiteDataStoreRef. 2) From our system's requirement, we have to pass root ca information supplied by the library client. >> Source/WebKit/UIProcess/API/C/curl/WKContextCurl.h:44 >> +WK_EXPORT WKStringRef WKContextGetSslCACertPath(WKContextRef); > > You are using toCopiedAPI to return a copy of string. You should name an API Copy instead of Get if you return a copy of string in WK API. > For example, WKPageCopyTitle. > And, You should name SSL instead of Ssl, for exameple WKContextSetAllowsAnySSLCertificateForWebSocketTesting. > WKContextGetSslCACertPath -> WKContextCopySSLCACertPath I see. Thanks. >> Source/WebKit/UIProcess/win/WebProcessPoolWin.cpp:33 >> + > > Remove this blank line. > https://webkit.org/code-style-guidelines/#include-system Got it.
Alex, we discussed about this and change our direction as removing SSL settings API. Then move those initialization into platform (i.e. Win, our platform) specific initialization when SSLHandle is initialized. The rest of patch is proxy setting API. Do you think those API also should also move to WKWebsiteDataStore? It seems Apple port doesn't have such, I want to hear your opinion. We need this proxy support on our platform by following WebKit manner.
If you need proxy settings, put them on WebsiteDataStore, not WebProcessPool.
(In reply to Alex Christensen from comment #14) > If you need proxy settings, put them on WebsiteDataStore, not WebProcessPool. Thanks.
By the way, on Apple platforms the proxy settings are configured system-wide or per-app underneath us, so we don't have to have that code in WebKit. I'm not sure what the libsoup code does.
(In reply to Alex Christensen from comment #16) > By the way, on Apple platforms the proxy settings are configured system-wide > or per-app underneath us, so we don't have to have that code in WebKit. I'm > not sure what the libsoup code does. Yep, that's why I asked to hear your opinion because I cannot find that in public. Gtk put the api in webcontext. That's why we've added in WebContext. But we follow your new design choice.
GTK should follow, too.
We store proxy settings in the SoupNetworkSession object. That's where we put all our platform-specific NetworkProcess configuration. In the public API, they're exposed via WebKitWebContext, which of course we can't change.
Created attachment 352390 [details] WIP Patch Alex, can you review is this the right direction to add proxy API?
Comment on attachment 352390 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352390&action=review > Source/WebCore/platform/network/NetworkStorageSession.h:138 > + WEBCORE_EXPORT void setProxySettings(const CurlProxySettings&) const; Probably should be CurlProxySettings&& since you are sinking the data (caller doesn't need it anymore), right? > Source/WebKit/NetworkProcess/NetworkProcess.h:306 > + void setNetworkProxySettings(PAL::SessionID, const WebCore::CurlProxySettings&); Ditto. > Source/WebKit/NetworkProcess/curl/NetworkProcessCurl.cpp:89 > + networkStorageSession->setProxySettings(settings); WTFMove? > Source/WebKit/NetworkProcess/curl/NetworkSessionCurl.cpp:40 > + networkStorageSession().setProxySettings(parameters.proxySettings); WTFMove! > Source/WebKit/NetworkProcess/curl/RemoteNetworkingContextCurl.cpp:45 > + SessionTracker::setSession(sessionID, NetworkSession::create(WTFMove(parameters.networkSessionParameters))); Yeah like this ;)
Comment on attachment 352390 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=352390&action=review Thanks, Michael! >> Source/WebCore/platform/network/NetworkStorageSession.h:138 >> + WEBCORE_EXPORT void setProxySettings(const CurlProxySettings&) const; > > Probably should be CurlProxySettings&& since you are sinking the data (caller doesn't need it anymore), right? Right. >> Source/WebKit/NetworkProcess/curl/RemoteNetworkingContextCurl.cpp:45 >> + SessionTracker::setSession(sessionID, NetworkSession::create(WTFMove(parameters.networkSessionParameters))); > > Yeah like this ;) Yeag! :D
Created attachment 353191 [details] PATCH Implement API for proxy configuration. Also add UI for Proxy on MiniBrowser.
Attachment 353191 [details] did not pass style-queue: ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 1 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Build Bot from comment #24) > Attachment 353191 [details] did not pass style-queue: > > > ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0: No copyright > message found. You should have a line: "Copyright [year] <Copyright Owner>" > [legal/copyright] [5] > Total errors found: 1 in 31 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. This file is generated and managed by VisualStudio.
(In reply to Basuke Suzuki from comment #25) > (In reply to Build Bot from comment #24) > > ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0: No copyright > > message found. You should have a line: "Copyright [year] <Copyright Owner>" > > [legal/copyright] [5] > > Total errors found: 1 in 31 files > > > > If any of these errors are false positives, please file a bug against > > check-webkit-style. > > This file is generated and managed by VisualStudio. So someone needs to update check-webkit-style to understand that.
(In reply to Darin Adler from comment #26) > (In reply to Basuke Suzuki from comment #25) > > (In reply to Build Bot from comment #24) > > > ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0: No copyright > > > message found. You should have a line: "Copyright [year] <Copyright Owner>" > > > [legal/copyright] [5] > > > Total errors found: 1 in 31 files > > > > > > If any of these errors are false positives, please file a bug against > > > check-webkit-style. > > > > This file is generated and managed by VisualStudio. > > So someone needs to update check-webkit-style to understand that. I'll do next week.
Comment on attachment 353191 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=353191&action=review > Source/WebKit/NetworkProcess/NetworkProcess.messages.in:37 > + SetNetworkProxySettings(PAL::SessionID sessionID, WebCore::CurlProxySettings settings) sessionID makes sense but soup equivalent is not doing so. I guess some GTK refactoring could be done based on that patch. > Source/WebKit/NetworkProcess/curl/NetworkProcessCurl.cpp:88 > + if (auto* networkStorageSession = NetworkStorageSession::storageSession(sessionID)) Can there be a case where we do not have any networkStorageSession? Should we ASSERT? > Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:133 > + encoder.encodeEnum(settings.mode()); We now use EnumTraits to encode the enum without encodeEnum. You should then be able to write: encoder << settings.mode(); > Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:141 > +bool ArgumentCoder<CurlProxySettings>::decode(Decoder& decoder, CurlProxySettings& settings) We now try to write decoders as: std::optional<CurlProxySettings> ArgumentCoder<CurlProxySettings>::decode(Decoder& decoder) Can that be done here? > Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:152 > + String url; I initially thought url should be URL but it is not. I wonder though whether it should be and if so if CurlProxySettings::url() should return a const URL&. > Source/WebKit/UIProcess/WebsiteData/curl/WebsiteDataStoreCurl.cpp:40 > +void WebsiteDataStore::setNetworkProxySettings(const WebCore::CurlProxySettings& proxySettings) Could be WebCore::CurlProxySettings&&? > Tools/MiniBrowser/win/Common.cpp:135 > + void setup() override override -> final here and below? > Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:130 > + WKWebsiteDataStoreDisableNetworkProxySettings(store); Maybe could be rewritten to: if (!m_proxy.enable) { WKWebsiteDataStoreDisableNetworkProxySettings(store); return; }
(In reply to youenn fablet from comment #28) > sessionID makes sense but soup equivalent is not doing so. > I guess some GTK refactoring could be done based on that patch. Thing is, WPE/GTK has no concept of network sessions: there's always exactly one per UI process. For this reason, I've never entirely understood why SessionID exists. I guess it's to allow a separate network session, e.g. for ephemeral views? (Epiphany always launches a new UI process for ephemeral mode, which is why I've never paid attention to this.)
Comment on attachment 353191 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=353191&action=review Thanks for reviewing! >> Source/WebKit/NetworkProcess/curl/NetworkProcessCurl.cpp:88 >> + if (auto* networkStorageSession = NetworkStorageSession::storageSession(sessionID)) > > Can there be a case where we do not have any networkStorageSession? > Should we ASSERT? I need to confirm this, but I believe it shouldn't be nil. Maybe ASSERT is good. >> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:133 >> + encoder.encodeEnum(settings.mode()); > > We now use EnumTraits to encode the enum without encodeEnum. > You should then be able to write: encoder << settings.mode(); Okay, I'll check that and try rewriting with it. >> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:141 >> +bool ArgumentCoder<CurlProxySettings>::decode(Decoder& decoder, CurlProxySettings& settings) > > We now try to write decoders as: > std::optional<CurlProxySettings> ArgumentCoder<CurlProxySettings>::decode(Decoder& decoder) > Can that be done here? Got it. Trying. >> Source/WebKit/UIProcess/WebsiteData/curl/WebsiteDataStoreCurl.cpp:40 >> +void WebsiteDataStore::setNetworkProxySettings(const WebCore::CurlProxySettings& proxySettings) > > Could be WebCore::CurlProxySettings&&? Right. >> Tools/MiniBrowser/win/Common.cpp:135 >> + void setup() override > > override -> final here and below? Ah, okay. That's better. >> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:130 >> + WKWebsiteDataStoreDisableNetworkProxySettings(store); > > Maybe could be rewritten to: > if (!m_proxy.enable) { > WKWebsiteDataStoreDisableNetworkProxySettings(store); > return; > } Okay, the point is stop nesting deep and use early return, right?
Comment on attachment 353191 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=353191&action=review >> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:152 >> + String url; > > I initially thought url should be URL but it is not. > I wonder though whether it should be and if so if CurlProxySettings::url() should return a const URL&. You are right. These are our mistake. Thanks. It should be String at anywhere. We'll change APIs and method calls to use String instead of URL. The background of this was here https://bugs.webkit.org/show_bug.cgi?id=184714#c11. Summary: There's no way to specify port 80 as a http proxy port. libcurl treat default port of http proxy to 1080 if port number is omitted. Once a string is stored in URL, port 80 is removed form the URL.
Comment on attachment 353191 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=353191&action=review >>> Source/WebKit/NetworkProcess/curl/NetworkProcessCurl.cpp:88 >>> + if (auto* networkStorageSession = NetworkStorageSession::storageSession(sessionID)) >> >> Can there be a case where we do not have any networkStorageSession? >> Should we ASSERT? > > I need to confirm this, but I believe it shouldn't be nil. Maybe ASSERT is good. I still am investigating this, but this kind of pattern are used in many place. https://github.com/WebKit/webkit/blob/5ebf9a85efa6c334e5a7d68bdd7d8cc4396b30a9/Source/WebKit/NetworkProcess/NetworkProcess.cpp#L548 if (auto* networkStorageSession = NetworkStorageSession::storageSession(sessionID)) networkStorageSession->removeAllStorageAccess(); else ASSERT_NOT_REACHED(); I don't see why, but it seems reasonable to follow this pattern.
Comment on attachment 353191 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=353191&action=review >>> Source/WebKit/Shared/curl/WebCoreArgumentCodersCurl.cpp:152 >>> + String url; >> >> I initially thought url should be URL but it is not. >> I wonder though whether it should be and if so if CurlProxySettings::url() should return a const URL&. > > You are right. These are our mistake. Thanks. It should be String at anywhere. We'll change APIs and method calls to use String instead of URL. > > The background of this was here https://bugs.webkit.org/show_bug.cgi?id=184714#c11. > > Summary: > There's no way to specify port 80 as a http proxy port. libcurl treat default port of http proxy to 1080 if port number is omitted. Once a string is stored in URL, port 80 is removed form the URL. No I was wrong. That was for libcurl implementation. We chose to act like standard. We translate the meaning of port and a user shouldn't care about this weird behavior of libcurl. Then these arguments are whole WebCore::URL and it should be used here.
Created attachment 353430 [details] FIX Fixed Youenn's reviewed points.
Attachment 353430 [details] did not pass style-queue: ERROR: Tools/MiniBrowser/win/MiniBrowserLibResource.h:0: No copyright message found. You should have a line: "Copyright [year] <Copyright Owner>" [legal/copyright] [5] Total errors found: 1 in 32 files If any of these errors are false positives, please file a bug against check-webkit-style.
Review please? Youenn?
Comment on attachment 353430 [details] FIX LGTM, some nits below. Usually, when we add new API, it is a good practice to write some unit tests if the API is not tested as part of WebKitTestRunner. I am not sure how much in that case that will help. Currently our API tests do not run concurrently to an HTTP server. View in context: https://bugs.webkit.org/attachment.cgi?id=353430&action=review > Source/WebCore/platform/network/NetworkStorageSession.h:139 > + WEBCORE_EXPORT void setProxySettings(CurlProxySettings&&) const; It is strange to have setters be const, mutable behind I guess. Should we make it non const? Ditto for setCookieDatabase. > Source/WebKit/ChangeLog:8 > + Added proxy configuration API to WebsiteDataStore. Three API was added in /WKWebsiteDataStoreRefCurl.h: s/was/were/. I would remove the / from WKWebsiteDataStoreRefCurl.h > Source/WebKit/NetworkProcess/curl/RemoteNetworkingContextCurl.cpp:42 > + return; We probably have the same first 3 lines in SOUP/Mac ports. Probably some refactoring could be done but it might be better as a follow-up. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1668 > #endif There is some potential refactoring code that could also be done here as well to share this code with Mac. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:198 > + const WebCore::CurlProxySettings& networkProxySettings() { return m_proxySettings; } Should be const probably.
Comment on attachment 353430 [details] FIX View in context: https://bugs.webkit.org/attachment.cgi?id=353430&action=review Thanks! Okay, try to put test in TestWebKitAPI. >> Source/WebCore/platform/network/NetworkStorageSession.h:139 >> + WEBCORE_EXPORT void setProxySettings(CurlProxySettings&&) const; > > It is strange to have setters be const, mutable behind I guess. > Should we make it non const? > Ditto for setCookieDatabase. Right. We'll change. >> Source/WebKit/ChangeLog:8 >> + Added proxy configuration API to WebsiteDataStore. Three API was added in /WKWebsiteDataStoreRefCurl.h: > > s/was/were/. > I would remove the / from WKWebsiteDataStoreRefCurl.h Okay. >> Source/WebKit/NetworkProcess/curl/RemoteNetworkingContextCurl.cpp:42 >> + return; > > We probably have the same first 3 lines in SOUP/Mac ports. > Probably some refactoring could be done but it might be better as a follow-up. Okay. I'll think about that. >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1668 >> #endif > > There is some potential refactoring code that could also be done here as well to share this code with Mac. Got it. >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:198 >> + const WebCore::CurlProxySettings& networkProxySettings() { return m_proxySettings; } > > Should be const probably. Right.
Created attachment 354367 [details] PATCH
Comment on attachment 354367 [details] PATCH We'll open a separate bug to add API test for this change. Thanks Youenn for r+.
Comment on attachment 354367 [details] PATCH Clearing flags on attachment: 354367 Committed r238051: <https://trac.webkit.org/changeset/238051>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45953089>