Refactor RST_STREAM status code handling.

Refactor RST_STREAM status code handling. Also handle SPDY4/HTTP2 status codes.

This lands server change 63409085 by hkhalil.

Fix an off-by-one DCHECK for frame lengths equal to the frame limit.

This lands server change 63285101 by raullenchai.
BUG=345769

[Johnny, 2014-04-21 23:17:23]

Ryan for SPDY review / Jim for histograms.xml.

Thanks

[jar (doing other things), 2014-04-22 00:02:59]

https://codereview.chromium.org/243643002/diff/1/net/spdy/spdy_protocol.h
File net/spdy/spdy_protocol.h (right):

https://codereview.chromium.org/243643002/diff/1/net/spdy/spdy_protocol.h#new...
net/spdy/spdy_protocol.h:373: RST_STREAM_FRAME_SIZE_ERROR = 11,  // Equivalent
to FRAME_TOO_LARGE
nit: If you're not going to distigiush them... why not just use FRAME_SIZE_ERROR
everywhere, and get rid of TOO_LARGE?

https://codereview.chromium.org/243643002/diff/1/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/243643002/diff/1/net/spdy/spdy_session.h#newc...
net/spdy/spdy_session.h:101: STATUS_CODE_ENHANCE_YOUR_CALM = 34,
What is this name meant to mean??  The others seem much more apparent.

[Johnny, 2014-04-22 00:31:27]

https://codereview.chromium.org/243643002/diff/1/net/spdy/spdy_protocol.h
File net/spdy/spdy_protocol.h (right):

https://codereview.chromium.org/243643002/diff/1/net/spdy/spdy_protocol.h#new...
net/spdy/spdy_protocol.h:373: RST_STREAM_FRAME_SIZE_ERROR = 11,  // Equivalent
to FRAME_TOO_LARGE
On 2014/04/22 00:02:59, jar wrote:
> nit: If you're not going to distigiush them... why not just use
FRAME_SIZE_ERROR
> everywhere, and get rid of TOO_LARGE?

Yea, this is not optimal, but: FRAME_TOO_LARGE is a specific error name in the
SPDY/3.1 specification. FRAME_SIZE_ERROR is a specific error name in the HTTP/2
specification.

We're supporting both specifications with this codebase; their meanings are so
semantically close that Hasan elected to map them to the same internal error in
this CL.

https://codereview.chromium.org/243643002/diff/1/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/243643002/diff/1/net/spdy/spdy_session.h#newc...
net/spdy/spdy_session.h:101: STATUS_CODE_ENHANCE_YOUR_CALM = 34,
On 2014/04/22 00:02:59, jar wrote:
> What is this name meant to mean??  The others seem much more apparent.

This is specification authors having fun :) See
http://http2.github.io/http2-spec/#ErrorCodes

ENHANCE_YOUR_CALM: "The endpoint detected that its peer is exhibiting a behavior
over a given amount of time that has caused it to refuse to process further
frames"

Also mapped (by some implementations) to HTTP status code 420:
http://httpstatus.es/420

[Ryan Hamilton, 2014-04-22 03:34:07]

lgtm

[jar (doing other things), 2014-04-25 16:03:44]

https://codereview.chromium.org/243643002/diff/1/net/spdy/spdy_protocol.h
File net/spdy/spdy_protocol.h (right):

https://codereview.chromium.org/243643002/diff/1/net/spdy/spdy_protocol.h#new...
net/spdy/spdy_protocol.h:373: RST_STREAM_FRAME_SIZE_ERROR = 11,  // Equivalent
to FRAME_TOO_LARGE
On 2014/04/22 00:31:27, Johnny wrote:
> On 2014/04/22 00:02:59, jar wrote:
> > nit: If you're not going to distigiush them... why not just use
> FRAME_SIZE_ERROR
> > everywhere, and get rid of TOO_LARGE?
> 
> Yea, this is not optimal, but: FRAME_TOO_LARGE is a specific error name in the
> SPDY/3.1 specification. FRAME_SIZE_ERROR is a specific error name in the
HTTP/2
> specification.
> 
> We're supporting both specifications with this codebase; their meanings are so
> semantically close that Hasan elected to map them to the same internal error
in
> this CL.

OK... good enough.  Might be worth a comment... but your choice.

https://codereview.chromium.org/243643002/diff/1/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/243643002/diff/1/net/spdy/spdy_session.h#newc...
net/spdy/spdy_session.h:101: STATUS_CODE_ENHANCE_YOUR_CALM = 34,
On 2014/04/22 00:31:27, Johnny wrote:
> On 2014/04/22 00:02:59, jar wrote:
> > What is this name meant to mean??  The others seem much more apparent.
> 
> This is specification authors having fun :) See
> http://http2.github.io/http2-spec/#ErrorCodes
> 
> ENHANCE_YOUR_CALM: "The endpoint detected that its peer is exhibiting a
behavior
> over a given amount of time that has caused it to refuse to process further
> frames"
> 
> Also mapped (by some implementations) to HTTP status code 420:
> http://httpstatus.es/420

IMO, you should map this to a meaningful name, and only comment that the "having
fun authors" use this meaningless term in the spec.  You would IMO do the same
thing if the spec authors named it with a meaningless hex sequence, etc.  We
need meaningful names for variables and enums.... and this does not seem to
reach that bar.

[Johnny, 2014-04-25 17:37:53]

> OK... good enough.  Might be worth a comment... but your choice.

I've expanded the comment, to tie the names to their specification versions.


> IMO, you should map this to a meaningful name, and only comment that the
"having
> fun authors" use this meaningless term in the spec.  You would IMO do the same
> thing if the spec authors named it with a meaningless hex sequence, etc.  We
> need meaningful names for variables and enums.... and this does not seem to
> reach that bar.

I'm not sure what a meaningful, concise name for this error would be. It's an
extremely broad error condition, and I don't think I (or anybody) currently know
what the uses of it will be in the future (clearly DOS, but also in response to
suspected malicious constructions or attacks). Given that, I honestly think the
current name is as good as any other.

My own conviction is also that the specification is ground truth, and this is
just an implementation of that specification. Precision to the spec terminology
& naming is important, as readers will generally have the spec open alongside
when learning the code (I know I did).

I'm also opposed to a rename here because this CL merges an internal change, and
deviations from the internal repo dramatically increase the burden of future
merges.

[jar (doing other things), 2014-04-26 20:30:31]

On 2014/04/25 17:37:53, Johnny wrote:
> > OK... good enough.  Might be worth a comment... but your choice.
> 
> I've expanded the comment, to tie the names to their specification versions.
> 
> 
> > IMO, you should map this to a meaningful name, and only comment that the
> "having
> > fun authors" use this meaningless term in the spec.  You would IMO do the
same
> > thing if the spec authors named it with a meaningless hex sequence, etc.  We
> > need meaningful names for variables and enums.... and this does not seem to
> > reach that bar.
> 
> I'm not sure what a meaningful, concise name for this error would be. It's an
> extremely broad error condition, and I don't think I (or anybody) currently
know
> what the uses of it will be in the future (clearly DOS, but also in response
to
> suspected malicious constructions or attacks). Given that, I honestly think
the
> current name is as good as any other.
> 
> My own conviction is also that the specification is ground truth, and this is
> just an implementation of that specification. Precision to the spec
terminology
> & naming is important, as readers will generally have the spec open alongside
> when learning the code (I know I did).
> 
> I'm also opposed to a rename here because this CL merges an internal change,
and
> deviations from the internal repo dramatically increase the burden of future
> merges.

The Twitter reference suggests "... when the client is being rate limited.".  I
assume this means some packets/frames will be discarded, but it is unclear.

Your quote from the spec suggests it means the far side will "...refuse to
process further frames."  That sounds like the spec calls for an absolute
black-hole, for good.

I don't mind referencing this spec name, but perhaps one of the following names
would be more readable:
"FRAMES_WILL_BE_DISCARDED"
or
"ALL_FRAMES_WILL_BE_DISCARDED"

It would probably be good to upstream such a more readable change into our
internal codebase, so that readability, and merges, would be improved and
simplified.

I'd suggest we adopt a name which corresponds to when we (in our server code?
client code?) send this error, and exactly what we do.

[Johnny, 2014-04-28 13:58:54]

> The Twitter reference suggests "... when the client is being rate limited.". 
I
> assume this means some packets/frames will be discarded, but it is unclear.
> 
> Your quote from the spec suggests it means the far side will "...refuse to
> process further frames."  That sounds like the spec calls for an absolute
> black-hole, for good.
> 
> I don't mind referencing this spec name, but perhaps one of the following
names
> would be more readable:
> "FRAMES_WILL_BE_DISCARDED"
> or
> "ALL_FRAMES_WILL_BE_DISCARDED"
> 
> It would probably be good to upstream such a more readable change into our
> internal codebase, so that readability, and merges, would be improved and
> simplified.
> 
> I'd suggest we adopt a name which corresponds to when we (in our server code?
> client code?) send this error, and exactly what we do.

Why don't we take this onto a thread? I'm happy to start one with other
stakeholders in the upstream code to discuss. There may be resistance, but I
have no dog in this fight beyond the position I've stated.

I do feel this review is not the place to have this out. Even if it's changed in
the upstream, we'll still want this CL to land as-is.

[Ryan Hamilton, 2014-04-28 15:19:07]

On 2014/04/28 13:58:54, Johnny wrote:
> Why don't we take this onto a thread? I'm happy to start one with other
> stakeholders in the upstream code to discuss. There may be resistance, but I
> have no dog in this fight beyond the position I've stated.
> 
> I do feel this review is not the place to have this out. Even if it's changed
in
> the upstream, we'll still want this CL to land as-is.

Totally agree.

[jar (doing other things), 2014-04-28 19:07:56]

Per offline discussion.. LGTM

I'm hopeful that we'll never use this oddly named value, and its presence in the
code is only to replicate the spec's mention of it.

[Johnny, 2014-04-28 19:16:30]

Thanks. Would you please also stamp https://codereview.chromium.org/246073007/,
which includes this CL?