Allow for an existing bad proxy to be given a new retry delay.

net/proxy/proxy_list.cc

Index: net/proxy/proxy_list.cc
diff --git a/net/proxy/proxy_list.cc b/net/proxy/proxy_list.cc
index 5574f0162cfdae95140605e73993a2d0ed2735ae..7d53b4cfb7ebd485eaada9d0096bfe012677bec1 100644
--- a/net/proxy/proxy_list.cc
+++ b/net/proxy/proxy_list.cc
@@ -187,16 +187,13 @@ void ProxyList::AddProxyToRetryList(ProxyRetryInfoMap* proxy_retry_info,
                                     int net_error,
                                     const BoundNetLog& net_log) const {
   // Mark this proxy as bad.
+  TimeTicks bad_until = TimeTicks::Now() + retry_delay;
   std::string proxy_key = proxy_to_retry.ToURI();
   ProxyRetryInfoMap::iterator iter = proxy_retry_info->find(proxy_key);
-  if (iter != proxy_retry_info->end()) {
-    // TODO(nsylvain): This is not the first time we get this. We should
-    // double the retry time. Bug 997660.

[mmenke, 2015/06/18 22:25:06]

Wow...This is a bug ID from before Chrome was open sourced.  Been around a
while.  :)

-    iter->second.bad_until = TimeTicks::Now() + iter->second.current_delay;
-  } else {
+  if (iter == proxy_retry_info->end() || bad_until > iter->second.bad_until) {
     ProxyRetryInfo retry_info;
     retry_info.current_delay = retry_delay;
-    retry_info.bad_until = TimeTicks().Now() + retry_info.current_delay;
+    retry_info.bad_until = bad_until;
     retry_info.try_while_bad = try_while_bad;

[mmenke, 2015/06/18 22:25:06]

Seems like changing this could have unexpected side effects.

We have two fundamentally different types of proxies marked as bad, and not sure
we should just decide between the two based on which ones is bad until the date
furthest in the future.

[sclittle, 2015/06/19 01:45:10]

I see your point. Although I do think that this logic makes more sense than the
old logic, it could potentially cause a proxy to be retried that was marked as
try_while_bad=false before that retry delay expires, if that entry was
overwritten by a try_while_bad=true entry.

One way to solve this would be to make the proxy retry list into a multimap that
can support multiple entries for a proxy, and then just have the proxy
resolution logic obey the most restrictive non-expired retry info entry. That
would require quite a bit of changes throughout this codebase, and be a bit more
complicated though.

Another, simpler option would be to leave try_while_bad alone, and only update
current_delay, bad_until, etc., which would closely match the current
implementation. However, it would seem strange if marking an existing bad proxy
as bad would not respect the new try_while_bad value.

WDYT?

[mmenke, 2015/06/19 15:15:56]

Or could overwrite the old try_while_bad value if the old one was true (Or,
equivalently, if the new one is false).

Or could use the current logic if the try_while_bad values are the same, but if
they're different, prefer the one that's false.

I'm sufficiently concerned about this that I'd really rather have someone who
better understood the proxy retry logic weighed in.  Mind waiting for eroman to
get back?  He's back next week, I believe.  Don't think we have any other proxy
experts on the net/ team, unfortunately.

[sclittle, 2015/06/19 19:55:36]

Sure, let's wait for eroman then. I've added him as a reviewer.

     retry_info.net_error = net_error;
     (*proxy_retry_info)[proxy_key] = retry_info;