Closed
Bug 55076
Opened 24 years ago
Closed 24 years ago
Proxy Auto Config (PAC) not honored in nsPluginHostImpl::FindProxyForURL().
Categories
(Core Graveyard :: Plug-ins, defect, P3)
Core Graveyard
Plug-ins
Tracking
(Not tracked)
VERIFIED
FIXED
People
(Reporter: edburns, Assigned: edburns)
References
Details
(Whiteboard: [rtm++])
Attachments
(4 files)
4.82 KB,
patch
|
Details | Diff | Splinter Review | |
5.88 KB,
patch
|
Details | Diff | Splinter Review | |
6.54 KB,
patch
|
Details | Diff | Splinter Review | |
6.72 KB,
patch
|
Details | Diff | Splinter Review |
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.
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.
This bug means plugins won't be able to get past the firewall if the browser is
configured with pac.
Comment 8•24 years ago
|
||
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
Comment 10•24 years ago
|
||
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
Assignee | ||
Comment 11•24 years ago
|
||
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?
Comment 12•24 years ago
|
||
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
Assignee | ||
Comment 13•24 years ago
|
||
Comment 14•24 years ago
|
||
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++]
Keywords: rtm
Whiteboard: [rtm need info]
Comment 15•24 years ago
|
||
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
Assignee | ||
Comment 16•24 years ago
|
||
Comment 17•24 years ago
|
||
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
Whiteboard: [rtm need info] → [rtm+]
Assignee | ||
Comment 19•24 years ago
|
||
Fix checked in to branch and trunk.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•