Bug 185224 – [Curl] Remove unused SystemProxyWin.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Basuke Suzuki]

Curl network layer has a new proxy setting interface since r230973: <https://trac.webkit.org/changeset/230973>/

[Basuke Suzuki]

Created attachment 339360 [details]
FIX

[Per Arne Vollan]

Comment on attachment 339360 [details]
FIX

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

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:68
> +    WebCore::CurlProxySettings settings(WebCore::URL(WebCore::ParsedURLString, String::format("http://%s:%d/", proxy, port)), String());

Is this guaranteed to be a well-formed url?

[Basuke Suzuki]

Comment on attachment 339360 [details]
FIX

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

Thanks for the review.

>> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:68
>> +    WebCore::CurlProxySettings settings(WebCore::URL(WebCore::ParsedURLString, String::format("http://%s:%d/", proxy, port)), String());
> 
> Is this guaranteed to be a well-formed url?

With our internal interface, `proxy` is passed as a host name. From here, url is parsed in the CurlProxySettings class and host name is extracted from the parsed result. I don't think there's security issue hidden here.

[Per Arne Vollan]

Comment on attachment 339360 [details]
FIX

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

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:66
>  void WindowsSystemProxy::setCurlHttpProxy(char* proxy, int port)

I think we could pass the first parameter as a String reference instead of a char pointer. Also, how about renaming it to host or hostname?

[Basuke Suzuki]

Created attachment 339763 [details]
FIX

Add host string normalization. Also rewrote to modernize other static methods in the file to read Windows registry information.

[Basuke Suzuki]

Comment on attachment 339763 [details]
FIX

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

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:43
> +static String normalizedHost(const String& host)

Use only host part of original string even if the string like `xxx:yyy` if passed.

> Source/WebKit/NetworkProcess/win/SystemProxyWin.h:-34
> -    static bool getSystemHttpsProxy(char* buffer, int bufferLen, int* port);

https and ftp version was not defined. Deleted.

> Source/WebKit/NetworkProcess/win/SystemProxyWin.h:34
> +    static std::optional<std::pair<String, unsigned>> getSystemHttpProxy();

Instead of bool + out variable, use optional and pair.

> Source/WebKit/NetworkProcess/win/SystemProxyWin.h:36
> +    static void setCurlHttpProxy(const String& host, unsigned port);

use unsigned to match with WebCore Url's port type.

> Source/WebKit/NetworkProcess/win/SystemProxyWin.h:41
> +    static std::optional<Vector<TCHAR>> readRegistryValue(const TCHAR* path, const TCHAR* valueName);

Extract registry value reading code.

[Per Arne Vollan]

Comment on attachment 339763 [details]
FIX

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

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:78
> +std::optional<std::pair<String, unsigned>> WindowsSystemProxy::parseProxyString(Vector<TCHAR>&& buffer)

Is it possible to use the URL or URLParser class to parse the proxy string?

[Basuke Suzuki]

Comment on attachment 339763 [details]
FIX

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

>> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:78
>> +std::optional<std::pair<String, unsigned>> WindowsSystemProxy::parseProxyString(Vector<TCHAR>&& buffer)
> 
> Is it possible to use the URL or URLParser class to parse the proxy string?

It seems neither URL nor URLParser can handle HOST:PORT string. Also I have to update my code to handle advanced format of the registry. I'll send another patch soon.

[Fujii Hironori]

This is the last patch to enable WK2 for WinCairo. What's the status?

[youenn fablet]

Comment on attachment 339763 [details]
FIX

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

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:50
> +    WebCore::URL url { WebCore::URL(), buffer.toString() };

You can probably write it as return WebCore:URL { WebCore::URL(), makeString("http://", host) };
But maybe it would be simpler to just search for ':' and create a substring if needed?

> Source/WebKit/NetworkProcess/win/SystemProxyWin.cpp:61
> +    buffer.appendNumber(port);

Might be able to use makeString also with StringConcatenateNumbers.h

[Fujii Hironori]

Thank you for the review, youenn.
I had a discussion with Basuke.
We agreed that this bug should be split into two parts, the build fix and the improvement.

At the moment, SystemProxyWin is not used at all even though it is compiled.
So, I will just remove SystemProxyWin.cpp in this bug.

Basuke will work on the improvement part.

[Fujii Hironori]

Created attachment 340680 [details]
Patch

[Fujii Hironori]

Committed r231947: <https://trac.webkit.org/changeset/231947>

[Radar WebKit Bug Importer]

<rdar://problem/40358534>