Closed Bug 1091263 Opened 5 years ago Closed 5 years ago

[http/2] HTTP_1_1_REQUIRED error code

Categories

(Core :: Networking: HTTP, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: u408661, Assigned: u408661)

References

Details

(Whiteboard: [spdy])

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #8517660 - Flags: review?(mcmanus)
I don't think this should be persistent.. so that's fine.
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?
Attachment #8517660 - Flags: review?(mcmanus)
thanks for doing this btw
(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 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
Attachment #8517660 - Flags: review+
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #8517660 - Attachment is obsolete: true
Attachment #8517826 - Flags: review?(mcmanus)
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.
Blocks: 1094519
(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.
Attachment #8517826 - Flags: review?(mcmanus) → review+
Attached patch patch v2.1Splinter Review
Update patch for checkin w/the comment Patrick requested.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a180f3804f91
Attachment #8517826 - Attachment is obsolete: true
Attachment #8518254 - Flags: review+
Keywords: checkin-needed
Blocks: 1049814
https://hg.mozilla.org/mozilla-central/rev/785384968f31
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1101157
You need to log in before you can comment on or make changes to this bug.