User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007 Firebird/0.7 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5) Gecko/20031007 Firebird/0.7 According to the auto-proxy configuration documentation, the browser is supposed to attempt to use the "primary" proxy server, and if not reachable, use the secondary and not retry for 30 minutes, and if still a problem at the 30 minute mark, add another 30 minutes, etc. Firebird, in fact, on every (almost) http get, attempts to establish a TCP session with the primary proxy server for 20 seconds, and then connects to the backup proxy server. On the next go around, it tries the primary again. Reproducible: Always Steps to Reproduce: 1. Create an auto-proxy script returning the following a bogus IP and port as primary with real as secondary: return "PROXY 1.1.1.1:8080; PROXY 121.12.14.34:80" 2. Run an analyzer capturing on the two IPs listed in step 1 3. Try to get a web page from the internet Actual Results: A tremendous amount of time is spent by the browser trying to contact the bogus proxy server. Some pages will time out. Expected Results: Follow the 30 minute rule that is listed in the documentation at: http://wp.netscape.com/eng/mozilla/2.0/relnotes/demo/proxy-live.html I tried this same test with IE 6 and Mozilla 1.5. With Mozilla 1.5 it also acts the same as Firebird except that the requests to the bogus proxy server happen faster and then it fails over (from 5-8 seconds). It continues to try the primary proxy server on every new http request With IE, the browser trys for 20 seconds on the first attempt and then does not retry to down server for 30 minutes. I did not wait the hour to see if it follows true to form. I did not try any version of netscape.

-> Networking

known problem.. i'll fix it for 1.6 beta.

this could improve pac proxy failover's performance.

bug 207633 also has the same issue, but it has more: " If all proxies are down, and there was no DIRECT option specified, the Navigator will ask if proxies should be temporarily ignored, and direct connections attempted. The Navigator will ask if proxies should be retried after 20 minutes has passed (then the next time " 40 minutes from the previous question, always adding 20 minutes).

only do pac failover darin, can you take a look?

Comment on attachment 134904 [details] [diff] [review] patch 0.1 adding to my review queue...

more completed patch

add prompt.

sorry for delaying in reviewing your patch jerry. i'm afraid it isn't going to make mozilla 1.7. however, i will see to it that it makes it into 1.8. my apologies for not getting to your patch in a timely manner.

Ok, I reviewed the Jerry's 0.3 and had quite a few changes I wanted to make, so I just revised the patch directly. Here's the revised patch. I tried to make the behavior of Moz match that of IE6 as much as possible. In other words, I do not bother with failover to DIRECT, so there is no prompting code in this patch. I'm content to leave it up to site admins to control whether or not failover to DIRECT occurs. Users can always manually enable DIRECT connections if they need to do so. I also do not implement Navigator's doubling algorithm. I just use a pref controlled timeout value each time a proxy fails. If all proxies fail, then I make ExamineForProxy return all proxies. That way, if there is a major network failure the user will be able to press reload to retry and not be stuck with either 1) Moz stuck thinking it can only do direct or 2) Moz stuck thinking it cannot do anything. This behavior seems to be consistent with IE6 (minus the doubling issue... I haven't checked how IE behaves after 30 minutes expire).

Comment on attachment 151972 [details] [diff] [review] v1 patch grrr. retyping from memory, after closing the wrong window. nsIProtocolProxyService.idl + * This method returns a proxy to be used for loading the given URL. does it return null or throw an exception if no proxy is to be used? please document :) + * This method may be called to construct a nsIProxyInfo instance from + * the given parameters. is there a need for this to be a public function? NS_NewProxyInfo certainly has no caller (http://lxr.mozilla.org/mozilla/search?string=NS_NewProxyI) and nsIProxyInfo _is_ supposedly meant to be an opaque cookie. (hmm, dwitte would like it ;) ) if you are going to keep it: maybe list the valid aType parameters it would make sense to make aPort an "unsigned short", since that's the valid range for ports + void configureFromPAC(in AUTF8String aURI); does this load synchronously or asynchronously? maybe the parameter should be nsIURI? + * instead. As a side-effect, this method may affect future return values + * from examineForProxy. ...as well as from getFailoverProxy. Index: netwerk/base/src/nsProtocolProxyService.cpp + char *mHost; // owning reference maybe this should be an nsCString, saves a string copy now that we have string sharing + // Add timeout to interval + dsec += mFailedProxyTimeout; this seems to be the value when the proxy can be tried again... maybe add a comment, saying that + mFailedProxies.Put(key, dsec); this can fail (OOM)... but I guess that's ok-ish, it'd just mean that this proxy is not really disabled... ExaminePACForProxy: + do { well... ok... this "loop" isn't, really. maybe its content should be in its own function, that calls itself in the + else if (!first && pruneDisabledProxies) { case. that'd be more clear, at least... url. loading it now, in the current thread results in a browser crash */ this comment is now misleading/wrong, since it is now explicit that the call happens on the current thread, the point is just that it's async. please change its wording. + // Verify that |aProxy| is one of our nsProxyInfo objects. + nsProxyInfo *pi = nsnull; + aProxy->QueryInterface(kProxyInfoID, (void **) &pi); + if (!pi) + return NS_ERROR_INVALID_ARG; + NS_RELEASE(aProxy); this release of an argument seems to be really a release of |pi|, so that you don't have to do it in the error cases. OK, but maybe you should use nsRefPtr<nsProxyInfo for pi, then you could also use getter_AddRefs, I think nsProtocolProxyService::LoadFilters(const char *filters) { + mFailedProxies.Clear(); I don't think that a change of the "no proxy for" pref should clear the list of failed proxies. that pref does not cause them to magically be up again :) Index: netwerk/base/src/nsProtocolProxyService.h +typedef nsDataHashtable<nsCStringHashKey,PRUint32> nsFailedProxyTable; looks like a space is missing after the comma + PRTime mSessionStart; maybe it would be better to not use seconds since session start, but instead convert that pref to usec when it's read. it'd do the conversion only once, not everytime you call SecondsSinceSessionStart...

ah, one more thing... shouldn't SOCKS failover as well?

(In reply to comment #12) > ah, one more thing... shouldn't SOCKS failover as well? It does, but only for HTTP(S) at the moment. If you mean, shouldn't the Socket Transport implement SOCKS failover, then... yes, I think I agree that it should. I'd rather take care of that problem in a separate bug.

> + * This method may be called to construct a nsIProxyInfo instance from > + * the given parameters. > > is there a need for this to be a public function? > NS_NewProxyInfo certainly has no caller > (http://lxr.mozilla.org/mozilla/search?string=NS_NewProxyI) and nsIProxyInfo > _is_ supposedly meant to be an opaque cookie. (hmm, dwitte would like it ;) ) Agreed, and I considered removing it until I realized that folks might want to be able to create a SOCKS connection and would then need to have a way of providing the nsISocketTransportService with a nsIProxyInfo instance. In Mozilla, we never need this functionality because we always start with a URI, so ExamineForProxy is sufficient. > if you are going to keep it: > maybe list the valid aType parameters done > it would make sense to make aPort an "unsigned short", since that's the valid > range for ports Nope, Necko defines ports as 32-bit signed integers. Keep in mind that it might be possible to use (or abuse) a port number to mean something other than what TCP defines it to mean. > + void configureFromPAC(in AUTF8String aURI); > > does this load synchronously or asynchronously? asynchronously. > maybe the parameter should be nsIURI? well, I considered that, but then I'd have to go change the callers. I'd rather not do that. in actual fact, i'm not sure that it makes any sense to expose this method. since we observe the PAC file pref, it should be enough for folks to just modify that pref directly. > + PRTime mSessionStart; > > maybe it would be better to not use seconds since session start, but instead > convert that pref to usec when it's read. it'd do the conversion only once, not > everytime you call SecondsSinceSessionStart... I decided against this because I did not want to store 64-bit integers in the hash table. I don't think SecondsSinceSessionStart will have a measurable perf cost. I applied all of your other suggestions and added a bit more comments.

revised patch. this also includes a bug fix. i found that if i toggled my squid proxy off and then back on again, that mozilla would still think that squid is disabled. that was caused by the fact that we never notify the PPS when a proxy works, we only inform it when a proxy fails. my solution is to enable any proxy that i return from ExamineForProxy. it seems to work well.

fixed-on-trunk

I also rev'd the IID for nsIProxyInfo ;-)

this caused bug 253190

*** Bug 207633 has been marked as a duplicate of this bug. ***

*** Bug 261765 has been marked as a duplicate of this bug. ***

Reporter, please set this fix to be blocking-aviary1.0, because it is core functionality that must work to use Firefox and Thunderbird in corporate environments. Must the bug report be reopened as well?

> Must the bug report be reopened as well? do NOT reopen the bug. also, you can set blocking-aviary? yourself.

Please do not set the blocking-aviary1.0+ flag unless you are a member of the Aviary or drivers@mozilla.org teams. Thanks.

If we consider this for aviary and 1.7, we should also look at the patch for bug 253190 which was a crash that seems to have been caused here.

It would be really useful if we can find out whether or not any of our current or prospective enterprise customers need this.

*** Bug 261764 has been marked as a duplicate of this bug. ***