Bug 53080 makes it so PAC works. However, PAC has nothing to do with Prefs. Perhaps this is a separate bug. In any case, we need to make it so nsPluginHostImpl::FindProxyForURL() honors the settings in PAC.

ACCEPT Adding CC.

depends on bug 53080.

Once I get approval to check in, here's the checkin message: This bug fix makes nsPluginHostImpl::FindProxyForURL() inspect the ProxyAutoConfig service for proxy values before trying to inspect the prefs. This bug fix makes the following assumptions: // ASSUMPTION: the only time where it's appropriate to NOT look at // the prefs is a successful return from PAC. That is, prefs will be consulted unless PAC returns successfiully returns a valid value. files in this fix: modules/plugin/nglsrc/nsPluginHostImpl.cpp

I looked at the changes and found them reasonable. However, AFAIR in current world to get anything in we should escalate the status of the bug to rtm+ _and_ get review from the team of reviewers.

I just got email with r=av.

This bug means plugins won't be able to get past the firewall if the browser is configured with pac.

Ed, if you're willing to do it now rather than later (later tends to mean never), please fix the code to use nsCOMPtr and nsXPIDLCString throughout (nsCOMPtr for uriIn, and nsXPIDLCString for protocol and type -- note that host can be combined with the existing proxyHost nsXPIDLCString). Also, is PL_strstr the right test? What if one hostname in noProxyList or similar is a superdomain of the host name we're testing, or otherwise contains it as a substring? /be

Looks good, but: - Why is (nsnull == url) tested twice, in the first and second if statements in the function? It should be tested very early, and NS_OK explicitly returned, I think -- unless NS_ERROR_INVALID_ARG or some such is better. BTW, (nsnull == url) is considered bad style in the Mozilla circles I travel in, these days -- both for the overparenthesization against conjoining and disjoining && and || ops, and because of the silly, MS-influenced nsnull == on the left -- !url is shorter and better, but url == nsnull is ok too if you must (the fear that you'd drop an = and assign to url unintentionally is silly, esp. in light of the warning you'd get from gcc in that case -- provided you removed the bogus extra parentheses, of course!). - Don't initialize nsCOMPtrs to nsnull -- it's unnecessary and incongruous next to uninitialized nsXPIDLCString variables. - What about my PL_strstr ambiguity point? Fix the extra url test, yank those = nsnull initializers, tell me why PL_strstr is the right way to test membership in a comma-separated list, and then I will give a fast a=brendan@mozilla.org. /be

Tried to avoid putting you on CC. I see what you're saying: noProxyList is: yousuck.com, javaweb.eng suck.com the host we're testing. This would say no proxy for suck.com. I don't think this is important because generally noProxyList list is for things inside your firewall. However, the safer thing to do would be to tokenize the noProxyList and compare it to each one. Brendan, is this necessary?

Ed: no need to cc: me once I've commented on a bug, I've configured my bugzilla prefs so I get notified about updates to any bug I've commented on. I don't see why it's so hard to test for membership in a comma-separated list: how about: if (NS_SUCCEEDED(res)) { PRUint32 proxyHostLen = PL_strlen(proxyHost); if (proxyHostLen != 0) { const char *substr = PL_strstr(noProxyList, proxyHost); if (substr && (substr == (const char *) noProxyList || substr[-1] == ' ' || substr[-1] == ',') && (substr[proxyHostLen] == '\0' || substr[proxyHostLen] == ' ' || substr[proxyHostLen] == ',')) { useDirect = PR_TRUE; } } } Make a subroutine if you need this elsewhere? /be

PDT marking [rtm need info]. When you guys have worked out the code review, please change to [rtm+] in order to get PDT evaluation for [rtm++]

Almost there (I promise): - nsresult res is uselessly initialized. - Need to check for malloc failure, and return NS_ERROR_OUT_OF_MEMORY, after calling Copy and getting null back, in: + char *noProxyCopy = nsXPIDLCString::Copy(noProxyList); + + token = nsCRT::strtok( noProxyCopy, comma, &newStr ); Thanks, good catch on that strtok (my PL_strstr code needed to loop, and looks to be slower than using nsCRT::strtok). /be

Ed, a=brendan@mozilla.org. Presumably av still gives r=. Thanks for fixing this to death. I'm restoring [rtm+]. Let's get this into the trunk. /be

PDT marking [rtm++]

Fix checked in to branch and trunk.

marking verified. The patch is in..