965869 - http/2 draft 10

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u408661]

The draft isn't even officially ready yet, but that doesn't mean we can't have a bug for it!

Both the client implementation and the test server will need upgraded.

Comment 2

[u408661]

Attachment: patch

So here's a patch that interops with nghttp2 just fine (including padding).

There is one thing missing - verification of the key type and strength requirements (DHE >= 2048b or ECDHE >= 128b). Part of that is in the code, but is #if'd out, as it causes us to not connect to nghttp2. I haven't figured out yet if we're just doing the check at the wrong time or if we're actually negotiating the wrong stuff. I'll get to that this week.

There's probably other things that are ugly, so don't be surprised.

We also need the test server to update. I'll start working on a pull request for node-http2 this week (and maybe even finish it, depending on amount of free time) so we can get the tests updated and this whole thing landed.

Comment 3

[Patrick McManus [:mcmanus]]

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

nick says to expect a new version

Comment 4

[u408661]

Attachment: patch v2

And here is said new version, with an update (not yet upstreamed) to node-http2 so that the tests work. Hooray!

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 8389268 [details] [diff] [review]
patch v2

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

let's make the testing patch live in its own changeset.. one bug is fine though

::: netwerk/protocol/http/Http2HuffmanIncoming.h
@@ +15,1 @@
>    HuffmanIncomingTable *mPtr;

HuffmanIncomingEntry::mValue can probably be 8 bit, and struct can be packed better

::: netwerk/protocol/http/Http2Session.cpp
@@ +1039,5 @@
> +
> +    if (self->mInputFrameFlags & kFlag_PAD_HIGH) {
> +      uint8_t paddingHighValue = *reinterpret_cast<uint8_t *>(self->mInputFrameBuffer + 8);
> +      paddingLength = static_cast<uint16_t>(paddingHighValue) * 256;
> +      ++paddingControlBytes;

if (!pad_low) return sessionerror()

@@ +1322,5 @@
> +
> +    if (self->mInputFrameFlags & kFlag_PAD_HIGH) {
> +      uint8_t paddingHighValue = *reinterpret_cast<uint8_t *>(self->mInputFrameBuffer + 8);
> +      paddingLength = static_cast<uint16_t>(paddingHighValue) * 256;
> +      ++paddingControlBytes;

if (!pad_low) return session err

@@ +1325,5 @@
> +      paddingLength = static_cast<uint16_t>(paddingHighValue) * 256;
> +      ++paddingControlBytes;
> +    }
> +
> +    if (self->mInputFrameFlags & kFlag_PAD_HIGH) {

I think you mean PAD_LOW?

@@ +1330,5 @@
> +      uint8_t paddingLowValue = *reinterpret_cast<uint8_t *>(self->mInputFrameBuffer + 8 + paddingControlBytes);
> +      paddingLength += paddingLowValue;
> +      ++paddingControlBytes;
> +    }
> +  }

maybe the two blocks (HIGH + LOW) above can be put into a helper function.. we've got at least 3 copies of this logic

@@ -2523,5 @@
>        SetNeedsCleanup();
>        return NS_BASE_STREAM_CLOSED;
>      }
>  
> -    count = std::min(count, mInputFrameDataSize - mInputFrameDataRead);

I think this change is incorrect.. the original meaning was "read what is left in the data frame"

@@ +2618,5 @@
> +      if (mInputFrameFlags & kFlag_PAD_HIGH) {
> +        char c = *data;
> +        ++data;
> +        ++paddingControlBytes;
> +        paddingLength = static_cast<uint16_t>(c) * 256;

if !PAD_LOW SesionError()

@@ +2640,5 @@
> +              mInputFrameDataSize));
> +        RETURN_SESSION_ERROR(this, PROTOCOL_ERROR);
> +      }
> +
> +      mHttpDataLength = mInputFrameDataSize - totalPadding;

lets just call that mDataLength

@@ +2656,5 @@
> +    // when we get back to our WriteSegments call, we will replace the HTTP-only
> +    // byte count with the on-wire byte count, so the HTTP connection layer can
> +    // do appropriate byte count reporting.
> +    mActualCountWritten = *countWritten;
> +

as discussed, rather than preserving countWritten and having actualCountWritten to deal with the countWritten = 0 case, let's try and return countWritten and NS_ERROR_STREAM_WOULD_BLOCK and get rid of actualCountWritten interfaces - especially as they are exposed outside of htttp2 classes

@@ +2813,5 @@
> +    LOG3(("Http2Session::ConfirmTLSProfile %p FAILED due to lack of TLS1.2\n", this));
> +    RETURN_SESSION_ERROR(this, INADEQUATE_SECURITY);
> +  }
> +
> +#if 0

let's resolve this before landing

::: netwerk/protocol/http/SpdySession3.cpp
@@ +2443,5 @@
>    return NS_ERROR_UNEXPECTED;
>  }
>  
> +nsresult
> +SpdySession3::GetActualCountWritten(uint32_t *)

Hopefully we don't need this in nsAHttpTransaction at all - but if we do need it let's provide a base implementation in the abstract class so this stub isn't implemented again and again and again

::: netwerk/protocol/http/SpdySession31.cpp
@@ +2576,5 @@
>    return NS_ERROR_UNEXPECTED;
>  }
>  
> +nsresult
> +SpdySession31::GetActualCountWritten(uint32_t *)

Hopefully we don't need this in nsAHttpTransaction at all - but if we do need it let's provide a base implementation in the abstract class so this stub isn't implemented again and again and again

::: netwerk/protocol/http/SpdyStream3.cpp
@@ +1480,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +SpdyStream3::GetActualCountWritten(uint32_t *)

Hopefully we don't need this in nsAHttpTransaction at all - but if we do need it let's provide a base implementation in the abstract class so this stub isn't implemented again and again and again

::: netwerk/protocol/http/SpdyStream31.cpp
@@ +1505,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +SpdyStream31::GetActualCountWritten(uint32_t *)

Hopefully we don't need this in nsAHttpTransaction at all - but if we do need it let's provide a base implementation in the abstract class so this stub isn't implemented again and again and again

::: netwerk/protocol/http/nsAHttpTransaction.h
@@ +219,5 @@
>      // any returned failure code stops segment iteration
>      virtual nsresult OnWriteSegment(char *segment,
>                                      uint32_t count,
>                                      uint32_t *countWritten) = 0;
> +    virtual nsresult GetActualCountWritten(uint32_t *actualCountWritten) = 0;

I hope we can get rid of this interface, but if we cannot - provide a base implementation of this like IsNullTransaction() is done (returning NOT_IMPLEMENTED). Then remove it from NS_DECL_NSAHTTPSEGMENTWRITER and declare it manually in the appropriate headers

::: netwerk/protocol/http/nsHttp.h
@@ +33,5 @@
>          // 24 was a internal spdy/3.1
>          // 25 was spdy/4a2
>          // 26 was http/2-draft08 and http/2-draft07 (they were the same)
> +        // 27 was http/2-draft09
> +        HTTP2_VERSION_DRAFT10 = 28

you can leave this at 27 and just update the comment.. no need to tease apart telemetry yet

::: netwerk/protocol/http/nsHttpConnection.cpp
@@ +1449,5 @@
>      return mSocketInCondition;
>  }
>  
>  nsresult
> +nsHttpConnection::GetActualCountWritten(uint32_t *)

see other comments on this function

::: netwerk/protocol/http/nsHttpPipeline.cpp
@@ +51,5 @@
>          *countWritten = count;
>          return NS_OK;
>      }
>  
> +    nsresult GetActualCountWritten(uint32_t *)

See other GetActualCountWritten comments

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ -683,5 @@
>          MOZ_EVENT_TRACER_MARK(static_cast<nsAHttpTransaction*>(trans),
>                                "net::http::first-read");
>      }
>  
> -    MOZ_ASSERT(*countWritten > 0, "bad writer");

potentially other layers of the stack will make this same assertion.. I think removing is something we want to avoid if possible

@@ +692,5 @@
> +    // traffic used by an app, which happens right here. So, for just this one
> +    // little thing, we have to reach into the depths of the code and get a
> +    // number that we otherwise don't care about until later.
> +    uint32_t actualCountWritten = *countWritten;
> +    trans->mWriter->GetActualCountWritten(&actualCountWritten);

comment assumption is that NS_FAILED means actualCountWritten unchanged

@@ +707,5 @@
> +        // OnInputStreamReady until all headers have been parsed.
> +        //
> +        rv = trans->ProcessData(buf, *countWritten, countWritten);
> +        if (NS_FAILED(rv))
> +            trans->Close(rv);

braces to avoid goto fail syndrome. (maintainers burden :()

Comment 6

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #5)
> Comment on attachment 8389268 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8389268 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> let's make the testing patch live in its own changeset.. one bug is fine
> though

Done in the next version.

> ::: netwerk/protocol/http/Http2HuffmanIncoming.h
> @@ +15,1 @@
> >    HuffmanIncomingTable *mPtr;
> 
> HuffmanIncomingEntry::mValue can probably be 8 bit, and struct can be packed
> better

We need the 16 bits (sadly), but the struct is successfully packed better.

> ::: netwerk/protocol/http/Http2Session.cpp
> @@ +1039,5 @@
> > +
> > +    if (self->mInputFrameFlags & kFlag_PAD_HIGH) {
> > +      uint8_t paddingHighValue = *reinterpret_cast<uint8_t *>(self->mInputFrameBuffer + 8);
> > +      paddingLength = static_cast<uint16_t>(paddingHighValue) * 256;
> > +      ++paddingControlBytes;
> 
> if (!pad_low) return sessionerror()

This is already taken care of in WriteSegments, where we first parse the frame header.

> @@ +1325,5 @@
> > +      paddingLength = static_cast<uint16_t>(paddingHighValue) * 256;
> > +      ++paddingControlBytes;
> > +    }
> > +
> > +    if (self->mInputFrameFlags & kFlag_PAD_HIGH) {
> 
> I think you mean PAD_LOW?

*facepalm*

> @@ +1330,5 @@
> > +      uint8_t paddingLowValue = *reinterpret_cast<uint8_t *>(self->mInputFrameBuffer + 8 + paddingControlBytes);
> > +      paddingLength += paddingLowValue;
> > +      ++paddingControlBytes;
> > +    }
> > +  }
> 
> maybe the two blocks (HIGH + LOW) above can be put into a helper function..
> we've got at least 3 copies of this logic

Done in 2 places (where we have the full frame buffered), but not in the third (where we're reading off the wire as needed, and the logic is way simpler).

> @@ -2523,5 @@
> >        SetNeedsCleanup();
> >        return NS_BASE_STREAM_CLOSED;
> >      }
> >  
> > -    count = std::min(count, mInputFrameDataSize - mInputFrameDataRead);
> 
> I think this change is incorrect.. the original meaning was "read what is
> left in the data frame"

*facepalm*

> @@ +2618,5 @@
> > +      if (mInputFrameFlags & kFlag_PAD_HIGH) {
> > +        char c = *data;
> > +        ++data;
> > +        ++paddingControlBytes;
> > +        paddingLength = static_cast<uint16_t>(c) * 256;
> 
> if !PAD_LOW SesionError()

This is already taken care of in WriteSegments, when we're still buffering the frame header.

> @@ +2813,5 @@
> > +    LOG3(("Http2Session::ConfirmTLSProfile %p FAILED due to lack of TLS1.2\n", this));
> > +    RETURN_SESSION_ERROR(this, INADEQUATE_SECURITY);
> > +  }
> > +
> > +#if 0
> 
> let's resolve this before landing

This is still to be done in the next version of the patch, but I wanted to get the majority of the review done before we figure this one out. 

> ::: netwerk/protocol/http/nsAHttpTransaction.h
> @@ +219,5 @@
> >      // any returned failure code stops segment iteration
> >      virtual nsresult OnWriteSegment(char *segment,
> >                                      uint32_t count,
> >                                      uint32_t *countWritten) = 0;
> > +    virtual nsresult GetActualCountWritten(uint32_t *actualCountWritten) = 0;
> 
> I hope we can get rid of this interface

It's gone, so all the rest of your comments around it are hereby rendered irrelevant :)

> @@ +707,5 @@
> > +        // OnInputStreamReady until all headers have been parsed.
> > +        //
> > +        rv = trans->ProcessData(buf, *countWritten, countWritten);
> > +        if (NS_FAILED(rv))
> > +            trans->Close(rv);
> 
> braces to avoid goto fail syndrome. (maintainers burden :()

So in the new version of the patch, this file isn't touched at all. I'm more than happy to fix this potential goto fail, but let's do it in a separate bug.

Comment 7

[u408661]

Attachment: part 1 - client implementation

Here's the client implementation. Tests (including node-http2 upgrade) in another patch.

Comment 8

[u408661]

Attachment: part 2 - tests

And here's the tests. node-http2 has already been reviewed by Gábor, so you shouldn't have to worry about that, the only changes for you are the xpcshell test.

Comment 9

[u408661]

Attachment: part 1 - client implementation

Oops, last version was missing a couple minor changes. Here's the real(-ish) one :)

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8395808 [details] [diff] [review]
part 2 - tests

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

::: testing/xpcshell/node-http2/node_modules/http2-protocol/.gitignore
@@ +1,1 @@
> +node_modules

did you mean to add this?

Comment 11

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #10)
> ::: testing/xpcshell/node-http2/node_modules/http2-protocol/.gitignore
> @@ +1,1 @@
> > +node_modules
> 
> did you mean to add this?

Yep... node_modules is used only during development (it's where npm installs the dependencies for things like unit tests, etc). This doesn't affect our xpcshell test at all.

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8395812 [details] [diff] [review]
part 1 - client implementation

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

i think this is ready. thanks nick!

::: netwerk/protocol/http/Http2Session.cpp
@@ +2803,5 @@
> +  uint16_t kea = ssl->GetKEAUsed();
> +  if (kea != ssl_kea_dh && kea != ssl_kea_ecdh) {
> +    LOG3(("Http2Session::ConfirmTLSProfile %p FAILED due to invalid KEA %d\n",
> +          this, kea));
> +    RETURN_SESSION_ERROR(this, INADEQUATE_SECURITY);

let's keep working this, but I'm ok now with landing the code with it disabled..  we're essentially missing an enforcement of a MUST-NOT from our peer..

::: netwerk/protocol/http/Http2Session.h
@@ +210,5 @@
>    enum internalStateType {
>      BUFFERING_OPENING_SETTINGS,
>      BUFFERING_FRAME_HEADER,
>      BUFFERING_CONTROL_FRAME,
> +    PROCESSING_DATA_FRAME_PADDING_CONTROL,

as awkward as this data-frame tristate is, its certainly better than where we were :)

Comment 13

[u408661]

Thanks, Pat! Quick try run to make sure I didn't do anything monumentally stupid, and then I'll get this landed (likely tomorrow) https://tbpl.mozilla.org/?tree=Try&rev=5542649140e7

Comment 14

[u408661]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/66007409ca35
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/70a43999d28d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66007409ca35
https://hg.mozilla.org/mozilla-central/rev/70a43999d28d