1091263 - [http/2] HTTP_1_1_REQUIRED error code

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u408661]

This is new in draft15 (and the only actual difference between draft15 and draft14). We should at least make sure we don't barf on this error code, and preferably actually dial back with http/1.1 if we get it, since that's what the spec wants us to do.

Comment 1

[u408661]

Attachment: patch

So here's a thing that implements HTTP_1_1_REQUIRED. The one bit of functionality that it could be argued should exist in this patch but doesn't is a "pinning" of HTTP_1_1_REQUIRED (at least for the duration of the browser session). This means that, if a session gets a GOAWAY/HTTP_1_1_REQUIRED, later channels that want to connect to the same origin will try h2 the first time around (potentially getting the same GOAWAY and having to dial back).

I'm not particularly worried about that happening, because HTTP_1_1_REQUIRED is something, IMHO, we should be discouraging, so sub-optimal behavior is fine here. It could be argued, though, that our behavior might look like a DoS. Pat, I'll leave that call up to you as module owner. If you want it, just cancel the r? and let me know, and I'll put something together.

Comment 2

[Patrick McManus [:mcmanus]]

I don't think this should be persistent.. so that's fine.

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 8517660 [details] [diff] [review]
patch

Review of attachment 8517660 [details] [diff] [review]:
-----------------------------------------------------------------

the patch needs to deal with the rst_stream case too, right? e.g. apache let's you setup client cert validation for just some resources..

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1795,5 @@
> +void
> +nsHttpTransaction::DisableSpdy()
> +{
> +    mCaps |= NS_HTTP_DISALLOW_SPDY;
> +    if (mConnInfo) {

make a comment that this is a cloned conn info - not the persistent one associated with the connection manager

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +176,5 @@
>      bool TimingEnabled() const { return mCaps & NS_HTTP_TIMING_ENABLED; }
>  
>      bool ResponseTimeoutEnabled() const MOZ_FINAL;
>  
> +    void DisableSpdy();

moz_override

::: netwerk/test/unit/test_http2.js
@@ +500,5 @@
>              , test_http2_multiplex
>              , test_http2_big
>              , test_http2_post
>              , test_http2_pushapi_1
> +            // This must always come last, since it causes the session to close

why does it matter that the session closes? wouldn't a new subsequent test open a new one given that it is a non-persistent condition?

Comment 4

[Patrick McManus [:mcmanus]]

thanks for doing this btw

Comment 5

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #3) 
> the patch needs to deal with the rst_stream case too, right? e.g. apache
> let's you setup client cert validation for just some resources..

It does! (At least, the code in necko allows for it - I didn't write a specific test for that case. I can add one.)

> ::: netwerk/protocol/http/nsHttpTransaction.cpp
> @@ +1795,5 @@
> > +void
> > +nsHttpTransaction::DisableSpdy()
> > +{
> > +    mCaps |= NS_HTTP_DISALLOW_SPDY;
> > +    if (mConnInfo) {
> 
> make a comment that this is a cloned conn info - not the persistent one
> associated with the connection manager

Will do.

> ::: netwerk/protocol/http/nsHttpTransaction.h
> @@ +176,5 @@
> >      bool TimingEnabled() const { return mCaps & NS_HTTP_TIMING_ENABLED; }
> >  
> >      bool ResponseTimeoutEnabled() const MOZ_FINAL;
> >  
> > +    void DisableSpdy();
> 
> moz_override

Will do.

> ::: netwerk/test/unit/test_http2.js
> @@ +500,5 @@
> >              , test_http2_multiplex
> >              , test_http2_big
> >              , test_http2_post
> >              , test_http2_pushapi_1
> > +            // This must always come last, since it causes the session to close
> 
> why does it matter that the session closes? wouldn't a new subsequent test
> open a new one given that it is a non-persistent condition?

I guess it doesn't (even though that's my preference). Since the comment just reflects my preference, I'll go ahead and remove the comment. I might at least place a restriction that the two h11required tests (rst stream and goaway) be together in a particular order, to ensure the rst stream case doesn't end up closing down the connection.

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8517660 [details] [diff] [review]
patch

Review of attachment 8517660 [details] [diff] [review]:
-----------------------------------------------------------------

you're right.. I expected to see it in the rcv_rst_stream handler, but its on the stack in writesegments. gotcha.
thanks

Comment 7

[u408661]

Attachment: patch v2

Good thing I added the test for the rst stream version, and ensured it didn't close down the existing connection... because that's what it did! (The nastiness from bug 1049814 struck again.)

Instead of taking the bool-on-stack approach that I took in the original patch form 1049814, I added some state to nsHttpTransaction. (That approach seemed easier and less invasive than plumbing parameters all the way down into nsHttpTransaction::Restart.) We can use this (or a future iteration of it, if you don't like this implementation) for bug 1049814, as well.

Other than that (and the new test, with server-side support), this is the same patch as before.

Comment 8

[u408661]

Of course, I forgot to add the one comment you mentioned in your initial review. I've added that to the bits that will land.

Comment 9

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #6)
> you're right.. I expected to see it in the rcv_rst_stream handler, but its
> on the stack in writesegments. gotcha.
> thanks

Yeah, I put it there, since that's where we already do other messing about with the state of the transaction (to force restarts in certain cases, for example), so it seemed to make the most sense there.

Comment 10

[u408661]

Attachment: patch v2.1

Update patch for checkin w/the comment Patrick requested.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a180f3804f91

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/785384968f31

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/785384968f31