Preserve non-POST methods on 301/302 requests.

Preserve non-POST methods on 301/302 requests.

BUG=56373
TEST=URLRequestTestHTTP.Redirect30*Tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107261

[mmenke, 2011-10-25 13:54:17]

I've run a successful tryjob, but ran it when results weren't being posted to
codereview.

[willchan no longer on Chromium, 2011-10-25 15:11:36]

LGTM, but I asked some of the httpbis people who requested the change to take a
glance, so please wait until the end of the day or tomorrow to commit this.

http://codereview.chromium.org/8375002/diff/16001/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/8375002/diff/16001/net/url_request/url_request...
net/url_request/url_request_unittest.cc:297: LOG(WARNING) << "Request method: "
<< request_method;
Is this necessary?

http://codereview.chromium.org/8375002/diff/16001/net/url_request/url_request...
net/url_request/url_request_unittest.cc:298: const char kData[] = "hello world";
static

[mmenke, 2011-10-25 15:36:06]

http://codereview.chromium.org/8375002/diff/16001/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/8375002/diff/16001/net/url_request/url_request...
net/url_request/url_request_unittest.cc:297: LOG(WARNING) << "Request method: "
<< request_method;
On 2011/10/25 15:11:36, willchan wrote:
> Is this necessary?

I don't mind removing it, but if any check fails other than the
"EXPECT_EQ(redirect_method, req.method());", we won't be able to tell if the
failure was with a POST or PUT from trybot output.

I could have just added 8 tests instead of doing two redirects per test, but
figured this way was simpler, since I don't have to worry about duplicating URL
strings, or adding them as constants to the top of the file.

http://codereview.chromium.org/8375002/diff/16001/net/url_request/url_request...
net/url_request/url_request_unittest.cc:298: const char kData[] = "hello world";
On 2011/10/25 15:11:36, willchan wrote:
> static

Done.  Not sure why this file doesn't just use const char* everywhere instead.

[willchan no longer on Chromium, 2011-10-25 15:38:32]

Heard back from one of the httpbis guys. No need to wait for them.

http://codereview.chromium.org/8375002/diff/12001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/8375002/diff/12001/net/url_request/url_request...
net/url_request/url_request.cc:705: bool rewrite_headers = true;
Julian Reschke sent me the following comments:
=====
- change "rewrite_headers" to "rewrite_method", and

- reverse the logic, so start with rewrite=false, and then switch to true for

 (status == 303 || was_post && (status == 301 || status == 302))
=====

I definitely agree with the first. The second I leave to your discretion, since
it's very much a matter of taste.

[willchan no longer on Chromium, 2011-10-25 15:49:13]

http://codereview.chromium.org/8375002/diff/16001/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/8375002/diff/16001/net/url_request/url_request...
net/url_request/url_request_unittest.cc:297: LOG(WARNING) << "Request method: "
<< request_method;
On 2011/10/25 15:36:06, Matt Menke wrote:
> On 2011/10/25 15:11:36, willchan wrote:
> > Is this necessary?
> 
> I don't mind removing it, but if any check fails other than the
> "EXPECT_EQ(redirect_method, req.method());", we won't be able to tell if the
> failure was with a POST or PUT from trybot output.
> 
> I could have just added 8 tests instead of doing two redirects per test, but
> figured this way was simpler, since I don't have to worry about duplicating
URL
> strings, or adding them as constants to the top of the file.

How about only printing it out if there's a test failure? See
http://code.google.com/p/googletest/source/browse/trunk/include/gtest/gtest.h....

http://codereview.chromium.org/8375002/diff/16001/net/url_request/url_request...
net/url_request/url_request_unittest.cc:298: const char kData[] = "hello world";
On 2011/10/25 15:36:06, Matt Menke wrote:
> On 2011/10/25 15:11:36, willchan wrote:
> > static
> 
> Done.  Not sure why this file doesn't just use const char* everywhere instead.

Would need to be const char* const if we wanted to make the pointer constant
which is necessary for certain compiler optimizations. And often times we don't
need the pointer. That said, it doesn't matter much for test code.

[mmenke, 2011-10-25 16:09:42]

http://codereview.chromium.org/8375002/diff/12001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/8375002/diff/12001/net/url_request/url_request...
net/url_request/url_request.cc:705: bool rewrite_headers = true;
On 2011/10/25 15:38:32, willchan wrote:
> Julian Reschke sent me the following comments:
> =====
> - change "rewrite_headers" to "rewrite_method", and
> 
> - reverse the logic, so start with rewrite=false, and then switch to true for
> 
>  (status == 303 || was_post && (status == 301 || status == 302))
> =====
> 
> I definitely agree with the first. The second I leave to your discretion,
since
> it's very much a matter of taste.

I went ahead and did the second, which also allowed me to remove the
rewrite_method variable.

http://codereview.chromium.org/8375002/diff/16001/net/url_request/url_request...
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/8375002/diff/16001/net/url_request/url_request...
net/url_request/url_request_unittest.cc:297: LOG(WARNING) << "Request method: "
<< request_method;
On 2011/10/25 15:49:13, willchan wrote:
> On 2011/10/25 15:36:06, Matt Menke wrote:
> > On 2011/10/25 15:11:36, willchan wrote:
> > > Is this necessary?
> > 
> > I don't mind removing it, but if any check fails other than the
> > "EXPECT_EQ(redirect_method, req.method());", we won't be able to tell if the
> > failure was with a POST or PUT from trybot output.
> > 
> > I could have just added 8 tests instead of doing two redirects per test, but
> > figured this way was simpler, since I don't have to worry about duplicating
> URL
> > strings, or adding them as constants to the top of the file.
> 
> How about only printing it out if there's a test failure? See
>
http://code.google.com/p/googletest/source/browse/trunk/include/gtest/gtest.h....

Done.

[commit-bot: I haz the power, 2011-10-25 17:27:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/8375002/24001

[wtc, 2011-10-25 18:04:48]

Review comments on Patch Set 5:

I only reviewed url_request.cc.  The comments need work.
Please see below.

http://codereview.chromium.org/8375002/diff/24001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/8375002/diff/24001/net/url_request/url_request...
net/url_request/url_request.cc:702: // user to confirm the generation of a new
requests, other than GET and HEAD

Nit: "a new requests" has a grammatical error.  I guess you
meant to remove "a".

BUT, this comment on 307 redirects is confusing because the
http_status_code != 307 test is gone in the new code.  This
comment should be updated to match the new code.  For example,
the special case for 303, where all methods are converted
to GET, should be explained.

We should reference httpbis if we're implementing the new
httpbis rules here.

[julian.reschke, 2011-10-25 18:23:00]

On 2011/10/25 18:04:48, wtc wrote:
> ...
> We should reference httpbis if we're implementing the new
> httpbis rules here.
> ...

Note that the only "new" thing is that HTTPbis allows to rewrite POST to GET;
RFC 2616 required to never rewrite.

The actual change will be in draft-ietf-httpbis-p2-semantics-17, which we plan
to submit next Monday.

[mmenke, 2011-10-25 18:25:47]

On 2011/10/25 18:23:00, julian.reschke wrote:
> On 2011/10/25 18:04:48, wtc wrote:
> > ...
> > We should reference httpbis if we're implementing the new
> > httpbis rules here.
> > ...
> 
> Note that the only "new" thing is that HTTPbis allows to rewrite POST to GET;
> RFC 2616 required to never rewrite.
> 
> The actual change will be in draft-ietf-httpbis-p2-semantics-17, which we plan
> to submit next Monday.

I'm a bit reluctant to refer to a draft that's not yet publicly available.

[cbentzel, 2011-10-25 18:28:16]

It's done in other locations:
http://codesearch.google.com/codesearch#search&q=draft-ietf&exact_package=chr...


On Tue, Oct 25, 2011 at 2:25 PM, <mmenke@chromium.org> wrote:

> On 2011/10/25 18:23:00, julian.reschke wrote:
>
>> On 2011/10/25 18:04:48, wtc wrote:
>> > ...
>> > We should reference httpbis if we're implementing the new
>> > httpbis rules here.
>> > ...
>>
>
>  Note that the only "new" thing is that HTTPbis allows to rewrite POST to
>> GET;
>> RFC 2616 required to never rewrite.
>>
>
>  The actual change will be in draft-ietf-httpbis-p2-**semantics-17, which
>> we plan
>> to submit next Monday.
>>
>
> I'm a bit reluctant to refer to a draft that's not yet publicly available.
>
>
http://codereview.chromium.**org/8375002/<http://codereview.chromium.org/8375...
>

[julian.reschke, 2011-10-25 18:29:39]

On 2011/10/25 18:25:47, Matt Menke wrote:
> ...
> I'm a bit reluctant to refer to a draft that's not yet publicly available.

Understood; how about saying "IETF Internet Draft
draft-ietf-httpbis-p2-semantics-17, Section 7.3" (it's not going to move), and
hold the commit until it's out? (will ping you).

[mmenke, 2011-10-25 18:31:29]

Chris:  Those are drafts that *are* publicly available.  Hence the urls.  It
looks to me like draft version 16, released in August, doesn't have the change,
and version 17 isn't yet available anywhere.

Julian:  Sounds good to me.  No real urgency in landing this.

On 2011/10/25 18:29:39, julian.reschke wrote:
> On 2011/10/25 18:25:47, Matt Menke wrote:
> > ...
> > I'm a bit reluctant to refer to a draft that's not yet publicly available.
> 
> Understood; how about saying "IETF Internet Draft
> draft-ietf-httpbis-p2-semantics-17, Section 7.3" (it's not going to move), and
> hold the commit until it's out? (will ping you).

[wtc, 2011-10-25 18:33:39]

http://codereview.chromium.org/8375002/diff/24001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/8375002/diff/24001/net/url_request/url_request...
net/url_request/url_request.cc:702: // user to confirm the generation of a new
requests, other than GET and HEAD

mmenke: it is less important to reference httpbis.

It is more important to make the comments describe the new
code.  The new code tests for 301, 302, and 303.  So the
comments should describe why we handle those three status
codes this way.  We probably can remove the old comment
about 307.

[julian.reschke, 2011-10-25 18:41:39]

On 2011/10/25 18:31:29, Matt Menke wrote:
> Chris:  Those are drafts that *are* publicly available.  Hence the urls.  It
> looks to me like draft version 16, released in August, doesn't have the
change,
> and version 17 isn't yet available anywhere.
> ...

Clarifying: 17 is what we "tag" next Monday; you can see our "trunk" at
<http://greenbytes.de/tech/webdav/draft-ietf-httpbis-p2-semantics-latest.html#...>
and
<http://trac.tools.ietf.org/wg/httpbis/trac/browser/draft-ietf-httpbis/latest/...>
(so it's "public", but not "published as Internet Draft", and what you want if
you want to cite is a stable reference).

[mmenke, 2011-10-25 18:54:00]

Thanks, Julian

http://codereview.chromium.org/8375002/diff/24001/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/8375002/diff/24001/net/url_request/url_request...
net/url_request/url_request.cc:702: // user to confirm the generation of a new
requests, other than GET and HEAD
On 2011/10/25 18:33:39, wtc wrote:
> 
> mmenke: it is less important to reference httpbis.
> 
> It is more important to make the comments describe the new
> code.  The new code tests for 301, 302, and 303.  So the
> comments should describe why we handle those three status
> codes this way.  We probably can remove the old comment
> about 307.

How's this:

  // For 303 redirects, all request methods are converted to GETs, as per RFC
  // 2616.  The latest httpbis draft also allows POST requests to be converted
  // to GETs when following 301/302 redirects for historical reasons.  All
  // normal browsers do this and so shall we.  The RFC says to prompt the user
  // to confirm the generation of new requests, other than GET and HEAD
  // requests, but IE omits these prompts and so shall we.
  // See: 
http://greenbytes.de/tech/webdav/draft-ietf-httpbis-p2-semantics-latest.html#...

[julian.reschke, 2011-10-25 19:01:45]

On 2011/10/25 18:54:00, Matt Menke wrote:
> How's this:
> 
>   // For 303 redirects, all request methods are converted to GETs, as per RFC
>   // 2616.  The latest httpbis draft also allows POST requests to be converted

Actually, HEAD shouldn't be rewritten ;-) But maybe we can treat this as a
separate issue (it's another thing we clarify in -17, see
<http://trac.tools.ietf.org/wg/httpbis/trac/ticket/310>. (I don't think the
difference will be observable from XMLHTTPRequest, but doing a GET when the
client only needed HEAD is a waste of bits on the wire...)

>   // to GETs when following 301/302 redirects for historical reasons.  All
>   // normal browsers do this and so shall we.  The RFC says to prompt the user
>   // to confirm the generation of new requests, other than GET and HEAD
>   // requests, but IE omits these prompts and so shall we.
>   // See: 
>
http://greenbytes.de/tech/webdav/draft-ietf-httpbis-p2-semantics-latest.html#...

Sounds ok. Once we finish HTTPbis we may want to review all code comments
mentioning drafts and previous RFCs and update all of those :-)

[mmenke, 2011-10-25 19:37:32]

On 2011/10/25 19:01:45, julian.reschke wrote:
> On 2011/10/25 18:54:00, Matt Menke wrote:
> > How's this:
> > 
> >   // For 303 redirects, all request methods are converted to GETs, as per
RFC
> >   // 2616.  The latest httpbis draft also allows POST requests to be
converted
> 
> Actually, HEAD shouldn't be rewritten ;-) But maybe we can treat this as a
> separate issue (it's another thing we clarify in -17, see
> <http://trac.tools.ietf.org/wg/httpbis/trac/ticket/310>. (I don't think the
> difference will be observable from XMLHTTPRequest, but doing a GET when the
> client only needed HEAD is a waste of bits on the wire...)

I noticed that earlier.  Wasn't sure if there's a reason we're rewriting 303
HEAD requests or not.

wtc, willchan:  Any reason for it?  If so, should probably mention that as well.
 If not, any reason not to fix it, if for no other reason than so I don't have
to give a reason we're not adhering to spec in my comment?  :)

[julian.reschke, 2011-10-25 19:47:16]

On 2011/10/25 19:37:32, Matt Menke wrote:
> ...
> wtc, willchan:  Any reason for it?  If so, should probably mention that as
well.
>  If not, any reason not to fix it, if for no other reason than so I don't have
> to give a reason we're not adhering to spec in my comment?  :)

I think the history for 303/HEAD is that HTML forms do not matter, and XHR
callers do not really care as long as the underlying library makes sure that the
response body is silently consumed.

In RFC 2616 this wasn't clear, so we have added a clarification in -17 as well.
And yes, other UAs get that wrong as well.

[willchan no longer on Chromium, 2011-10-25 20:03:47]

I'm down for fixing it and seeing if people complain afterward. This is
early enough in Chrome 17's cycle that we have plenty of time to experiment
and see what breaks for people.

On Tue, Oct 25, 2011 at 12:47 PM, <julian.reschke@googlemail.com> wrote:

> On 2011/10/25 19:37:32, Matt Menke wrote:
>
>> ...
>>
>> wtc, willchan:  Any reason for it?  If so, should probably mention that as
>>
> well.
>
>>  If not, any reason not to fix it, if for no other reason than so I don't
>> have
>> to give a reason we're not adhering to spec in my comment?  :)
>>
>
> I think the history for 303/HEAD is that HTML forms do not matter, and XHR
> callers do not really care as long as the underlying library makes sure
> that the
> response body is silently consumed.
>
> In RFC 2616 this wasn't clear, so we have added a clarification in -17 as
> well.
> And yes, other UAs get that wrong as well.
>
>
>
http://codereview.chromium.**org/8375002/<http://codereview.chromium.org/8375...
>

[mmenke, 2011-10-25 20:07:45]

On 2011/10/25 20:03:47, willchan wrote:
> I'm down for fixing it and seeing if people complain afterward. This is
> early enough in Chrome 17's cycle that we have plenty of time to experiment
> and see what breaks for people.

Assuming there are no objections, I'll plan to land this as-is tomorrow, and
then I'll make a quick little follow up CL to fix the 303 issue and slightly
update the comment, so if anything breaks, can be reverted independently.

[wtc, 2011-10-25 20:42:56]

mmenke: the url_request.cc comments in Patch Set 7 LGTM.
Thanks!  (I didn't review the other files.)

Your plan to land this as-is first and deal with the 303/HEAD
issue in a follow-up CL is fine by me.

http://codereview.chromium.org/8375002/diff/24005/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/8375002/diff/24005/net/url_request/url_request...
net/url_request/url_request.cc:701: // normal browsers do this and so shall we. 
The RFC says to prompt the user

Nit: "normal browsers" sounds weird.  Are there abnormal
browsers? :-)

(I know this comes from the original comment.)

[mmenke, 2011-10-25 20:53:33]

http://codereview.chromium.org/8375002/diff/24005/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/8375002/diff/24005/net/url_request/url_request...
net/url_request/url_request.cc:701: // normal browsers do this and so shall we. 
The RFC says to prompt the user
On 2011/10/25 20:42:56, wtc wrote:
> 
> Nit: "normal browsers" sounds weird.  Are there abnormal
> browsers? :-)
> 
> (I know this comes from the original comment.)

I think the comment implies Chrome isn't a 'normal' browser, otherwise the "and
so shall we" would be unnecessary.  I assume that's because Chrome is a superior
browser, rather than a normal one (And presumably not abnormal or paranormal).

I've switch it to "Most major browsers"

[commit-bot: I haz the power, 2011-10-25 23:38:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/8375002/20012

[commit-bot: I haz the power, 2011-10-26 00:50:31]

Change committed as 107261

[julian.reschke, 2011-10-31 15:46:42]

Stable link to published Internet Draft:
https://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-17#section-7.3

[mmenke, 2011-10-31 15:49:05]

On 2011/10/31 15:46:42, julian.reschke wrote:
> Stable link to published Internet Draft:
> https://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-17#section-7.3

Thanks.  I'll update it in the 303 HEAD CL.