Bug 223897 – Add ability to set HTTP proxy on a per-WKWebView basis

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

[Brady Eidson]

Add ability to set HTTP proxy on a per-WKWebView basis

[Brady Eidson]

Created attachment 424574 [details]
Patch

[Alex Christensen]

Comment on attachment 424574 [details]
Patch

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

> Source/WebKit/ChangeLog:18
> +        If multiple WKWebViews are created with the same proxy configuration they'll all share the
> +        same proxy-specific NSURLSessions.

Could you add a test that verifies this?

> Source/WebKit/Shared/ProxyConfigurationParameters.cpp:38
> +    IPC::encode(encoder, proxyConfigurationDictionary.get());

Do we care if someone puts something in the NSDictionary that isn't encodable?

> Source/WebKit/Shared/ProxyConfigurationRegistry.h:60
> +    HashMap<ProxyConfigurationIdentifier, ProxyConfiguration*> m_proxyConfigurations;

WeakHashMap?

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.h:213
> + For more information refer to the documentation of NSURLSessionConfiguration

This documentation isn't great.  It would be nice to have a list of supported values, and then sanitize the dictionary.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:616
> +        auto proxyConfiguration = WebKit::ProxyConfigurationRegistry::singleton().getOrCreateProxyConfiguration(_proxyConfiguration.get());

I don't think it's a good idea to have this in the UI process.  Just pass the dictionary along and do the session finding in the network process.
I especially don't think it's a good idea to have this function do more than just call API::PageConfiguration::copy

> Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:99
> +    TCPServer server([] (int socket) mutable {

Would it be possible to use HTTPServer instead?  I'm trying to move away from TCPServer, which uses non-main threads and hangs the whole test if anything goes wrong instead of being well debuggable.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:115
> +        (NSString *)kCFStreamPropertyHTTPProxyPort : [NSNumber numberWithInt:server.port()],

@(server.port()) is nicer syntax.

[Brady Eidson]

(In reply to Alex Christensen from comment #2)
> Comment on attachment 424574 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=424574&action=review
> 
> > Source/WebKit/ChangeLog:18
> > +        If multiple WKWebViews are created with the same proxy configuration they'll all share the
> > +        same proxy-specific NSURLSessions.
> 
> Could you add a test that verifies this?

Not without adding test-specific SPI.

Which I can if you think is worth it.

> 
> > Source/WebKit/Shared/ProxyConfigurationParameters.cpp:38
> > +    IPC::encode(encoder, proxyConfigurationDictionary.get());
> 
> Do we care if someone puts something in the NSDictionary that isn't
> encodable?

What do we do for other dictionaries that fall into that?

> 
> > Source/WebKit/Shared/ProxyConfigurationRegistry.h:60
> > +    HashMap<ProxyConfigurationIdentifier, ProxyConfiguration*> m_proxyConfigurations;
> 
> WeakHashMap?

The lifetime is explicitly managed.

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.h:213
> > + For more information refer to the documentation of NSURLSessionConfiguration
> 
> This documentation isn't great.  It would be nice to have a list of
> supported values, and then sanitize the dictionary.

I'll ping you on slack, but I don't know how we'd go about making that list.

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:616
> > +        auto proxyConfiguration = WebKit::ProxyConfigurationRegistry::singleton().getOrCreateProxyConfiguration(_proxyConfiguration.get());
> 
> I don't think it's a good idea to have this in the UI process.  Just pass
> the dictionary along and do the session finding in the network process.
> I especially don't think it's a good idea to have this function do more than
> just call API::PageConfiguration::copy

At one point, I was convinced it was important to keep this in the UI process for a very specific reason related to sessions outliving web views, or something like that.

But I can't remember why right now - Or, something changed over the life of the patch.

I'll strongly consider this.

> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:99
> > +    TCPServer server([] (int socket) mutable {
> 
> Would it be possible to use HTTPServer instead?  I'm trying to move away
> from TCPServer, which uses non-main threads and hangs the whole test if
> anything goes wrong instead of being well debuggable.

Sure.

> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:115
> > +        (NSString *)kCFStreamPropertyHTTPProxyPort : [NSNumber numberWithInt:server.port()],
> 
> @(server.port()) is nicer syntax.

"Back in my day, we didn't *HAVE* objective-c literals... uphill... both ways..."

[Brady Eidson]

Created attachment 424657 [details]
Patch

[Alex Christensen]

Comment on attachment 424657 [details]
Patch

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

> Source/WebKit/Shared/Cocoa/ProxyOptionsCocoa.mm:32
> +// These constants are copied from <SystemConfiguration/SCSchemaDefinitions.h>
> +static NSString *httpProxyHostKey = @"HTTPProxy";

I think it would be better to use them from the header, kSCPropNetProxiesHTTPProxy instead of @"HTTPProxy" etc.

> Source/WebKit/Shared/Cocoa/ProxyOptionsCocoa.mm:53
> +    auto podPort = [(NSNumber *)port longLongValue];

unsigned long long value?

> Source/WebKit/Shared/Cocoa/ProxyOptionsCocoa.mm:58
> +    portResult = (uint16_t)podPort;

static_cast

> Source/WebKit/Shared/Cocoa/ProxyOptionsCocoa.mm:68
> +NSDictionary *ProxyOptions::toProxyConfigurationDictionary() const

This should probably return nil instead of an allocated empty dictionary if all proxies are unset.

> Source/WebKit/Shared/Cocoa/ProxyOptionsCocoa.mm:70
> +    auto result = [NSMutableDictionary dictionaryWithCapacity:3];

6

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.h:217
> + Key - kSCPropNetProxiesHTTPPort - @"HTTPPort"

I don't think it's good to have the value @"HTTPPort" which should be opaque.  We don't want people to write that value.

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.h:218
> + Value - NSNumber in the range 1-65535

No port zero, huh?

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:183
> +    RetainPtr<NSDictionary> _connectionProxyDictionary;

This is unnecessary and has the side effect that if you set a dictionary with ignored values, the getter will have a copy of those values.  I think we should just call toProxyConfigurationDictionary() in the getter instead.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:101
> +- (void)_proxySessionSetCountForTesting:(void(^)(unsigned))completionHandler;

This needs availability macros.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:96
> +TEST(WebKit, WKWebViewProxyAPI)

Let's test adding invalid things to the dictionary and catching the exceptions.  Like negative port numbers, etc.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/NetworkProcess.mm:126
> +    Util::run(&requested);

This currently mandates that the second web view makes a second connection to the proxy.  This is probably a valid assumption because we never respond.

[Brady Eidson]

Created attachment 424686 [details]
Patch

[Alex Christensen]

Comment on attachment 424686 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1316
> +#ifndef NDEBUG
> +    if (!webPageProxyID) {

This is a bit verbose.  ASSERT(webPageProxyID) would do.

> Source/WebKit/Shared/ProxyConfiguration.cpp:44
> +ProxyConfiguration::ProxyConfiguration()

I think it would be better to pass the identifier and dictionary as constructor parameters than to not initialize m_identifier at all in the constructor, then set it after the one call site of the constructor.

[Brady Eidson]

Nevermind, not quite ready to do this.