[SPDY] Close SPDY sessions on session flow control errors

[SPDY] Close SPDY sessions on session flow control errors

But just emit something to NetLog for errors in a SETTINGS
frame.

Treat the receive window going negative as a valid error for SpdyStream.

Remove net:: prefix from spdy_{session,stream}.*.

Update some enums in histograms.xml.

BUG=176592
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195309

[akalin, 2013-04-19 03:05:19]

+rch for review

[Ryan Hamilton, 2013-04-19 03:59:19]

lgtm

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

https://codereview.chromium.org/14348012/diff/1/net/spdy/spdy_session.cc#newc...
net/spdy/spdy_session.cc:2305: // TODO(akalin): Also send a GOAWAY frame.
perhaps this todo should move to close session on error?

[akalin, 2013-04-19 05:55:14]

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

https://codereview.chromium.org/14348012/diff/1/net/spdy/spdy_session.cc#newc...
net/spdy/spdy_session.cc:2305: // TODO(akalin): Also send a GOAWAY frame.
On 2013/04/19 03:59:19, Ryan Hamilton wrote:
> perhaps this todo should move to close session on error?

Done. Also everywhere else.

[akalin, 2013-04-19 05:57:21]

+jar for histograms

[jar (doing other things), 2013-04-19 06:16:10]

https://codereview.chromium.org/14348012/diff/5001/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/14348012/diff/5001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:5074: <!-- SpdyFramer::SpdyErrors -->
nit: I *think* these comments will be removed during the pretty-print run, which
will also alphabetize etc.

https://codereview.chromium.org/14348012/diff/5001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:5089: <int value="11" label="Protocol
Error"/>
Usually, when the enums change, best practice is to define a new enum, and at
most, obsolete the old enums (and perhasp the histograms that use the old
enums).

...unless of course the enums were already/always wrong.

If you just change the enums, then all the old histogram data will appear
corrupt and confused :-(.  Since there are generally lots of recent/older
versions sending in UMA data, this tends to lead to a lot of confusion.

[akalin, 2013-04-19 08:46:23]

PTAL

https://codereview.chromium.org/14348012/diff/5001/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/14348012/diff/5001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:5074: <!-- SpdyFramer::SpdyErrors -->
On 2013/04/19 06:16:11, jar wrote:
> nit: I *think* these comments will be removed during the pretty-print run,
which
> will also alphabetize etc.

Nope, they were kept.

https://codereview.chromium.org/14348012/diff/5001/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:5089: <int value="11" label="Protocol
Error"/>
On 2013/04/19 06:16:11, jar wrote:
> Usually, when the enums change, best practice is to define a new enum, and at
> most, obsolete the old enums (and perhasp the histograms that use the old
> enums).
> 
> ...unless of course the enums were already/always wrong.
> 
> If you just change the enums, then all the old histogram data will appear
> corrupt and confused :-(.  Since there are generally lots of recent/older
> versions sending in UMA data, this tends to lead to a lot of confusion.

Done. Looks like I inadvertently changed the enums in a previous CL. :( So I
defined a new enum and new histograms and added various comments.

[akalin, 2013-04-19 20:42:51]

ping!

On 2013/04/19 08:46:23, akalin wrote:
> PTAL
> 
>
https://codereview.chromium.org/14348012/diff/5001/tools/metrics/histograms/h...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/14348012/diff/5001/tools/metrics/histograms/h...
> tools/metrics/histograms/histograms.xml:5074: <!-- SpdyFramer::SpdyErrors -->
> On 2013/04/19 06:16:11, jar wrote:
> > nit: I *think* these comments will be removed during the pretty-print run,
> which
> > will also alphabetize etc.
> 
> Nope, they were kept.
> 
>
https://codereview.chromium.org/14348012/diff/5001/tools/metrics/histograms/h...
> tools/metrics/histograms/histograms.xml:5089: <int value="11" label="Protocol
> Error"/>
> On 2013/04/19 06:16:11, jar wrote:
> > Usually, when the enums change, best practice is to define a new enum, and
at
> > most, obsolete the old enums (and perhasp the histograms that use the old
> > enums).
> > 
> > ...unless of course the enums were already/always wrong.
> > 
> > If you just change the enums, then all the old histogram data will appear
> > corrupt and confused :-(.  Since there are generally lots of recent/older
> > versions sending in UMA data, this tends to lead to a lot of confusion.
> 
> Done. Looks like I inadvertently changed the enums in a previous CL. :( So I
> defined a new enum and new histograms and added various comments.

[jar (doing other things), 2013-04-19 21:26:19]

See nit below.... then LGTM

https://codereview.chromium.org/14348012/diff/6007/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/14348012/diff/6007/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:1962: SpdySessionErrorDetails2 on
2013-04-19.
If you use the <obsolete> tag in these, then the won't clutter the dashboard
(unless you click the box to get the obsolete stuff).

[akalin, 2013-04-19 21:34:31]

https://codereview.chromium.org/14348012/diff/6007/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/14348012/diff/6007/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:1962: SpdySessionErrorDetails2 on
2013-04-19.
On 2013/04/19 21:26:19, jar wrote:
> If you use the <obsolete> tag in these, then the won't clutter the dashboard
> (unless you click the box to get the obsolete stuff).

Done.

[commit-bot: I haz the power, 2013-04-19 21:34:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/14348012/15001

[commit-bot: I haz the power, 2013-04-19 22:16:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/14348012/26001

[akalin, 2013-04-19 22:16:52]

Per conversation with rch@, removing TODOs re. sending a GOAWAY frame, as it's
not really the right use case for GOAWAY.

[akalin, 2013-04-19 22:18:31]

Committed patchset #5 manually as r195309 (presubmit successful).

[jar (doing other things), 2013-04-19 23:12:37]

https://codereview.chromium.org/14348012/diff/6007/tools/metrics/histograms/h...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/14348012/diff/6007/tools/metrics/histograms/h...
tools/metrics/histograms/histograms.xml:5102: the values below for Chrome builds
after that revision. -->
<ugh>  No one will notice the comment when looking at the bogus data.

Perhaps you should edit the labels below.  Maybe add the word "conflated" to
each label <sigh>.  

Maybe you should add a "warning" to the histograms that use this enum. That
might be easier!  Up front, in all caps, in addition to <obsolete>ing them.

https://codereview.chromium.org/14348012/diff/26001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/14348012/diff/26001/net/spdy/spdy_session.cc#...
net/spdy/spdy_session.cc:2258: base::IntToString(delta_window_size) +
nit: Probably indent this 4 more than the arguments above, to show it is a line
wrap of a single arg.

https://codereview.chromium.org/14348012/diff/26001/net/spdy/spdy_session.cc#...
net/spdy/spdy_session.cc:2340: " in DecreaseRecvWindowSize, which is larger than
the receive " +
nit: indent 4 more?

same on line 1859

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

https://codereview.chromium.org/14348012/diff/26001/net/spdy/spdy_session.h#n...
net/spdy/spdy_session.h:64: // in histograms.xml.
Please add warning: DO NOT INSERT ENUMS INTO THIS LIST! Add only to the end.

https://codereview.chromium.org/14348012/diff/26001/net/spdy/spdy_session.h#n...
net/spdy/spdy_session.h:92: // SpdySession errors
nit: I'd put this at the end of line 93, to discourage folks from inserting more
enums on blank lines above (delete the blank line spacing).

https://codereview.chromium.org/14348012/diff/26001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/14348012/diff/26001/net/spdy/spdy_stream.cc#n...
net/spdy/spdy_stream.cc:382: " in DecreaseRecvWindowSize, which is larger than
the receive " +
nit: indent 4 more