New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 14
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug

Blocking:
issue 554220



Sign in to add a comment

Add histogram for HTTP/2 push header validation

Project Member Reported by b...@chromium.org, Apr 11

Issue description

After  issue 554220  is completed, exactly one of the following can happen to any stream pushed on an HTTP/2 connection:
* the pushed stream is rejected when received (for example, because of invalid scheme or method);
* the pushed stream is rejected when a request is generated because the pushed response headers have not been received yet;
* the pushed stream is rejected when the pushed response headers are received (for example, because the response is 206 Partial Range);
* the pushed stream is rejected when a request is generated because the pushed response headers do not match the request headers (for example, because of Vary header);
* the pushed stream is matched with a request;
* the pushed stream is never matched with a request and is reset after a timeout.

A histogram is needed with these buckets that is recorded exactly once for every pushed stream.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e7023f8c2d97b4e7c0a109659dfb7e3ed4961ae3

commit e7023f8c2d97b4e7c0a109659dfb7e3ed4961ae3
Author: Bence Béky <bnc@chromium.org>
Date: Mon May 14 01:53:03 2018

Clean up some HTTP/2 push related SpdyNetworkTransactionTests.

Remove SpdyNetworkTransactionTest.ServerPushBeforeHeaders, because this
case is covered by SpdyNetworkTransactionPushTest.*/0.

Remove SpdyNetworkTransactionTest.ServerPushInvalidCrossOrigin, because
this case is covered by
SpdyNetworkTransactionTest.ServerPushCrossOriginCorrectness.

Change ServerPushCrossOriginCorrectness into a parametrized subclass,
just like SpdyNetworkTransactionPushTest.  For clarity, rename
SpdyNetworkTransactionPushTest to SpdyNetworkTransactionPushHeaderTest,
ServerPushCrossOriginCorrectness to SpdyNetworkTransactionPushUrlTest,
and use PushHeaderTestParams and PushHeaderUrlParams as test data struct
name, respectively.

This will make it much easier to add tests for an upcoming histogram.

Bug:  831536 
Change-Id: I298fbd9a2dd4b6567b8cb597fcb704b956819218
Reviewed-on: https://chromium-review.googlesource.com/1055751
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558173}
[modify] https://crrev.com/e7023f8c2d97b4e7c0a109659dfb7e3ed4961ae3/net/spdy/spdy_network_transaction_unittest.cc

Project Member

Comment 2 by bugdroid1@chromium.org, May 14

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6d5075c4f0103c938c3fbdbe279c40a3efdde3b6

commit 6d5075c4f0103c938c3fbdbe279c40a3efdde3b6
Author: Bence Béky <bnc@chromium.org>
Date: Mon May 14 10:07:52 2018

Do not handle impossible cases in SpdySession.

Clean up some corner cases concerning HTTP/2 pushed streams.  I plan to
add a histogram that records every possible outcome of receiving a
pushed stream, and it is cleaner if impossible cases are not handled
with an early return, but they are simply documented by a DCHECK.

Note that GetPromisedUrlFromHeaders() guarantees a lot of things about
the pushed request: if the scheme is not http or https, or if the method
is not GET or HEAD, then it returns an invalid URL.  Therefore there is
no need to handle these cases explicitly in SpdySession.  There is test
coverage in SpdyNetworkTransactionTest that enforces this.

Bug:  831536 
Change-Id: Ic3dae56b026598fa2965c8eee0db02cfbc95eb06
Reviewed-on: https://chromium-review.googlesource.com/1055760
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558244}
[modify] https://crrev.com/6d5075c4f0103c938c3fbdbe279c40a3efdde3b6/net/spdy/spdy_session.cc

Status: Fixed (was: Started)
Project Member

Comment 5 by bugdroid1@chromium.org, May 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/40e9c2237ae1a0d338ba596bf4c661f07bbc8080

commit 40e9c2237ae1a0d338ba596bf4c661f07bbc8080
Author: Bence Béky <bnc@chromium.org>
Date: Thu May 24 20:21:20 2018

Do not expand UMA_HISTOGRAM_ENUMERATION in 19 places.

Instead of expanding UMA_HISTOGRAM_ENUMERATION macro in 19 places, use a
single static method to log this histogram.  The motivation for this
change is to avoid binary bloat.

This is a follow-up to https://crrev.com/c/1055949.

This CL results in 2384 byte decrease in binary size for locally
compiled release build on Linux, and no change for debug build.

Bug:  831536 
Change-Id: I0b5e741e29529c9f6ae22336b3a6c3d194042249
Reviewed-on: https://chromium-review.googlesource.com/1071731
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561607}
[modify] https://crrev.com/40e9c2237ae1a0d338ba596bf4c661f07bbc8080/net/spdy/spdy_http_stream.cc
[modify] https://crrev.com/40e9c2237ae1a0d338ba596bf4c661f07bbc8080/net/spdy/spdy_session.cc
[modify] https://crrev.com/40e9c2237ae1a0d338ba596bf4c661f07bbc8080/net/spdy/spdy_session.h

Sign in to add a comment