ChromeFrame's host network stack implementation for IE full tab mode implicit...

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
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=32896

[amit, 2009-11-20 16:20:58]

http://codereview.chromium.org/402107/diff/3002/2005
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/402107/diff/3002/2005#newcode203
Line 203: if (!(redirect_status_ == 301 ||
What happens in case the redirect response is not one of these, it seems that
chrome will want to continue to follow the request but we have aborted the
binding on the other side so it will never get a response?

[ananta, 2009-11-20 18:19:03]

http://codereview.chromium.org/402107/diff/3002/2005
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/402107/diff/3002/2005#newcode203
Line 203: if (!(redirect_status_ == 301 ||
On 2009/11/20 16:20:58, amit wrote:
> What happens in case the redirect response is not one of these, it seems that
> chrome will want to continue to follow the request but we have aborted the
> binding on the other side so it will never get a response?

Actually we can safely have UrlmonUrlRequest end the request anyway in
onstopbinding as chrome would issue a new request for the redirect. CL updated.

[ananta, 2009-11-21 05:06:54]

On 2009/11/20 16:20:58, amit wrote:
> http://codereview.chromium.org/402107/diff/3002/2005
> File chrome/browser/automation/url_request_automation_job.cc (right):
> 
> http://codereview.chromium.org/402107/diff/3002/2005#newcode203
> Line 203: if (!(redirect_status_ == 301 ||
> What happens in case the redirect response is not one of these, it seems that
> chrome will want to continue to follow the request but we have aborted the
> binding on the other side so it will never get a response?

I have updated the CL to match up the way redirects are processed in chrome
frame and chrome. If we receive a redirect code of 0, we default to 302.

[amit, 2009-11-21 06:10:36]

http://codereview.chromium.org/402107/diff/10001/11003
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/402107/diff/10001/11003#newcode203
Line 203: if (!(redirect_status_ == 301 ||
I guess another concern still exists, what if the redirect code is 304? Chrome
will try to continue with this request but we would have aborted it on the other
side.

http://codereview.chromium.org/402107/diff/10001/11001
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/402107/diff/10001/11001#newcode221
Line 221: if (ignore_redirect_stop_binding_error_) {
Instead of using a variable, can we just check for E_ABORT? I am not sure in
what other cases it would return E_ABORT though.

[ananta, 2009-11-23 18:21:16]

http://codereview.chromium.org/402107/diff/10001/11003
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/402107/diff/10001/11003#newcode203
chrome/browser/automation/url_request_automation_job.cc:203: if
(!(redirect_status_ == 301 ||
On 2009/11/21 06:10:38, amit wrote:
> I guess another concern still exists, what if the redirect code is 304? Chrome
> will try to continue with this request but we would have aborted it on the
other
> side.

304 is not a redirect code? I tried a number of runs with SSL sites with fiddler
and while fiddler reported a number of 304 status codes, I never received them
in the bind status callback.

http://codereview.chromium.org/402107/diff/10001/11001
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/402107/diff/10001/11001#newcode221
chrome_frame/urlmon_url_request.cc:221: if (ignore_redirect_stop_binding_error_)
{
On 2009/11/21 06:10:38, amit wrote:
> Instead of using a variable, can we just check for E_ABORT? I am not sure in
> what other cases it would return E_ABORT though.

To ensure that the request object is cleaned up correctly I moved this check to
EndRequestInternal.

[wtc, 2009-11-23 19:07:02]

http://codereview.chromium.org/402107/diff/10001/11003
File chrome/browser/automation/url_request_automation_job.cc (right):

http://codereview.chromium.org/402107/diff/10001/11003#newcode203
chrome/browser/automation/url_request_automation_job.cc:203: if
(!(redirect_status_ == 301 ||
On 2009/11/23 18:21:16, ananta wrote:
>
> 304 is not a redirect code?

304 means "Not Modified".

If you can't call HttpResponseHeaders::IsRedirect()
here, perhaps we should create an IsRedirectStatusCode()
function.

[ananta, 2009-11-23 19:41:03]

On 2009/11/23 19:07:02, wtc wrote:
> http://codereview.chromium.org/402107/diff/10001/11003
> File chrome/browser/automation/url_request_automation_job.cc (right):
> 
> http://codereview.chromium.org/402107/diff/10001/11003#newcode203
> chrome/browser/automation/url_request_automation_job.cc:203: if
> (!(redirect_status_ == 301 ||
> On 2009/11/23 18:21:16, ananta wrote:
> >
> > 304 is not a redirect code?
> 
> 304 means "Not Modified".
> 
> If you can't call HttpResponseHeaders::IsRedirect()
> here, perhaps we should create an IsRedirectStatusCode()
> function.

Done.

[wtc, 2009-11-23 20:09:58]

I only reviewed your changes to net/http/http_response_headers.{h,cc}.
I'd like to suggest a renaming and another minor change.

http://codereview.chromium.org/402107/diff/15003/14005
File net/http/http_response_headers.cc (right):

http://codereview.chromium.org/402107/diff/15003/14005#newcode755
net/http/http_response_headers.cc:755: if (!(status_code == 301 ||
Please just say
  return (response_code == 301 ||
          response_code == 302 ||
          response_code == 303 ||
          response_code == 307);

http://codereview.chromium.org/402107/diff/15003/14006
File net/http/http_response_headers.h (right):

http://codereview.chromium.org/402107/diff/15003/14006#newcode175
net/http/http_response_headers.h:175: static bool IsRedirectStatusCode(int
status_code);
I suggest a minor change.  Could you please rename the
function to IsRedirectResponseCode and the parameter to
response_code, to be consistent with the existing
response_code() method?  Thanks.

[amit, 2009-11-23 21:37:59]

lgtm with wtc's comments addressed.

http://codereview.chromium.org/402107/diff/15003/14002
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/402107/diff/15003/14002#newcode620
chrome_frame/urlmon_url_request.cc:620: // error value is E_ACCESSDENIED, that
means an unsafe redirect was attempted.
80

[ananta, 2009-11-23 21:43:32]

http://codereview.chromium.org/402107/diff/15003/14002
File chrome_frame/urlmon_url_request.cc (right):

http://codereview.chromium.org/402107/diff/15003/14002#newcode620
chrome_frame/urlmon_url_request.cc:620: // error value is E_ACCESSDENIED, that
means an unsafe redirect was attempted.
On 2009/11/23 21:37:59, amit wrote:
> 80
> 

Done.

http://codereview.chromium.org/402107/diff/15003/14005
File net/http/http_response_headers.cc (right):

http://codereview.chromium.org/402107/diff/15003/14005#newcode755
net/http/http_response_headers.cc:755: if (!(status_code == 301 ||
On 2009/11/23 20:09:58, wtc wrote:
> Please just say
>   return (response_code == 301 ||
>           response_code == 302 ||
>           response_code == 303 ||
>           response_code == 307);

Done.

http://codereview.chromium.org/402107/diff/15003/14006
File net/http/http_response_headers.h (right):

http://codereview.chromium.org/402107/diff/15003/14006#newcode175
net/http/http_response_headers.h:175: static bool IsRedirectStatusCode(int
status_code);
On 2009/11/23 20:09:58, wtc wrote:
> I suggest a minor change.  Could you please rename the
> function to IsRedirectResponseCode and the parameter to
> response_code, to be consistent with the existing
> response_code() method?  Thanks.
>  

Done.

[wtc, 2009-11-23 22:37:20]

The changes to net/http/http_response_headers.{h,cc} in
this CL look good to me!