Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(298)

Issue 333043: If a HTTP post to a server returns any redirect code other than 307, then bro... (Closed)

Created:
11 years, 1 month ago by ananta
Modified:
9 years, 6 months ago
CC:
chromium-reviews_googlegroups.com, amit, darin (slow to review)
Visibility:
Public.

Description

If a HTTP post to a server returns any redirect code other than 307, then browsers don't preserve the request method, i.e. they convert the POST request to GET. For 307 redirects browsers preserve redirects. This CL fixes the following issues :- 1. As per the above description, we reset the method which ensures that we don't generate the post related headers. The Post302RedirectGet net test does test this very case. However it works correctly as Chrome follows the redirect and reissues the GET request. In this case this does not occur as the only calls which are invoked on the bind status callback after the redirect are GetBindInfo and BeginningTransaction where we incorrectly return the post related information. Ideally we would want to turn off follow redirects in Urlmon or Chrome. I tried the latter which has a number of issues. 2. In debug mode the chrome_frame_net_tests cause a DCHECK to be fired which indicates that the test is not being run on the UI thread. 3. As the Urlmon requests are now destroyed asynchronously having a DCHECK after the Stop call on the Urlmon request object in the CleanupAsyncRequests function is incorrect. Removed this DCHECK Fixes bug http://code.google.com/p/chromium/issues/detail?id=25643 Bug=25643 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30261

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -9 lines) Patch
M chrome_frame/chrome_frame_automation.cc View 1 chunk +0 lines, -1 line 0 comments Download
M chrome_frame/plugin_url_request.h View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome_frame/test/net/fake_external_tab.h View 1 3 chunks +3 lines, -2 lines 0 comments Download
M chrome_frame/test/net/fake_external_tab.cc View 3 chunks +3 lines, -6 lines 0 comments Download
M chrome_frame/urlmon_url_request.cc View 1 chunk +11 lines, -0 lines 2 comments Download

Messages

Total messages: 5 (0 generated)
ananta
11 years, 1 month ago (2009-10-27 05:28:16 UTC) #1
tommi (sloooow) - chröme
lgtm. nice
11 years, 1 month ago (2009-10-27 17:49:25 UTC) #2
amit
Good catch. http://codereview.chromium.org/333043/diff/4001/4003 File chrome_frame/urlmon_url_request.cc (right): http://codereview.chromium.org/333043/diff/4001/4003#newcode176 Line 176: // IE omits this prompt and ...
11 years, 1 month ago (2009-10-27 17:53:57 UTC) #3
ananta
http://codereview.chromium.org/333043/diff/4001/4003 File chrome_frame/urlmon_url_request.cc (right): http://codereview.chromium.org/333043/diff/4001/4003#newcode176 Line 176: // IE omits this prompt and so shall ...
11 years, 1 month ago (2009-10-27 18:04:34 UTC) #4
amit
11 years, 1 month ago (2009-10-27 19:00:17 UTC) #5
ok, lgtm.
But you now owe a couple of tests :) OK to do them as a separate checkin.

On Tue, Oct 27, 2009 at 11:04 AM, <ananta@chromium.org> wrote:

>
> http://codereview.chromium.org/333043/diff/4001/4003
> File chrome_frame/urlmon_url_request.cc (right):
>
> http://codereview.chromium.org/333043/diff/4001/4003#newcode176
> Line 176: // IE omits this prompt and so shall we.
> On 2009/10/27 17:53:57, amit wrote:
>
>> Does chrome do this as well?
>>
> yes,
>
>
> http://codereview.chromium.org/333043
>

Powered by Google App Engine
This is Rietveld 408576698