Receiving Connection: Proxy-Bypass induces proxy fallback.

net/http/http_network_transaction.cc

Index: net/http/http_network_transaction.cc
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index dd538e27b7e6ba85016deb4b5d98f1280b863a19..810b6b168dd7526f0f899a621ea2752ae8aad445 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -862,6 +862,22 @@ int HttpNetworkTransaction::DoReadHeadersComplete(int result) {
   }
   DCHECK(response_.headers);
 
+  // Server-induced fallback is supported only if this is a PAC configured

[eroman, 2012/09/26 22:01:58]

Can you additionally link to the document you wrote, so that others reading this
will have some context (since proxy-bypass is not a standard thing).

[Michael Piatek, 2012/09/27 00:30:24]

Added a link to the bug, which has additional context and discussion.

+  // proxy.
+  if (response_.was_fetched_via_proxy && proxy_info_.did_use_pac_script()) {
+    if (response_.headers != NULL &&
+        response_.headers->HasHeaderValue("connection", "proxy-bypass")) {
+      ProxyService* proxy_service = session_->proxy_service();
+      // TODO(piatek): const_cast, or non-const accessor?

[eroman, 2012/09/26 01:36:20]

Internalize the mutation of the proxy retry map into a method of ProxyService
(moreover don't mutate proxy_info_).

[Michael Piatek, 2012/09/26 20:13:53]

Done. (Although, I think proxy_info_'s list needs to be modified to remove the
bad proxy, otherwise this seems to loop forever trying the same proxy, even if
added to the service's ProxyRetryInfoMap.)

[eroman, 2012/09/26 22:01:58]

The reason I don't want proxy_info_ to be modified here, is because it
represents the information for the current response. If we modify it here, that
is no longer true for this window of time (until request is restarted), making
it easier to introduce subtle bugs in the future.

My understanding is that when you call ResetConnectionAndRequestForResend() it
will re-create the stream, which will in turn re-fetch a proxy_info_. So I don't
believe the value needs to be removed. Are you seeing something different
happen?
 modified here, then it introduces the possibility for 

[Michael Piatek, 2012/09/27 00:30:24]

Done. (You're right -- I'm not sure what I was doing before, but factoring out a
const helper to only update the bad proxies map seems to work fine. Thanks!)

+      ProxyRetryInfoMap& map =
+          const_cast<ProxyRetryInfoMap&>(proxy_service->proxy_retry_info());
+      if (proxy_info_.proxy_list().Fallback(&map, net_log_)) {
+        ResetConnectionAndRequestForResend();

[eroman, 2012/09/26 01:36:20]

I thought your strategy was going to be to have the proxy return a redirect to
the same URL?

With this approach, you need to consider:
  - Protections against entering an infinite resend-loop
  - What to do with POSTs

It seems the assumption is that the proxy list is terminated by a DIRECT option.
If it isn't, then I suspect this will result in continually resending the
request to the same proxy.

[Michael Piatek, 2012/09/26 20:13:53]

If there's no DIRECT option, Fallback() returns false and the response provided
by the proxy is displayed. (This seems preferable to a self-redirect, since the
proxy can provide an error page for browsers that do not support fallback
without inducing a redirect loop.)

Good point RE: POSTs, it seems clear that we shouldn't retry in that case, since
there's no guarantee about the proxy having taken no action. Anything else you
were thinking of along those lines?

[eroman, 2012/09/26 22:01:58]

Ah good point, that resend loop won't be possible then. Be sure to cover this in
the unit-tests later :)

[Michael Piatek, 2012/09/27 00:30:24]

will do

+        return OK;
+      }
+    }
+  }
+
   // Like Net.HttpResponseCode, but only for MAIN_FRAME loads.
   if (request_->load_flags & LOAD_MAIN_FRAME) {
     const int response_code = response_.headers->response_code();