950768 - Land http/2 draft08 pref'd off in mozilla-central

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u408661]

Current plan is to land draft08 pref'd off in nightly, then (shortly after, before the Zurich interim) land draft09 pref'd on in nightly. This bug exists to have a place to review the draft08 patches before landing them.

Comment 1

[u408661]

Attachment: http2_draft08.patch

Herein lies the gigantic patch for http/2-draft08. There's lots of bits that need reviewing, some needs reviewed by me (testing stuff, main protocol implementation, most of the compression), some needs reviewed by mcmanus (most of the compression), and some by others (PSM, NSS, & XPCOM Glue changes) I have not yet identified (but will in concert with mcmanus).

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: 0001-nss-alpn-support.patch

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: 0002-use-alpn-in-gecko-psm.patch

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: 0003-idl-for-getting-the-ssl-handshake-version.patch

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: 0004-nsDeque-SetObjectAt.patch

Comment 6

[Patrick McManus [:mcmanus]]

Attachment: 0005-http-2-draft08.patch

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: 0006-http2-tests-in-node-from-gabor.patch

Comment 8

[Patrick McManus [:mcmanus]]

Attachment: 0007-pref-on-http2.patch

Comment 9

[Patrick McManus [:mcmanus]]

well, this split doesn't make it much easier for you and me nick - but it at least pulls out some of the non core items. Let's review 5 and 6 here, and I'll use other bugs (tomorrow) fro 1 through 4.. they are just listed here (along with 7) for completeness, testing, etc..

I've rebased this against the current trunk.

I agree we should both review 5.. I'll leave 6 in nick's hands.

Comment 10

[Patrick McManus [:mcmanus]]

the 0005 patch shows a nsDeque leak on try.. Its presumably the static compressor table. I will fix it up and then upload a new patch when try gives green for it. After that it probably makes sense to take review-fixing patches as additions to the queue rather than modifications.

Comment 11

[Patrick McManus [:mcmanus]]

the linux platforms are all reporting (on try):

8:08:19     INFO -  TEST-INFO | skipping /builds/slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_spdy.js | run-if: hasNode
18:08:19     INFO -  TEST-INFO | skipping /builds/slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_http2.js | run-if: hasNode

Comment 12

[Patrick McManus [:mcmanus]]

Attachment: 0004-setobjectat

Comment 13

[Patrick McManus [:mcmanus]]

Comment on attachment 8348806 [details] [diff] [review]
0004-setobjectat

with the removal of substitution indexing we don't need this xpcom patch anymore

Comment 14

[Patrick McManus [:mcmanus]]

Attachment: 0005-http2-draft-08-patch.r2

this is the patch updated for the nsdq changes

Comment 15

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #11)
> the linux platforms are all reporting (on try):
> 
> 8:08:19     INFO -  TEST-INFO | skipping
> /builds/slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_spdy.js
> | run-if: hasNode
> 18:08:19     INFO -  TEST-INFO | skipping
> /builds/slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_http2.
> js | run-if: hasNode

Argh, yeah. I know why this is. We aren't zipping up everything we need now to run the tests (because why would we zip up everything in the xpcshell directory, when we can make it fragile by having you explicitly list what files you want in the test .zip?) It's a pretty simple fix, though it will likely cause the tests to fail on 32-bit linux (we need a more modern node than the test infrastructure has, and Gábor added one to the tree, but it's 64-bit only). I'm sure there's probably some xpcshell.ini incantation I can use to work around that, though.

Comment 16

[u408661]

Comment on attachment 8348468 [details] [diff] [review]
0006-http2-tests-in-node-from-gabor.patch

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

I've already reviewed this as part of the initial node-http2 implementation, and when the tests were being written over on github. As mentioned in comment 15, though, there are a couple more bits that need to be added to make the tests actually run.

Comment 17

[u408661]

Comment on attachment 8348844 [details] [diff] [review]
0005-http2-draft-08-patch.r2

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

I've gotten through everything bug Http2Stream.{cpp,h}. Those will come after the holidays.

::: modules/libpref/src/init/all.js
@@ +997,5 @@
>  pref("network.http.spdy.enabled", true);
>  pref("network.http.spdy.enabled.v3", true);
>  pref("network.http.spdy.enabled.v3-1", true);
> +pref("network.http.spdy.enabled.http2draft", false);
> +pref("network.http.spdy.enforce-tls-profile", true);

I'm not wild about shoving http2 under spdy in the prefs hierarchy. What do you think about network.http.v2.*? Can be done in a followup.

::: netwerk/protocol/http/Http2Compression.cpp
@@ +16,5 @@
> +
> +static nsDeque *gStaticHeaders = nullptr;
> +
> +void
> +Http2CompressionCleanup() 

nit: whitespace

@@ +403,5 @@
> +  }
> +
> +  // Status comes first
> +  if (name.Equals(NS_LITERAL_CSTRING(":status"))) {
> +    nsAutoCString status(NS_LITERAL_CSTRING("HTTP/1.1 "));

We should change this to 2.0 at some point. Doesn't have to be now (who knows what'll break when we do), but it's something to keep track of.

@@ +497,5 @@
> +
> +  // This is a character!
> +  if (table->mEntries[idx].mValue == 256) {
> +    // EOS
> +    foundEOS = true;

I think this has been recently updated to be a protocol error, so we should just return NS_ERROR_ILLEGAL_VALUE here. Probably doable as a followup, since it wasn't specified until now.

@@ +573,5 @@
> +                                  bitsLeft, foundEOS);
> +  }
> +
> +  if (table->mEntries[idx].mValue == 256) {
> +    foundEOS = true;

Same here.

::: netwerk/protocol/http/Http2HuffmanIncoming.h
@@ +1,2 @@
> +/*
> + * THIS FILE IS AUTO-GENERATED. DO NOT EDIT!

I have the scripts that auto-generate this and Http2HuffmanOutgoing.h in a github repo right now. I should document them and put them alongside these files to have everything in one place.

::: netwerk/protocol/http/Http2Session.cpp
@@ +47,5 @@
> +  0x54, 0x50, 0x2f, 0x32, 0x2e, 0x30, 0x0d, 0x0a,
> +  0x0d, 0x0a, 0x53, 0x4d, 0x0d, 0x0a, 0x0d, 0x0a
> +};
> +
> +#define ReturnSessionError(o,x)  \

total nit: I'm a fan of making this all caps with underscores, to call out that it's a macro (and thus will actually return from the function).

@@ +164,5 @@
> +  LOG3(("Http2Session::~Http2Session %p mDownstreamState=%X",
> +        this, mDownstreamState));
> +
> +  mStreamTransactionHash.Enumerate(ShutdownEnumerator, this);
> +  Telemetry::Accumulate(Telemetry::SPDY_PARALLEL_STREAMS, mConcurrentHighWater);

More followup fodder: Similar to prefs, I think we should break the telemetry out into HTTP2_* instead of SPDY_*. Makes it more obvious what's experimental and what's not (or at least on track to not being experimental), and will make it easier to rip out SPDY entirely if/when the time comes.

@@ +743,5 @@
> +
> +  // last-good-stream-id are bytes 8-11 reflecting pushes
> +  uint32_t goAway = PR_htonl(mOutgoingGoAwayID);
> +  memcpy (packet + 7, &goAway, 4);
> +  

nit: whitespace

@@ +1005,5 @@
> +
> +  // If this doesn't have END_HEADERS set on it then require the next
> +  // frame to be HEADERS of the same ID
> +  bool endHeadersFlag = self->mInputFrameFlags & kFlag_END_HEADERS;
> +  

nit: whitespace

@@ +1299,5 @@
> +    promisedID &= 0x7fffffff;
> +  }
> +
> +  uint32_t associatedID = self->mInputFrameID;
> +  

nit: whitespace

@@ +1307,5 @@
> +  } else {
> +    self->mExpectedPushPromiseID = self->mInputFrameID;
> +    self->mContinuedPromiseStream = promisedID;
> +  }
> +  

nit: whitespace

@@ +1320,5 @@
> +  // confirm associated-to
> +  nsresult rv = self->SetInputFrameDataStream(associatedID);
> +  if (NS_FAILED(rv))
> +    return rv;
> +  

n: w

@@ +1340,5 @@
> +  } else if (!gHttpHandler->AllowPush()) {
> +    // MAX_CONCURRENT_STREAMS of 0 in settings disabled push
> +    LOG3(("Http2Session::RecvPushPromise Push Recevied when Disabled\n"));
> +    self->GenerateRstStream(REFUSED_STREAM_ERROR, promisedID);
> +  } else if (!(self->mInputFrameFlags & kFlag_END_PUSH_PROMISE)) {

It looks like we already support this from later on in the patch. If I'm wrong, we can add the support as a followup, otherwise, we should kill this check.

@@ +1479,5 @@
> +    LOG3(("Http2Session::RecvPing %p PING needs stream ID of 0. 0x%X\n",
> +          self, self->mInputFrameID));
> +    ReturnSessionError(self, PROTOCOL_ERROR);
> +  }
> +  

n: w

@@ +1611,5 @@
> +  if (self->mInputFrameID) { // stream window
> +    nsresult rv = self->SetInputFrameDataStream(self->mInputFrameID);
> +    if (NS_FAILED(rv))
> +      return rv;
> +  

n: w

@@ +1621,5 @@
> +        self->GenerateRstStream(PROTOCOL_ERROR, self->mInputFrameID);
> +      self->ResetDownstreamState();
> +      return NS_OK;
> +    }
> +  

n: w

@@ +1638,5 @@
> +
> +    LOG3(("Http2Session::RecvWindowUpdate %p stream 0x%X window "
> +          "%d increased by %d now %d.\n", self, self->mInputFrameID,
> +          oldRemoteWindow, delta, oldRemoteWindow + delta));
> +  

n: w

@@ +1672,5 @@
> +  MOZ_ASSERT(self->mInputFrameType == FRAME_TYPE_CONTINUATION);
> +  MOZ_ASSERT(self->mInputFrameID);
> +  MOZ_ASSERT(self->mExpectedPushPromiseID || self->mExpectedHeaderID);
> +  MOZ_ASSERT(!(self->mExpectedPushPromiseID && self->mExpectedHeaderID));
> +  

n: w

@@ +1679,5 @@
> +        self, self->mInputFrameFlags, self->mInputFrameID,
> +        self->mExpectedPushPromiseID, self->mExpectedHeaderID));
> +
> +  self->SetInputFrameDataStream(self->mInputFrameID);
> +  

n: w

@@ +1691,5 @@
> +  if (self->mExpectedHeaderID) {
> +    self->mInputFrameFlags &= ~kFlag_PRIORITY;
> +    return RecvHeaders(self);
> +  }
> +  

n: w

@@ +1977,5 @@
> +    mInputFrameID &= 0x7fffffff;
> +    mInputFrameDataRead = 0;
> +
> +    if (mInputFrameType == FRAME_TYPE_DATA || mInputFrameType == FRAME_TYPE_HEADERS)  {
> +      mInputFrameFinal = mInputFrameFlags & kFlag_END_STREAM; 

n: w

@@ +2183,5 @@
> +
> +  MOZ_ASSERT(mInputFrameBufferUsed == 8, "Frame Buffer Header Not Present");
> +  MOZ_ASSERT(mInputFrameDataSize + 8 <= mInputFrameBufferSize,
> +             "allocation for control frame insufficient");
> +  

n: w

@@ +2597,5 @@
> +  }
> +
> +  if (!mConnection)
> +    return NS_ERROR_FAILURE;
> +  

n: w

Comment 18

[Patrick McManus [:mcmanus]]

Attachment: 008-soft-error

this is a port of 955161 that applies to the http2 code too

Comment 19

[u408661]

Comment on attachment 8355533 [details] [diff] [review]
008-soft-error

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

This patch appears to be missing some Very Important Pieces (namely those in Http2Session.cpp)

Comment 20

[Patrick McManus [:mcmanus]]

Attachment: 008-soft-error v2

sorry about that last one.. tooling fail. (well, no doubt UE with tooling)

Comment 21

[u408661]

Comment on attachment 8348844 [details] [diff] [review]
0005-http2-draft-08-patch.r2

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

Herein lie my comments for Http2Stream.{cpp,h}.

Looking at previous comments, this is r=me with a few of the issues addressed before landing (misleading comments are the only things I'm really dying to have fixed before landing, the rest can be done in cleanup patches and other followups)

::: netwerk/protocol/http/Http2Stream.cpp
@@ +201,5 @@
> +
> +// WriteSegments() is used to read data off the socket. Generally this is
> +// just the HTTP/2 frame header and from there the appropriate stream
> +// is identified from the Stream-ID. The http transaction associated with
> +// that read then pulls in the data directly.

This comment seems copied directly from Http2Session::WriteSegments, which makes it a bit misleading, since the Stream isn't the one reading the frame header :)

@@ +380,5 @@
> +  // frame for the new headers and for the first one a priority field. There is
> +  // no question this is ugly, but a 16KB HEADERS frame should be a long
> +  // tail event, so this is really just for correctness and a nop in the base case.
> +  //
> +  

nit: whitespace

@@ +388,5 @@
> +  uint32_t maxFrameData = Http2Session::kMaxFrameData - 4; // 4 byes for priority
> +  uint32_t numFrames = 1;
> +
> +  if (dataLength > maxFrameData) {
> +    numFrames += ((dataLength - maxFrameData) + Http2Session::kMaxFrameData - 1) / 

nit: whitespace

@@ +398,5 @@
> +
> +  uint32_t messageSize = dataLength;
> +  messageSize += 12; // header frame overhead
> +  messageSize += (numFrames - 1) * 8; // continuation frames overhead
> +  

nit: whitespace

@@ +429,5 @@
> +    dataLength -= frameLen;
> +
> +    mSession->CreateFrameHeader(mTxInlineFrame.get() + outputOffset,
> +                                frameLen + (idx ? 0 : 4),
> +                                (idx) ? 

nit: whitespace (this is also a case where i would be ok breaking the current 80-char rule, since i think ternary operators broken across lines are evil)

@@ +453,5 @@
> +  uint32_t ratio =
> +    compressedData.Length() * 100 /
> +    (11 + mTransaction->RequestHead()->RequestURI().Length() +
> +     mFlatHttpRequestHeaders.Length());
> +  

nit: whitespace

@@ +454,5 @@
> +    compressedData.Length() * 100 /
> +    (11 + mTransaction->RequestHead()->RequestURI().Length() +
> +     mFlatHttpRequestHeaders.Length());
> +  
> +  // TODO - is this the right place for this?

we can probably drop this comment, or at least change it to explain what on earth is actually going on here. i think my hack is going to stick for the foreseeable future (you may think otherwise)

@@ +524,5 @@
> +  }
> +
> +  uint8_t *packet = mTxInlineFrame.get() + mTxInlineFrameUsed;
> +  Http2Session::EnsureBuffer(mTxInlineFrame,
> +                                    mTxInlineFrameUsed + 12,

nit: indentation mismatch (hard tabs?)

@@ +552,5 @@
> +  if (mStreamID || !mPushSource)
> +    return;
> +
> +  MOZ_ASSERT(mPushSource->mStreamID && !(mPushSource->mStreamID & 1));
> +  

nit: whitespace

@@ +560,5 @@
> +    return;
> +
> +  uint8_t *packet = mTxInlineFrame.get() + mTxInlineFrameUsed;
> +  Http2Session::EnsureBuffer(mTxInlineFrame,
> +                                    mTxInlineFrameUsed + 12,

nit: indentation mismatch (hard tabs?)

@@ +750,5 @@
> +{
> +  LOG3(("Http2Stream::ChangeState() %p from %X to %X",
> +        this, mUpstreamState, newState));
> +  mUpstreamState = newState;
> +  return;

get rid of this

@@ +804,5 @@
> +  if (status.IsEmpty()) {
> +    LOG3(("Http2Stream::ConvertHeaders %p Error - no status\n", this));
> +    return NS_ERROR_ILLEGAL_VALUE;
> +  }
> +  

nit: whitespace

@@ +819,5 @@
> +  return NS_OK;
> +}
> +
> +// ConvertHeaders is used to convert the response headers
> +// into HTTP/1 format and report some telemetry

copy/pasted comment :(

@@ +878,5 @@
> +void
> +Http2Stream::UpdateServerReceiveWindow(int32_t delta)
> +{
> +  mServerReceiveWindow += delta;
> +  

nit: whitespace

@@ +897,5 @@
> +{
> +  mRecvdFin = aStatus ? 1 : 0;
> +  if (!aStatus)
> +    return;
> +  

nit: whitespace

@@ +901,5 @@
> +  
> +  if (mState == OPEN || mState == RESERVED_BY_REMOTE) {
> +    mState = CLOSED_BY_REMOTE;
> +  }
> +  else if (mState == CLOSED_BY_LOCAL) {

i thought our convention was } on the same line as else (also in SetSentFin, possibly other places I've missed)

@@ +909,5 @@
> +
> +void
> +Http2Stream::SetSentFin(bool aStatus)
> +{
> +  mSentFin = aStatus ? 1 : 0;  

nit: whitespace

@@ +912,5 @@
> +{
> +  mSentFin = aStatus ? 1 : 0;  
> +  if (!aStatus)
> +    return;
> +  

nit: whitespace

@@ +997,5 @@
> +    mBlockedOnRwin = false;
> +
> +    // The chunk is the smallest of: availableData, configured chunkSize,
> +    // stream window, session window, or 16 bit framing limit.
> +    // Its amazing we send anything at all.

turtles all the way down, man

@@ +1001,5 @@
> +    // Its amazing we send anything at all.
> +    dataLength = std::min(count, mChunkSize);
> +
> +    if (dataLength > Http2Session::kMaxFrameData)
> +      dataLength = Http2Session::kMaxFrameData;

why not also use std::min here?

@@ +1006,5 @@
> +
> +    if (mSession->ServerUsesFlowControl()) {
> +      if (dataLength > mSession->ServerSessionWindow())
> +        dataLength = static_cast<uint32_t>(mSession->ServerSessionWindow());
> +      

again with the explicit > check instead of std::min. also...
nit: whitespace

@@ +1008,5 @@
> +      if (dataLength > mSession->ServerSessionWindow())
> +        dataLength = static_cast<uint32_t>(mSession->ServerSessionWindow());
> +      
> +      if (dataLength > mServerReceiveWindow)
> +        dataLength = static_cast<uint32_t>(mServerReceiveWindow);

std::mincomment :)

::: netwerk/protocol/http/Http2Stream.h
@@ +9,5 @@
> +#include "mozilla/Attributes.h"
> +#include "nsAHttpTransaction.h"
> +#include "nsISupportsPriority.h"
> +
> +namespace mozilla { namespace net {

Should these be on separate lines?

@@ +142,5 @@
> +  nsCString     mOrigin;
> +  nsCString     mHeaderHost;
> +  nsCString     mHeaderScheme;
> +  nsCString     mHeaderPath;
> +  

nit: whitespace

Comment 22

[u408661]

Attachment: enable_tests.patch

This patch fixes up the last few bits required to make the tests run (and pass!) on infrastructure, not just locally.

(1) We weren't packaging up the required files during the build process, so the tests weren't running
(2) SPDY and HTTP2 tests can (for now) only run on x86_64, since we need a custom node binary (newer than the one on infrastructure), and the one we have checked in is 64-bit (this can be undone once we get infrastructure upgraded - I should file a bug for that)
(3) We were conflicting on port with some existing xpcshell tests, causing them to fail once our http2 server was running. Changed our port, everything's happy.
(4) node-http2 advertised draft07 implemented, instead of draft08 (same bits on the wire, different name) and the test was still checking for draft06 in the injected header.

All of this work also exists on github on the wip/draft09 branch of todesschaf/http2 (along with node-http2 updates to support draft09), so we're ready to go on draft09 tests when we get to landing that.

Comment 23

[Patrick McManus [:mcmanus]]

(In reply to Nicholas Hurley [:hurley] from comment #17)
> I'm not wild about shoving http2 under spdy in the prefs hierarchy. What do
> you think about network.http.v2.*? Can be done in a followup.
> 

It is a little awkward, but I think for the next couple years spdy is simply the name for early version of http/2 in the code :).. anything that is policy based around spdy is pretty much the same for http2.. so let's keep it the same for now.

> @@ +403,5 @@
> > +  }
> > +
> > +  // Status comes first
> > +  if (name.Equals(NS_LITERAL_CSTRING(":status"))) {
> > +    nsAutoCString status(NS_LITERAL_CSTRING("HTTP/1.1 "));
> 
> We should change this to 2.0 at some point. Doesn't have to be now (who
> knows what'll break when we do), but it's something to keep track of.

let's take that as a patch right after zurich

> 
> @@ +497,5 @@
> > +
> > +  // This is a character!
> > +  if (table->mEntries[idx].mValue == 256) {
> > +    // EOS
> > +    foundEOS = true;
> 
> I think this has been recently updated to be a protocol error, so we should
> just return NS_ERROR_ILLEGAL_VALUE here. Probably doable as a followup,
> since it wasn't specified until now.
> 

http-04 says "Given that only between 0-7 bits of the
   EOS symbol is included in any huffman-encoded string, and given that
   the EOS symbol is at least 8 bits long, it is expected that it should
   never be successfully decoded." .. so I'll put this in my review list of followups for you to do as we land -08

> 
> @@ +164,5 @@
> > +  LOG3(("Http2Session::~Http2Session %p mDownstreamState=%X",
> > +        this, mDownstreamState));
> > +
> > +  mStreamTransactionHash.Enumerate(ShutdownEnumerator, this);
> > +  Telemetry::Accumulate(Telemetry::SPDY_PARALLEL_STREAMS, mConcurrentHighWater);
> 
> More followup fodder: Similar to prefs, I think we should break the
> telemetry out into HTTP2_* instead of SPDY_*. Makes it more obvious what's
> experimental and what's not (or at least on track to not being
> experimental), and will make it easier to rip out SPDY entirely if/when the
> time comes.
> 

as above.. right now I consider this a rev of spdy. when that's historical we can make the shift

> 
> @@ +1340,5 @@
> > +  } else if (!gHttpHandler->AllowPush()) {
> > +    // MAX_CONCURRENT_STREAMS of 0 in settings disabled push
> > +    LOG3(("Http2Session::RecvPushPromise Push Recevied when Disabled\n"));
> > +    self->GenerateRstStream(REFUSED_STREAM_ERROR, promisedID);
> > +  } else if (!(self->mInputFrameFlags & kFlag_END_PUSH_PROMISE)) {
> 
> It looks like we already support this from later on in the patch. If I'm
> wrong, we can add the support as a followup, otherwise, we should kill this
> check.

it gracefully covers the case when the pref has been changed to false after the settings frame was sent.. and also if the server pushed data before it recvd our settings frame (which is impossible right now, but will certainly happen in the future). we should actually send a new settings when the pref changes, probably file a followup for it - but we would still need this check as it would be racy.

Comment 24

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #23)
> (In reply to Nicholas Hurley [:hurley] from comment #17)
> > @@ +1340,5 @@
> > > +  } else if (!gHttpHandler->AllowPush()) {
> > > +    // MAX_CONCURRENT_STREAMS of 0 in settings disabled push
> > > +    LOG3(("Http2Session::RecvPushPromise Push Recevied when Disabled\n"));
> > > +    self->GenerateRstStream(REFUSED_STREAM_ERROR, promisedID);
> > > +  } else if (!(self->mInputFrameFlags & kFlag_END_PUSH_PROMISE)) {
> > 
> > It looks like we already support this from later on in the patch. If I'm
> > wrong, we can add the support as a followup, otherwise, we should kill this
> > check.
> 
> it gracefully covers the case when the pref has been changed to false after
> the settings frame was sent.. and also if the server pushed data before it
> recvd our settings frame (which is impossible right now, but will certainly
> happen in the future). we should actually send a new settings when the pref
> changes, probably file a followup for it - but we would still need this
> check as it would be racy.

Argh, poor review comment diff selection. My comment was intended towards the check about kFlag_END_PUSH_PROMISE, not AllowPush. I agree that AllowPush is a valid check to have :)

Comment 25

[Patrick McManus [:mcmanus]]

(In reply to Nicholas Hurley [:hurley] from comment #24)

> Argh, poor review comment diff selection. My comment was intended towards
> the check about kFlag_END_PUSH_PROMISE, 

gotcha :)

that continued-push code isn't complete.. see there is no logic like the if (!endheaders) {queue; return;} logic like is used to process the pull header blocks. so the check is necessary.

can you file a http2 specific followup and tag it with spdy in the whiteboard?

Comment 26

[Patrick McManus [:mcmanus]]

> 
> again with the explicit > check instead of std::min. also...
>

iirc there was some platform with a typing problem involving the 64bit argument to min


> ::: netwerk/protocol/http/Http2Stream.h
> @@ +9,5 @@
> > +#include "mozilla/Attributes.h"
> > +#include "nsAHttpTransaction.h"
> > +#include "nsISupportsPriority.h"
> > +
> > +namespace mozilla { namespace net {
> 
> Should these be on separate lines?

I call it a local custom :) (they drive me nuts separated)

Comment 27

[Patrick McManus [:mcmanus]]

Attachment: rc1

my review followups for patch 0005 from comments 17 and 21

Comment 28

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #26)
> > > +namespace mozilla { namespace net {
> > 
> > Should these be on separate lines?
> 
> I call it a local custom :) (they drive me nuts separated)

Fair enough, though we have other http/2 files with them on separate lines. We should at least be consistent, whichever one we use.

Comment 29

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #23)
> http-04 says "Given that only between 0-7 bits of the
>    EOS symbol is included in any huffman-encoded string, and given that
>    the EOS symbol is at least 8 bits long, it is expected that it should
>    never be successfully decoded." .. so I'll put this in my review list of
> followups for you to do as we land -08

Indeed, it does, though behavior was never specified on the off-chance it was decoded, so I think we're compliant with -08 here (and -09, iirc, though I may have messed that bit up). Either way, it's a simple fix if we want it for -08.

Comment 30

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #25)
> can you file a http2 specific followup and tag it with spdy in the
> whiteboard?

bug 958712

Comment 31

[Patrick McManus [:mcmanus]]

Comment on attachment 8348844 [details] [diff] [review]
0005-http2-draft-08-patch.r2

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

oh man - this is really great. I can almost taste it. Its much nicer reading code that already has some experience (and therefore iterations of cleanups) in it.

if you file a patch with followups then I think we can land everything except the alpn bits (preffed off). the maybe we can get -09 in early next week. hopefully bitrot will be acceptable - I un-rotted it on 12/15 or so and most of that is vacation.

::: netwerk/protocol/http/Http2Compression.cpp
@@ +13,5 @@
> +
> +namespace mozilla {
> +namespace net {
> +
> +static nsDeque *gStaticHeaders = nullptr;

given that this is a global pointer, we should assert the socket thread when we create and destroy it

@@ +265,5 @@
> +    mAlternateReferenceSet[i] = mAlternateReferenceSet[i] + 1;
> +  }
> +}
> +
> +Http2Decompressor::Http2Decompressor()

move this to .h I guess

@@ +348,5 @@
> +
> +  while (chainBit) {
> +    // really big offsets are just trawling for overflows
> +    if (accum >= 0x800000)
> +      return NS_ERROR_ILLEGAL_VALUE;

let's add a NS_WARNING here..

@@ +351,5 @@
> +    if (accum >= 0x800000)
> +      return NS_ERROR_ILLEGAL_VALUE;
> +
> +    if (mOffset >= mDataLen)
> +      return NS_ERROR_ILLEGAL_VALUE;

also NS_WARNING here

@@ +373,5 @@
> +      name.Equals(NS_LITERAL_CSTRING("transfer-encoding")) ||
> +      name.Equals(NS_LITERAL_CSTRING("upgrade")) ||
> +      name.Equals(("accept-encoding"))) {
> +    nsCString toLog(name);
> +    LOG3(("HTTP Decompressor illegal respones header found : %s",

typo

@@ +390,5 @@
> +      return NS_ERROR_ILLEGAL_VALUE;
> +    }
> +  }
> +
> +  // Look for CR OR LF in value - could be smuggling Sec 10.3

section references is stale.. just remove it

@@ +420,5 @@
> +  }
> +
> +  // http/2 transport level headers shouldn't be gatewayed into http/1
> +  if(*(name.BeginReading()) == ':') {
> +    return NS_OK;

log

@@ +497,5 @@
> +
> +  // This is a character!
> +  if (table->mEntries[idx].mValue == 256) {
> +    // EOS
> +    foundEOS = true;

not only is this an error, we can remove all the foundeos function parameters and checks, right?

@@ +558,5 @@
> +
> +  if (table->mEntries[idx].mPtr) {
> +    if (bytesConsumed >= mDataLen) {
> +      if (!bitsLeft || (bytesConsumed > mDataLen)) {
> +        // No info left in input to try to consume, we're done

logs in all the huffman code err cases.. they're all we've got when we try to troubleshoot :)

@@ +573,5 @@
> +                                  bitsLeft, foundEOS);
> +  }
> +
> +  if (table->mEntries[idx].mValue == 256) {
> +    foundEOS = true;

note this

@@ +589,5 @@
> +  if (bitsLeft >= 8) {
> +    mOffset--;
> +    bytesConsumed--;
> +    bitsLeft -= 8;
> +  }

MOZ_ASSERT(bitsLeft < 8) ?

@@ +622,5 @@
> +    }
> +  }
> +
> +  if (bytesRead > bytes) {
> +    return NS_ERROR_ILLEGAL_VALUE;

log

@@ +643,5 @@
> +    // they make sense (ie, are all ones)
> +    uint8_t mask = (1 << bitsLeft) - 1;
> +    uint8_t bits = mData[mOffset - 1] & mask;
> +    if (bits != mask) {
> +      return NS_ERROR_ILLEGAL_VALUE;

log

@@ +648,5 @@
> +    }
> +  }
> +
> +  val = buf;
> +  return NS_OK;

maybe a succesful log?

@@ +694,5 @@
> +    return NS_OK;
> +  }
> +
> +  rv = OutputHeader(index);
> +  if (index >= mHeaderTable.VariableLength()) { // 3.2.1

any references to sections of draft specs should be removed if you happen to notice them.. in retrospect that doesn't make any sense before rfc status (too easy to get out of sync)

@@ +826,5 @@
> +
> +/////////////////////////////////////////////////////////////////
> +
> +Http2Compressor::Http2Compressor()
> +  : mParsedContentLength(-1)

relegate this to the header too

@@ +1095,5 @@
> +Http2Compressor::MakeRoom(uint32_t amount)
> +{
> +  // make room in the header table
> +  uint32_t removedCount = 0;
> +  while (mHeaderTable.VariableLength() && ((mHeaderTable.ByteCount() + amount) > mMaxBuffer)) { // 3.2.4

there's another one! sorry :)

@@ +1262,5 @@
> +}
> +
> +void
> +Http2Compressor::SetMaxBufferSize(uint32_t maxBufferSize)
> +{

log that this is called

@@ +1266,5 @@
> +{
> +  uint32_t removedCount = 0;
> +
> +  while (mHeaderTable.VariableLength() && (mHeaderTable.ByteCount() > maxBufferSize)) {
> +    mHeaderTable.RemoveElement();

log that this is being forced along with the sizes involved

::: netwerk/protocol/http/Http2Compression.h
@@ +6,5 @@
> +#ifndef mozilla_net_Http2Compression_Internal_h
> +#define mozilla_net_Http2Compression_Internal_h
> +
> +// HPACK
> +// tools.ietf.org/html/draft-ietf-httpbis-header-compression-02

this is -04

::: netwerk/protocol/http/Http2HuffmanIncoming.h
@@ +1,3 @@
> +/*
> + * THIS FILE IS AUTO-GENERATED. DO NOT EDIT!
> + */

reminder to include the scripts in the review followup patch

::: netwerk/protocol/http/Http2Session.h
@@ +19,5 @@
> +#include "Http2Compression.h"
> +
> +class nsISocketTransport;
> +
> +namespace mozilla { namespace net {

put on 2 lines if you like

::: netwerk/protocol/http/Http2Stream.h
@@ +9,5 @@
> +#include "mozilla/Attributes.h"
> +#include "nsAHttpTransaction.h"
> +#include "nsISupportsPriority.h"
> +
> +namespace mozilla { namespace net {

put them on 2 lines if you like

Comment 32

[Patrick McManus [:mcmanus]]

nick, fyi, when you get your 0005 followup done I'll take the whole patchset and rebase it and get a full try run before landing (08 preffed off).. I'll also land 951119

Comment 33

[u408661]

Attachment: hurley_rc1.patch

And here we have it. Lots more logging and assertions ahoy. Also removed all the foundEOS bits, which I'm 99.9% certain won't have any interop consequences. It's that 0.1% that's the killer, though :)

Comment 34

[Daniel Stenberg [:bagder]]

Just one question: where those NS_WARNING() calls are used, would it help if they also logged which the invalid value was that triggered them? Just thinking it can be useful in debugging...

Comment 35

[Patrick McManus [:mcmanus]]

Comment on attachment 8358640 [details] [diff] [review]
hurley_rc1.patch

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

::: netwerk/protocol/http/Http2Compression.cpp
@@ +20,5 @@
>  
>  void
>  Http2CompressionCleanup() 
>  {
> +  MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);

my bad - the cleanup actually happens on the main thread after the socket thread has been destroyed.. the impt assert is the lazy initializer - so I'lljust keep that one

Comment 36

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=f506a35dadc4
https://tbpl.mozilla.org/?tree=Try&rev=51a1f28c63a5

Comment 37

[Patrick McManus [:mcmanus]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/088897ae34f2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b35dc4d2e1b9

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/088897ae34f2
https://hg.mozilla.org/mozilla-central/rev/b35dc4d2e1b9

Comment 39

[(not currently active) Ted Mielczarek]

Just a note, we noticed you landed a node binary in testing/xpcshell, and this patch didn't get review from a build or testing peer. I think you should have run it by one or the other. I don't understand why you didn't simply go with the same mechanism that you used for bug 719609, especially considering bug 729392 has already installed node onto the test slaves?

Comment 40

[(not currently active) Ted Mielczarek]

Also, the runxpcshelltests.py changes definitely should have had review from a testing peer.

Comment 41

[u408661]

Ted - apologies for missing the testing review, it was an oversight on my part. Please feel free to suggest any changes that should be made, I'll get them fixed.

The separate node binary was done in the interest of expediency (node-http2 requires at least one bugfix and some features that aren't in the version of node on the test slaves last I checked - which reminds me, I need to file a bug to get a newer version of node on infrastructure). The binary (and the special-case logic in runxpcshelltests.py to handle it) will be removed once infrastructure is upgraded.

Comment 42

[Gregory Szorc [:gps]]

In addition, we're likely failing to meet licensing requirements. There's no licensing info for the binary and no source code. I'm pretty sure at least one part of the node.js distribution (v8, etc) requires these. Furthermore, downstream consumers don't like just binaries in the tree because they can't be verified nor trusted. Granted, we don't ship this binary. But still.

As build module owner, this patch would have been r-'d by me.

This commit should not have landed.

Comment 43

[u408661]

Ted, gps, let's take this over to bug 964563

Comment 44

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

need to file a secreview bug for this one