Bug 78766 - NPN_GetValueForURL / NPNURLVProxy returns DIRECT when proxy configured via PAC
Summary: NPN_GetValueForURL / NPNURLVProxy returns DIRECT when proxy configured via PAC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Normal
Assignee: Mark Rowe (bdash)
URL:
Keywords: InRadar, Regression
Depends on:
Blocks:
 
Reported: 2012-02-15 17:44 PST by Mark Rowe (bdash)
Modified: 2012-02-16 12:20 PST (History)
2 users (show)

See Also:


Attachments
Patch v1 (7.48 KB, patch)
2012-02-15 17:52 PST, Mark Rowe (bdash)
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2012-02-15 17:44:38 PST
ProxyServerCFNet.cpp has a great big // FIXME: Handle PAC URLs.

<rdar://problem/10729283>
Comment 1 Mark Rowe (bdash) 2012-02-15 17:52:02 PST
Created attachment 127285 [details]
Patch v1
Comment 2 Alexey Proskuryakov 2012-02-15 22:54:07 PST
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.
Comment 3 Mark Rowe (bdash) 2012-02-16 00:29:49 PST
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?
Comment 4 Mark Rowe (bdash) 2012-02-16 00:33:38 PST
> 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.
Comment 5 Alexey Proskuryakov 2012-02-16 09:20:24 PST
> 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().
Comment 6 Mark Rowe (bdash) 2012-02-16 11:46:42 PST
(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.
Comment 7 Mark Rowe (bdash) 2012-02-16 12:20:44 PST
Landed in <http://trac.webkit.org/changeset/107966>.