ChromeFrame's host network stack implementation for IE full tab mode implicitly follows redirects.

When Chrome receives a notification about a redirect it also attempts to follow the redirect request. While
this works in most cases, some sites actually returned an error for the second request initiated by Chrome.

Fix is to abort the request in urlmon, when we receive a notification about a redirect. I also fixed the
IsRedirectResponse function in the UrlRequestAutomationJob class to only treat 301, 302, 303 and 307 as 
redirect codes on the same lines as the default http job.

Test=covered by existing network tests.

I also verified that http://code.google.com/p/chromium/issues/detail?id=25643 works with this CL.

Fixes http://code.google.com/p/chromium/issues/detail?id=28296

Bug=28296


Review URL: http://codereview.chromium.org/402107

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@32896 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 107 insertions, 50 deletions

diff --git a/chrome/browser/automation/url_request_automation_job.cc b/chrome/browser/automation/url_request_automation_job.cc
index f6d4ecd..97522d2 100644
--- a/chrome/browser/automation/url_request_automation_job.cc
+++ b/chrome/browser/automation/url_request_automation_job.cc
@@ -198,20 +198,12 @@ int URLRequestAutomationJob::GetResponseCode() const {
 
 bool URLRequestAutomationJob::IsRedirectResponse(
     GURL* location, int* http_status_code) {
-  static const int kDefaultHttpRedirectResponseCode = 301;
-
-  if (!redirect_url_.empty()) {
-    DLOG_IF(ERROR, redirect_status_ == 0) << "Missing redirect status?";
-    *http_status_code = redirect_status_ ? redirect_status_ :
-                                           kDefaultHttpRedirectResponseCode;
-    *location = GURL(redirect_url_);
-    return true;
-  } else {
-    DCHECK(redirect_status_ == 0)
-        << "Unexpectedly have redirect status but no URL";
-  }
+  if (!net::HttpResponseHeaders::IsRedirectResponseCode(redirect_status_))
+    return false;
 
-  return false;
+  *http_status_code = redirect_status_;
+  *location = GURL(redirect_url_);
+  return true;
 }
 
 bool URLRequestAutomationJob::MayFilterMessage(const IPC::Message& message,
@@ -255,7 +247,7 @@ void URLRequestAutomationJob::OnRequestStarted(int tab, int id,
 
   GURL url_for_cookies =
       GURL(redirect_url_.empty() ? request_->url().spec().c_str() :
-          redirect_url_.c_str());
+           redirect_url_.c_str());
 
   URLRequestContext* ctx = request_->context();
 
diff --git a/chrome_frame/urlmon_url_request.cc b/chrome_frame/urlmon_url_request.cc
index 71aee9b..6124979 100644
--- a/chrome_frame/urlmon_url_request.cc
+++ b/chrome_frame/urlmon_url_request.cc
@@ -27,7 +27,8 @@ UrlmonUrlRequest::UrlmonUrlRequest()
       thread_(PlatformThread::CurrentId()),
       redirect_status_(0),
       parent_window_(NULL),
-      worker_thread_(NULL) {
+      worker_thread_(NULL),
+      ignore_redirect_stop_binding_error_(false) {
   DLOG(INFO) << StringPrintf("Created request. Obj: %X", this)
       << " Count: " << ++instance_count_;
 }
@@ -177,27 +178,32 @@ STDMETHODIMP UrlmonUrlRequest::OnLowResource(DWORD reserved) {
 
 STDMETHODIMP UrlmonUrlRequest::OnProgress(ULONG progress, ULONG max_progress,
     ULONG status_code, LPCWSTR status_text) {
+  static const int kDefaultHttpRedirectCode = 302;
+
   switch (status_code) {
-    case BINDSTATUS_REDIRECTING:
+    case BINDSTATUS_REDIRECTING: {
+      // Fetch the redirect status as they aren't all equal (307 in particular
+      // retains the HTTP request verb).
+      // We assume that valid redirect codes are 301, 302, 303 and 307. If we
+      // receive anything else we would abort the request which would
+      // eventually result in the request getting cancelled in Chrome.
+      int redirect_status = GetHttpResponseStatus();
       DCHECK(status_text != NULL);
       DLOG(INFO) << "URL: " << url() << " redirected to "
                  << status_text;
       redirect_url_ = status_text;
-      // Fetch the redirect status as they aren't all equal (307 in particular
-      // retains the HTTP request verb).
-      redirect_status_ = GetHttpResponseStatus();
-      // NOTE: Even though RFC 2616 says to preserve the request method when
-      // following a 302 redirect, normal browsers don't do that. Instead they
-      // all convert a POST into a GET in response to a 302 and so shall we.
-      // For 307 redirects, browsers preserve the method.  The RFC says to
-      // prompt the user to confirm the generation of a new POST request, but
-      // IE omits this prompt and so shall we.
-      if (redirect_status_ != 307 &&
-          LowerCaseEqualsASCII(method(), "post")) {
-        set_method("get");
-        ClearPostData();
-      }
-      break;
+      redirect_status_ =
+          redirect_status > 0 ? redirect_status : kDefaultHttpRedirectCode;
+      // Chrome should decide whether a redirect has to be followed. To achieve
+      // this we send over a fake response to Chrome and abort the redirect.
+      std::string headers = GetHttpHeaders();
+      OnResponse(0, UTF8ToWide(headers).c_str(), NULL, NULL);
+      ignore_redirect_stop_binding_error_ = true;
+      DCHECK(binding_ != NULL);
+      binding_->Abort();
+      binding_ = NULL;
+      return E_ABORT;
+    }
 
     default:
       DLOG(INFO) << " Obj: " << std::hex << this << " OnProgress(" << url()
@@ -215,6 +221,7 @@ STDMETHODIMP UrlmonUrlRequest::OnStopBinding(HRESULT result, LPCWSTR error) {
   DLOG(INFO) << StringPrintf("URL: %s Obj: %X", url().c_str(), this) <<
       " - Request stopped, Result: " << std::hex << result <<
       " Status: " << status_.status();
+
   if (FAILED(result)) {
     status_.set_status(URLRequestStatus::FAILED);
     status_.set_os_error(HresultToNetError(result));
@@ -607,20 +614,27 @@ HRESULT UrlmonUrlRequest::StartAsyncDownload() {
 
 void UrlmonUrlRequest::EndRequest() {
   DLOG(INFO) << __FUNCTION__;
-  // Special case.  If the last request was a redirect and the current OS
-  // error value is E_ACCESSDENIED, that means an unsafe redirect was attempted.
-  // In that case, correct the OS error value to be the more specific
-  // ERR_UNSAFE_REDIRECT error value.
-  if (!status_.is_success() && status_.os_error() == net::ERR_ACCESS_DENIED) {
-    int status = GetHttpResponseStatus();
-    if (status >= 300 && status < 400) {
-      redirect_status_ = status;  // store the latest redirect status value.
-      status_.set_os_error(net::ERR_UNSAFE_REDIRECT);
+
+  // In case of a redirect notification we prevent urlmon from following the
+  // redirect and rely on Chrome, in which case AutomationMsg_RequestEnd
+  // IPC will be sent over by Chrome to end this request.
+  if (!ignore_redirect_stop_binding_error_) {
+    // Special case.  If the last request was a redirect and the current OS
+    // error value is E_ACCESSDENIED, that means an unsafe redirect was
+    // attempted. In that case, correct the OS error value to be the more
+    // specific ERR_UNSAFE_REDIRECT error value.
+    if (!status_.is_success() && status_.os_error() == net::ERR_ACCESS_DENIED) {
+      int status = GetHttpResponseStatus();
+      if (status >= 300 && status < 400) {
+        redirect_status_ = status;  // store the latest redirect status value.
+        status_.set_os_error(net::ERR_UNSAFE_REDIRECT);
+      }
     }
+    OnResponseEnd(status_);
+  } else {
+    ignore_redirect_stop_binding_error_ = false;
   }
 
-  OnResponseEnd(status_);
-
   // Remove the request mapping and release the outstanding reference to us in
   // the context of the UI thread.
   PostTask(FROM_HERE,
@@ -649,8 +663,10 @@ int UrlmonUrlRequest::GetHttpResponseStatus() const {
   if (SUCCEEDED(info.QueryFrom(binding_))) {
     char status[10] = {0};
     DWORD buf_size = sizeof(status);
+    DWORD flags = 0;
+    DWORD reserved = 0;
     if (SUCCEEDED(info->QueryInfo(HTTP_QUERY_STATUS_CODE, status, &buf_size,
-                                  0, NULL))) {
+                                  &flags, &reserved))) {
       http_status = StringToInt(status);
     } else {
       NOTREACHED() << "Failed to get HTTP status";
@@ -662,6 +678,42 @@ int UrlmonUrlRequest::GetHttpResponseStatus() const {
   return http_status;
 }
 
+std::string UrlmonUrlRequest::GetHttpHeaders() const {
+  if (binding_ == NULL) {
+    DLOG(WARNING) << "GetHttpResponseStatus - no binding_";
+    return std::string();
+  }
+
+  ScopedComPtr<IWinInetHttpInfo> info;
+  if (FAILED(info.QueryFrom(binding_))) {
+    DLOG(WARNING) << "Failed to QI for IWinInetHttpInfo";
+    return std::string();
+  }
+
+  scoped_ptr<char> buffer;
+  DWORD size = 0;
+  DWORD flags = 0;
+  DWORD reserved = 0;
+  HRESULT hr = info->QueryInfo(HTTP_QUERY_RAW_HEADERS_CRLF, NULL, &size,
+                               &flags, &reserved);
+  if (!size) {
+    DLOG(WARNING) << "Failed to query HTTP headers size. Error 0x%x" << hr;
+    return std::string();
+  }
+
+  buffer.reset(new char[size]);
+  memset(buffer.get(), 0, size);
+
+  hr = info->QueryInfo(HTTP_QUERY_RAW_HEADERS_CRLF, buffer.get(),
+                       &size, &flags, &reserved);
+  if (FAILED(hr)) {
+    DLOG(WARNING) << "Failed to query HTTP headers. Error 0x%x" << hr;
+    return std::string();
+  }
+
+  return buffer.get();
+}
+
 //
 // UrlmonUrlRequest::Cache implementation.
 //
diff --git a/chrome_frame/urlmon_url_request.h b/chrome_frame/urlmon_url_request.h
index c95f47d..0612f33 100644
--- a/chrome_frame/urlmon_url_request.h
+++ b/chrome_frame/urlmon_url_request.h
@@ -9,8 +9,8 @@
 #include <atlbase.h>
 #include <atlcom.h>
 #include <atlwin.h>
-
 #include <algorithm>
+#include <string>
 
 #include "base/lock.h"
 #include "base/platform_thread.h"
@@ -239,6 +239,7 @@ END_MSG_MAP()
   // reference to us. It also deletes the request mapping for this instance.
   void EndRequestInternal();
   int GetHttpResponseStatus() const;
+  std::string GetHttpHeaders() const;
 
   static net::Error HresultToNetError(HRESULT hr);
 
@@ -255,6 +256,9 @@ END_MSG_MAP()
   PlatformThreadId thread_;
   static int instance_count_;
   HWND parent_window_;
+  // Set to true if a redirect notification was aborted.
+  bool ignore_redirect_stop_binding_error_;
+
   DISALLOW_COPY_AND_ASSIGN(UrlmonUrlRequest);
 };
 
diff --git a/net/http/http_response_headers.cc b/net/http/http_response_headers.cc
index 9348006..e342d3b 100644
--- a/net/http/http_response_headers.cc
+++ b/net/http/http_response_headers.cc
@@ -724,12 +724,7 @@ bool HttpResponseHeaders::GetCharset(std::string* charset) const {
 }
 
 bool HttpResponseHeaders::IsRedirect(std::string* location) const {
-  // Users probably want to see 300 (multiple choice) pages, so we don't count
-  // them as redirects that need to be followed.
-  if (!(response_code_ == 301 ||
-        response_code_ == 302 ||
-        response_code_ == 303 ||
-        response_code_ == 307))
+  if (!IsRedirectResponseCode(response_code_))
     return false;
 
   // If we lack a Location header, then we can't treat this as a redirect.
@@ -753,6 +748,16 @@ bool HttpResponseHeaders::IsRedirect(std::string* location) const {
   return true;
 }
 
+// static
+bool HttpResponseHeaders::IsRedirectResponseCode(int response_code) {
+  // Users probably want to see 300 (multiple choice) pages, so we don't count
+  // them as redirects that need to be followed.
+  return (response_code == 301 ||
+          response_code == 302 ||
+          response_code == 303 ||
+          response_code == 307);
+}
+
 // From RFC 2616 section 13.2.4:
 //
 // The calculation to determine if a response has expired is quite simple:
diff --git a/net/http/http_response_headers.h b/net/http/http_response_headers.h
index f1512b4..c071389 100644
--- a/net/http/http_response_headers.h
+++ b/net/http/http_response_headers.h
@@ -171,6 +171,10 @@ class HttpResponseHeaders
   // location of the redirect is optionally returned if location is non-null.
   bool IsRedirect(std::string* location) const;
 
+  // Returns true if the HTTP response code passed in corresponds to a
+  // redirect.
+  static bool IsRedirectResponseCode(int response_code);
+
   // Returns true if the response cannot be reused without validation.  The
   // result is relative to the current_time parameter, which is a parameter to
   // support unit testing.  The request_time parameter indicates the time at