Bug 209465 – [Curl] Proxy setting API does not work until Network process starts.

• Component: Platform
• Status: NEW

[Takashi Komori]

On Curl port we set proxy server by calling WKWebsiteDataStoreEnableCustomNetworkProxySettings.
But the API is not working before start of the Network process.
This is because the API can't send proxy setting message to the process which doesn't start yet.

In this ticket we will fix the flaw by passing proxy setting with initialization parameters to the Network process.

[Takashi Komori]

Created attachment 394356 [details]
Patch

[Takashi Komori]

Curl.EnableProxyBeforeLoading test checks the situation WKWebsiteDataStoreEnableCustomNetworkProxySettings is invoked before start of Network Process.

[Basuke Suzuki]

Comment on attachment 394356 [details]
Patch

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

This is an informal review.

> Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:98
> +    auto [proxyServer, proxyURL] = createServer("", 59001, "Respond from proxy server");

I cannot follow the logic. It seems this is the test for proxy server setting and these two `createServer` denote real http and proxy server for test environment, right? Where the logic to implement proxying?

[Takashi Komori]

(In reply to Basuke Suzuki from comment #3)
> Comment on attachment 394356 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394356&action=review
> 
> This is an informal review.
> 
> > Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:98
> > +    auto [proxyServer, proxyURL] = createServer("", 59001, "Respond from proxy server");
> 
> I cannot follow the logic. It seems this is the test for proxy server
> setting and these two `createServer` denote real http and proxy server for
> test environment, right? Where the logic to implement proxying?

When a browser connects via proxy it send a request line to the proxy as almost same as direct connection, and gets response from the proxy.
Whether using proxy or not, request and response sequence is almost same (The request line differs).
So just checking responses we can't distinct whether the responses come directly or via proxy.

In this test we use the behavior that a browser always connects to the proxy server when proxy is set.
If proxy setting works, the browser connects to specified proxy server otherwise it connects to specified URL directly.
We can check the response is from proxy or not by only checking response.

In EnableProxyBeforeLoading test, if proxy server is set to http://127.0.0.1:59001 WKPageLoadURL always connects to it.

[Basuke Suzuki]

(In reply to Takashi Komori from comment #4)
> When a browser connects via proxy it send a request line to the proxy as
> almost same as direct connection, and gets response from the proxy.
> Whether using proxy or not, request and response sequence is almost same
> (The request line differs).
> So just checking responses we can't distinct whether the responses come
> directly or via proxy.
> 
> In this test we use the behavior that a browser always connects to the proxy
> server when proxy is set.
> If proxy setting works, the browser connects to specified proxy server
> otherwise it connects to specified URL directly.
> We can check the response is from proxy or not by only checking response.
> 
> In EnableProxyBeforeLoading test, if proxy server is set to
> http://127.0.0.1:59001 WKPageLoadURL always connects to it.

Make sense. Looks good to me.

[Fujii Hironori]

Comment on attachment 394356 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        [Curl] Proxy setting API does not working until Network process starts.

working → work
Remove the period. Let's follow the majority style.
This sentence doesn't make send to me. Do you mean "Proxy setting API does not work if Network process isn't started"?

> Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.cpp:40
> +    auto& endpoint = Inspector::RemoteInspectorSocketEndpoint::singleton();

I don't think it's a good idea to use Inspector::RemoteInspectorSocketEndpoint here.
Why don't you use Tools/TestWebKitAPI/TCPServer.cpp?

> Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.h:69
> +class HTTPServer final : public RemoteInspectorSocketEndpoint::Listener {

Do you know there is already TestWebKitAPI::HTTPServer class in Tools/TestWebKitAPI/cocoa/HTTPServer.mm?

[Fujii Hironori]

Comment on attachment 394356 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.cpp:35
> +HTTPServer::HTTPServer(String&& address, uint16_t port, HTTPRequestHandler::RequestHandler&& requestHandler)

String&& address → const String& address or const char*.

[Fujii Hironori]

Comment on attachment 394356 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:84
> +    auto url = makeString("http://127.0.0.1:", String::number(server->getPort().value()));

If you statically specify "127.0.0.1" here, createServer doesn't need to take 'address' as the argument.

> Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:133
> +    auto [proxyServer, proxyURL] = createServer("", 59001, "Respond from proxy server");

You can specify '0' to use unused port.

[Fujii Hironori]

Comment on attachment 394356 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:132
> +    auto [httpServer, httpURL] = createServer("", 59000, "Respond from HTTP server");

The word "respond" is a verb. I think "Response from HTTP server" or  "Responded from HTTP server" are better than "Respond from HTTP server".

[Fujii Hironori]

Comment on attachment 394356 [details]
Patch

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

>> Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:132
>> +    auto [httpServer, httpURL] = createServer("", 59000, "Respond from HTTP server");
> 
> The word "respond" is a verb. I think "Response from HTTP server" or  "Responded from HTTP server" are better than "Respond from HTTP server".

"Response from HTTP server" or  "HTTP server responded"

[Takashi Komori]

Created attachment 395239 [details]
Patch

[Takashi Komori]

(In reply to Fujii Hironori from comment #6)
> Comment on attachment 394356 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394356&action=review
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        [Curl] Proxy setting API does not working until Network process starts.
> 
> working → work
> Remove the period. Let's follow the majority style.
> This sentence doesn't make send to me. Do you mean "Proxy setting API does
> not work if Network process isn't started"?

Fixed.

> 
> > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.cpp:40
> > +    auto& endpoint = Inspector::RemoteInspectorSocketEndpoint::singleton();
> 
> I don't think it's a good idea to use
> Inspector::RemoteInspectorSocketEndpoint here.
> Why don't you use Tools/TestWebKitAPI/TCPServer.cpp?
> 
> > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.h:69
> > +class HTTPServer final : public RemoteInspectorSocketEndpoint::Listener {
> 
> Do you know there is already TestWebKitAPI::HTTPServer class in
> Tools/TestWebKitAPI/cocoa/HTTPServer.mm?

We chose RemoteInspectorSocketEndpoint because it is working on wincairo.
If we use TCPServer, we have to implement some for wincairo.
RemoteInspectorSocketEndpoint is not generic name, but its implementation seems to be for generic server connection.

[Takashi Komori]

(In reply to Fujii Hironori from comment #7)
> Comment on attachment 394356 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394356&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.cpp:35
> > +HTTPServer::HTTPServer(String&& address, uint16_t port, HTTPRequestHandler::RequestHandler&& requestHandler)
> 
> String&& address → const String& address or const char*.

Changed to const char*

[Takashi Komori]

(In reply to Fujii Hironori from comment #8)
> Comment on attachment 394356 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394356&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:84
> > +    auto url = makeString("http://127.0.0.1:", String::number(server->getPort().value()));
> 
> If you statically specify "127.0.0.1" here, createServer doesn't need to
> take 'address' as the argument.
> 
> > Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:133
> > +    auto [proxyServer, proxyURL] = createServer("", 59001, "Respond from proxy server");
> 
> You can specify '0' to use unused port.

FIXED.

[Takashi Komori]

(In reply to Fujii Hironori from comment #10)
> Comment on attachment 394356 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394356&action=review
> 
> >> Tools/TestWebKitAPI/Tests/WebKit/curl/Proxy.cpp:132
> >> +    auto [httpServer, httpURL] = createServer("", 59000, "Respond from HTTP server");
> > 
> > The word "respond" is a verb. I think "Response from HTTP server" or  "Responded from HTTP server" are better than "Respond from HTTP server".
> 
> "Response from HTTP server" or  "HTTP server responded"

Changed to "Response from HTTP server"

[Fujii Hironori]

Comment on attachment 395239 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +        [Curl] Proxy setting API does not work until Network process starts.

Remove the period. Let's follow the majority style.
This sentence doesn't make send to me. Do you mean "Proxy setting API does not work if Network process isn't started"?

[Fujii Hironori]

(In reply to Takashi Komori from comment #12)
> > 
> > > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.cpp:40
> > > +    auto& endpoint = Inspector::RemoteInspectorSocketEndpoint::singleton();
> > 
> > I don't think it's a good idea to use
> > Inspector::RemoteInspectorSocketEndpoint here.
> > Why don't you use Tools/TestWebKitAPI/TCPServer.cpp?
> > 
> > > Tools/TestWebKitAPI/Tests/WebKit/curl/HTTPServer.h:69
> > > +class HTTPServer final : public RemoteInspectorSocketEndpoint::Listener {
> > 
> > Do you know there is already TestWebKitAPI::HTTPServer class in
> > Tools/TestWebKitAPI/cocoa/HTTPServer.mm?
> 
> We chose RemoteInspectorSocketEndpoint because it is working on wincairo.
> If we use TCPServer, we have to implement some for wincairo.
> RemoteInspectorSocketEndpoint is not generic name, but its implementation
> seems to be for generic server connection.

I think Apple folks ask you to use TCPServer. Could you ask them?

[Takashi Komori]

(In reply to Fujii Hironori from comment #17)
> I think Apple folks ask you to use TCPServer. Could you ask them?

OK I try to use TCPServer on wincairo.

[Fujii Hironori]

I'm also concerning the duplicated TestWebKitAPI::HTTPServer classes.
It'd also be great if you will port HTTPServer.mm into C++, and improve it, and use it.

[Takashi Komori]

Created attachment 397822 [details]
Patch

[Takashi Komori]

We made a http server for tests.
The source has Win suffix but the interfaces resemble cocoa's HTTPServer and we will be able to use this code on other ports commonly.

We added Win suffix because we think it is hard to replace cocoa's HTTPServer at once.
But we have a plan to replace it.

[Fujii Hironori]

Comment on attachment 397822 [details]
Patch

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

> Tools/ChangeLog:47
> +        * TestWebKitAPI/win/HTTPServerWin.cpp: Added.

Bug 215379 is going to land HTTPServer.cpp as a separate patch. Let's redo this patch after Bug 215379.