Bug 78766 – NPN_GetValueForURL / NPNURLVProxy returns DIRECT when proxy configured via PAC

• Component: Plug-ins
• Status: RESOLVED FIXED

[Mark Rowe (bdash)]

ProxyServerCFNet.cpp has a great big // FIXME: Handle PAC URLs.

<rdar://problem/10729283>

[Mark Rowe (bdash)]

Created attachment 127285 [details]
Patch v1

[Alexey Proskuryakov]

Comment on attachment 127285 [details]
Patch v1

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

It's unfortunate that this shares no code with SocketStreamHandleCFNet.cpp.

> Source/WebCore/platform/network/cf/ProxyServerCFNet.cpp:52
> +    if (proxies) {
> +        processProxyServers(*proxyServers, proxies, 0);
>          return;

We prefer early return on error.

> Source/WebCore/platform/network/cf/ProxyServerCFNet.cpp:70
> +                // FIXME: We can't handle nested PAC files. Is this even a possibility?

No.

> Source/WebCore/platform/network/cf/ProxyServerCFNet.cpp:71
> +                continue;

I'd expect a "break" here. Not sure how to prove it though.

> Source/WebCore/platform/network/cf/ProxyServerCFNet.cpp:83
> +            CFTimeInterval timeout = 5.0;

No ".0" per coding style.

> Source/WebCore/platform/network/cf/ProxyServerCFNet.cpp:84
> +            CFRunLoopAddSource(CFRunLoopGetCurrent(), runLoopSource.get(), privateRunLoopMode);

This is a wrong run loop to use on Windows.

[Mark Rowe (bdash)]

Comment on attachment 127285 [details]
Patch v1

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

>> Source/WebCore/platform/network/cf/ProxyServerCFNet.cpp:70
>> +                // FIXME: We can't handle nested PAC files. Is this even a possibility?
> 
> No.

Cool. I'll convert the FIXME to an assertion.

>> Source/WebCore/platform/network/cf/ProxyServerCFNet.cpp:71
>> +                continue;
> 
> I'd expect a "break" here. Not sure how to prove it though.

Breaking here would mean we'd bail out without processing any further entries in the array of proxies. Why is that better than continuing to process further entries?

>> Source/WebCore/platform/network/cf/ProxyServerCFNet.cpp:84
>> +            CFRunLoopAddSource(CFRunLoopGetCurrent(), runLoopSource.get(), privateRunLoopMode);
> 
> This is a wrong run loop to use on Windows.

Can you elaborate on why this would matter when the intent is to synchronously run a nested runloop?

[Mark Rowe (bdash)]

> It's unfortunate that this shares no code with SocketStreamHandleCFNet.cpp.

Now that I'm aware of it, I agree! It shouldn't be too complicated to refactor such that  SocketStreamHandleCFNet can share code with this. That's something to think about in the future though.

[Alexey Proskuryakov]

> Breaking here would mean we'd bail out without processing any further entries in the array of proxies. Why is that better than continuing to process further entries?

PAC is always going to return an answer (I don't know what happens if it's undefined, raises an exception, or is just in wrong form, but I expect that to be equivalent to DIRECT). I don't understand how further entries could ever be of use.

> > This is a wrong run loop to use on Windows.
> 
> Can you elaborate on why this would matter when the intent is to synchronously run a nested runloop?

Actually, you're probably right, I don't see why it shouldn't work for synchronous loading. For async calls, callbacks do not arrive to main run loop because it's not a CFRunLoop, so we use a separate thread running loaderRunLoop().

[Mark Rowe (bdash)]

(In reply to comment #5)
> > Breaking here would mean we'd bail out without processing any further entries in the array of proxies. Why is that better than continuing to process further entries?
> 
> PAC is always going to return an answer (I don't know what happens if it's undefined, raises an exception, or is just in wrong form, but I expect that to be equivalent to DIRECT). I don't understand how further entries could ever be of use.

I'm not sure that it matters either way what we do in this situation since we should never be receiving a kCFProxyTypeAutoConfigurationURL back in the proxy information after evaluating the PAC file. I've updated the patch to ASSERT this, and I'm going to leave the "continue" in only as a means of ensuring that this can't cause a crash in release builds if the assumption is incorrect.

> 
> > > This is a wrong run loop to use on Windows.
> > 
> > Can you elaborate on why this would matter when the intent is to synchronously run a nested runloop?
> 
> Actually, you're probably right, I don't see why it shouldn't work for synchronous loading. For async calls, callbacks do not arrive to main run loop because it's not a CFRunLoop, so we use a separate thread running loaderRunLoop().

That matches my understanding. I'll leave this code using CFRunLoopGetCurrent() for now.

[Mark Rowe (bdash)]

Landed in <http://trac.webkit.org/changeset/107966>.