RESOLVED FIXED 78766
NPN_GetValueForURL / NPNURLVProxy returns DIRECT when proxy configured via PAC
https://bugs.webkit.org/show_bug.cgi?id=78766
Summary NPN_GetValueForURL / NPNURLVProxy returns DIRECT when proxy configured via PAC
Mark Rowe (bdash)
Reported 2012-02-15 17:44:38 PST
ProxyServerCFNet.cpp has a great big // FIXME: Handle PAC URLs. <rdar://problem/10729283>
Attachments
Patch v1 (7.48 KB, patch)
2012-02-15 17:52 PST, Mark Rowe (bdash)
andersca: review+
Mark Rowe (bdash)
Comment 1 2012-02-15 17:52:02 PST
Created attachment 127285 [details] Patch v1
Alexey Proskuryakov
Comment 2 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.
Mark Rowe (bdash)
Comment 3 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?
Mark Rowe (bdash)
Comment 4 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.
Alexey Proskuryakov
Comment 5 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().
Mark Rowe (bdash)
Comment 6 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.
Mark Rowe (bdash)
Comment 7 2012-02-16 12:20:44 PST
Note You need to log in before you can comment on or make changes to this bug.