Correctly create a new stream when the server requires authentication.

Correctly create a new stream when the server requires authentication.

The silly existing behavior is try to create another stream with the same ID,
which causes a crash.

BUG=102295
TEST=Start Chrome with --enable-http-pipelining, visit
     http://test.webdav.org/auth-basic/, enter "user1" for both username and
     password. It should return a "Not found" page.


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

[cbentzel, 2011-11-01 13:03:11]

I don't know the pipelining code - but will this create a new TCP
connection?

For Negotiate+NTLM auth we need to make sure that each round of the
handshake is done on the same TCP connection.

On Mon, Oct 31, 2011 at 10:53 PM, <simonjam@chromium.org> wrote:

> Reviewers: Matt Menke, willchan,
>
> Description:
> Correctly create a new stream when the server requires authentication.
>
> The silly existing behavior is try to create another stream with the same
> ID,
> which causes a crash.
>
> BUG=102295
> TEST=Start Chrome with --enable-http-pipelining, visit
>     http://test.webdav.org/auth-**basic/<http://test.webdav.org/auth-basic/>,
> enter "user1" for both username and
>     password. It should return a "Not found" page.
>
>
> Please review this at
http://codereview.chromium.**org/8432024/<http://codereview.chromium.org/8432...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M net/http/http_pipelined_**stream.cc
>
>
> Index: net/http/http_pipelined_**stream.cc
> diff --git a/net/http/http_pipelined_**stream.cc
> b/net/http/http_pipelined_**stream.cc
> index 2183c528fcfbd749c99683af6cc4b2**0e73ac1b7f..**
> f4b8401d187f3705b268d966e09d94**670e1738eb 100644
> --- a/net/http/http_pipelined_**stream.cc
> +++ b/net/http/http_pipelined_**stream.cc
> @@ -71,8 +71,11 @@ void HttpPipelinedStream::Close(**bool not_reusable) {
>  }
>
>  HttpStream* HttpPipelinedStream::**RenewStreamForAuth() {
> -  // FIXME: What does this mean on a pipeline? Is it for proxies?
> -  return new HttpPipelinedStream(pipeline_, pipeline_id_);
> +  if (pipeline_->usable()) {
> +    return pipeline_->CreateNewStream();
> +  } else {
> +    return NULL;
> +  }
>  }
>
>  bool HttpPipelinedStream::**IsResponseBodyComplete() const {
>
>
>

[mmenke, 2011-11-01 13:54:23]

On 2011/11/01 13:03:11, cbentzel wrote:
> I don't know the pipelining code - but will this create a new TCP
> connection?
> 
> For Negotiate+NTLM auth we need to make sure that each round of the
> handshake is done on the same TCP connection.

This will reuse the connection, however, since it's a pipelined connection,
there's no guarantee there won't have been an intervening request between the
auth query and response.  If this is a problem, on these requests, we'll have to
abandon the request entirely, and start from scratch on a non-pipelined
connection, at least in some cases.

[cbentzel, 2011-11-01 14:07:10]

On Tue, Nov 1, 2011 at 9:54 AM, <mmenke@chromium.org> wrote:

> On 2011/11/01 13:03:11, cbentzel wrote:
>
>> I don't know the pipelining code - but will this create a new TCP
>> connection?
>>
>
>  For Negotiate+NTLM auth we need to make sure that each round of the
>> handshake is done on the same TCP connection.
>>
>
> This will reuse the connection, however, since it's a pipelined connection,
> there's no guarantee there won't have been an intervening request between
> the
> auth query and response.  If this is a problem, on these requests, we'll
> have to
> abandon the request entirely, and start from scratch on a non-pipelined
> connection, at least in some cases.
>

That will likely be a problem - it will likely restart the handshake, and
may mean that there is no guarantee that it will succeed. We'll have to
deal with both servers which have authentication and proxies which have
this authentication.


>
http://codereview.chromium.**org/8432024/<http://codereview.chromium.org/8432...
>

[mmenke, 2011-11-01 14:15:00]

On 2011/11/01 14:07:10, cbentzel wrote:
> That will likely be a problem - it will likely restart the handshake, and
> may mean that there is no guarantee that it will succeed. We'll have to
> deal with both servers which have authentication and proxies which have
> this authentication.

That's going to make this case much more complicated to handle correctly.  Since
this fix will at least get around a crash (The old code created the new stream
incorrectly), I suggest we land this as-is, and fix the proxy case in a
subsequent CL.  We'll going to have to handle server's that don't support
pipelining anyways, and we could reuse that for authentication, just by not
pipelining servers that use certain types of authentication.

If we're willing to risk having to re-send requests, we can think about
re-sending requests on pipelining + auth, if there's another request that's gone
out since then, or just preventing the pipeline from taking new requests until
the auth negotiation is done, otherwise.

[cbentzel, 2011-11-01 14:17:20]

Yes, I agree that this should be taken for the crash fix, but need to think
through the Negotiate/NTLM + pipelining case.

On Tue, Nov 1, 2011 at 10:15 AM, <mmenke@chromium.org> wrote:

> On 2011/11/01 14:07:10, cbentzel wrote:
>
>> That will likely be a problem - it will likely restart the handshake, and
>> may mean that there is no guarantee that it will succeed. We'll have to
>> deal with both servers which have authentication and proxies which have
>> this authentication.
>>
>
> That's going to make this case much more complicated to handle correctly.
>  Since
> this fix will at least get around a crash (The old code created the new
> stream
> incorrectly), I suggest we land this as-is, and fix the proxy case in a
> subsequent CL.  We'll going to have to handle server's that don't support
> pipelining anyways, and we could reuse that for authentication, just by not
> pipelining servers that use certain types of authentication.
>
> If we're willing to risk having to re-send requests, we can think about
> re-sending requests on pipelining + auth, if there's another request
> that's gone
> out since then, or just preventing the pipeline from taking new requests
> until
> the auth negotiation is done, otherwise.
>
>
http://codereview.chromium.**org/8432024/<http://codereview.chromium.org/8432...
>

[mmenke, 2011-11-01 14:20:24]

Code LGTM, then.

[mmenke, 2011-11-01 14:21:55]

Modulo nit.

http://codereview.chromium.org/8432024/diff/1/net/http/http_pipelined_stream.cc
File net/http/http_pipelined_stream.cc (right):

http://codereview.chromium.org/8432024/diff/1/net/http/http_pipelined_stream....
net/http/http_pipelined_stream.cc:76: } else {
nit:  It's chrome style not to use else statements in this case.

[commit-bot: I haz the power, 2011-11-01 18:10:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/8432024/5001

[James Simonsen, 2011-11-01 18:18:40]

On 2011/11/01 14:17:20, cbentzel wrote:
> Yes, I agree that this should be taken for the crash fix, but need to think
> through the Negotiate/NTLM + pipelining case.

It sounds like the best we'll be able to do is just stop pipelining if we see
NTLM. The pipelining code can't guarantee that the same TCP connection can be
re-used, because it might be closed due to other pipelining-related failures.

I'll disable pipelining on NTLM once I write the code to dynamically disable
pipelining.

[commit-bot: I haz the power, 2011-11-01 19:22:31]

Change committed as 108151