New issue
Advanced search Search tips
Starred by 11 users
Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 776415

Blocking:
issue 750107



Sign in to add a comment
Implement correct request matching for pushed responses
Project Member Reported by tombergan@chromium.org, Nov 10 2015 Back to list
When matching a request to pushed responses, Chrome needs to validate all relevant headers to ensure the pushed response is a valid match for the request. In particular, if the response contains a non-empty Vary header, the varied headers in the request should match those in the response's PUSH_PROMISE.

For further discussion, see here:
https://groups.google.com/a/chromium.org/forum/#!topic/net-dev/kuPNwmKZWzg
 
Cc: ckrasic@chromium.org
Comment 2 by b...@chromium.org, Apr 15 2016
Status: Available
Cc: tombergan@chromium.org
Cc: zhongyi@chromium.org
Comment 5 by ckrasic@google.com, Oct 5 2016
QUIC implementation of server push respects the Vary header.
Comment 6 by y...@yoav.ws, Nov 29 2016
Cc: y...@yoav.ws
In addition to Vary, a PUSH_PROMISE can in theory be a Range request that gets a pushed 206 response. Chrome needs to support this as well, at minimum by matching the Range header of the request with the Range header from the PUSH_PROMISE.

ckrasic@ -- From looking at the code (linked below), it looks like QUIC supports pushed responses with Vary but not pushed 206 responses?
https://cs.chromium.org/chromium/src/net/quic/core/quic_client_promised_info.cc?rcl=d25bb92682c7fb325d1f2b5608832a195fb0dd27&l=80
tombergan@ - correct, QUIC's push support does not consider range requests.
Cc: bol...@chromium.org
 Issue 727655  has been merged into this issue.
Cc: b...@chromium.org
+bnc@chromium.org
Project Member Comment 13 by bugdroid1@chromium.org, Jul 26
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/602c40ce2fb52c32518c074ebe56c2795d60a4bd

commit 602c40ce2fb52c32518c074ebe56c2795d60a4bd
Author: Bence Béky <bnc@chromium.org>
Date: Wed Jul 26 05:22:56 2017

Small cleanup in HTTP/2 and QUIC server push code.

* Remove unused QuicChromiumClientSession::GetStreamIdForPush().
* Remove unused SpdySession::GetStreamIdForPush().
* Add early returns in SpdySession::GetPushStream().
* Minor cleanup in SpdySession::TryCreatePushStream().
* Better error messages; change tests accordingly.

No functional change except for more informative error messages
(both in LOG() and in GOAWAY frames).

BUG=554220

Change-Id: If7d67c33d291a9dd2bd5b3721cf65aa17a7134d9
Reviewed-on: https://chromium-review.googlesource.com/584009
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#489557}
[modify] https://crrev.com/602c40ce2fb52c32518c074ebe56c2795d60a4bd/net/quic/chromium/quic_chromium_client_session.cc
[modify] https://crrev.com/602c40ce2fb52c32518c074ebe56c2795d60a4bd/net/quic/chromium/quic_chromium_client_session.h
[modify] https://crrev.com/602c40ce2fb52c32518c074ebe56c2795d60a4bd/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/602c40ce2fb52c32518c074ebe56c2795d60a4bd/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/602c40ce2fb52c32518c074ebe56c2795d60a4bd/net/spdy/chromium/spdy_session.h

Blocking: 750107
rdsmith@ pointed out that if in the vast majority of cases (~99%) the pushed response headers are available at the time when the request is matched up with the pushed stream, then it might not be worth the code complexity to do async magic for request matching: instead, in the few cases when response headers are not available, the pushed stream can be ignored.  (It is quite possible that this is the case, since the server or reverse proxy might have the response cached, so the response headers might even be in the same TCP packet as the PUSH_PROMISE.)  I'm therefore adding a histogram to measure this.
Re: comment #15.  I don't buy it.  

AFAIK, the common case is that server triggers the generation of pushes based on headers (preload, or older x-associated-content).   For each pushed resource, it has to initiate the corresponding request to backend.  So this races with PUSH_PROMISE and HEADERS that refer to the push on the way to client.  Client would be expected to process headers ASAP, which should detect the preload/associated headers, causing the corresponding requests to be generated. 

For the case that response headers are available, it has to be that server initiating response to backend and subsequently forwarding response to client is faster than client reading header and preload generating a request to rendezvous with the pushed response.  The former involves network communication, the latter is all local to the client, albeit with some IPCs within Chrome.   

I would think the case where client wins the race *should* be the common one.
Project Member Comment 17 by bugdroid1@chromium.org, Aug 28
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ae145e73456bf7520da710f62078508bd30faa15

commit ae145e73456bf7520da710f62078508bd30faa15
Author: Bence Béky <bnc@chromium.org>
Date: Mon Aug 28 20:02:13 2017

Measure if response headers are available when matching pushed stream.

Add Net.PushedStreamAlreadyHasResponseHeaders histogram to measure
if response headers are available when matching a request to a pushed
stream.

BUG=554220

Change-Id: I94902e59f4f7c1624c3a3ed421412a39bd16275a
Reviewed-on: https://chromium-review.googlesource.com/635811
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497856}
[modify] https://crrev.com/ae145e73456bf7520da710f62078508bd30faa15/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/ae145e73456bf7520da710f62078508bd30faa15/tools/metrics/histograms/histograms.xml

Re: comment #16.  I find your argument plausible, and would not be surprised if you were right.  A histogram just landed to measure this, so we should find out soon.
FYI data so far show that 78% of claimed pushed streams have already received response headers by the time of rendezvous.  This is not high enough that we could afford dropping the remaining 22% of pushed streams, so I'll implement the async matching.
Project Member Comment 20 by bugdroid1@chromium.org, Oct 2
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/824e493803f18649da0e7961bfa6d027e8e2700d

commit 824e493803f18649da0e7961bfa6d027e8e2700d
Author: Bence Béky <bnc@chromium.org>
Date: Mon Oct 02 16:51:54 2017

Create Http2PushPromiseIndex class.

Factor out SpdySessionPool::unclaimed_pushed_streams_ into its own
class, similar to QuicClientPushPromiseIndex.  This class will handle
async push promise validation after some future CLs.

No functional change.

Bug: 554220
Change-Id: If42d077c6d258fc419b822db7511623904629aae
Reviewed-on: https://chromium-review.googlesource.com/692935
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505647}
[modify] https://crrev.com/824e493803f18649da0e7961bfa6d027e8e2700d/net/BUILD.gn
[add] https://crrev.com/824e493803f18649da0e7961bfa6d027e8e2700d/net/spdy/chromium/http2_push_promise_index.cc
[add] https://crrev.com/824e493803f18649da0e7961bfa6d027e8e2700d/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/824e493803f18649da0e7961bfa6d027e8e2700d/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/824e493803f18649da0e7961bfa6d027e8e2700d/net/spdy/chromium/spdy_session_pool.cc
[modify] https://crrev.com/824e493803f18649da0e7961bfa6d027e8e2700d/net/spdy/chromium/spdy_session_pool.h

Project Member Comment 21 by bugdroid1@chromium.org, Oct 3
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4c000bb7ae539258d4b95dff16d540dfa0b9d383

commit 4c000bb7ae539258d4b95dff16d540dfa0b9d383
Author: Bence Béky <bnc@chromium.org>
Date: Tue Oct 03 15:04:28 2017

Do not check for pushed streams in every FindAvailableSession() call.

This CL removes the call to Http2PushPromiseIndex::Find() from
SpdySessionPool::FindAvailableSession() body, and calls it directly at
some but not all of the call sites.  The motivation is that it will be
complicated to handle async returns of Http2PushPromiseIndex::Find()
once implemented, so I'm removing unnecessary calls as a preparation.

HttpStreamFactoryImpl::Job calls FindAvailableSession() up to four
times:
* at the beginning,
* upon IP resolution (in a callback passed to InitSocketHandle*()),
* if the call in the callback returns a SpdySession, then once again to
  find out what that SpdySession is (because the callback drops the
  return value),
* one last time after connection establishment, just in case another
  HTTP/2 connection has been created in the meanwhile, in which case
  the new socket is closed.

However, it is not necessary to check for pushed streams upon IP
resolution (either in the callback or afterwards, if the callback finds
a SpdySession): in case a server pushes a matching resource in the
meanwhile, the last call (after connection establishment) will catch
that anyway.  Note that HTTP/2 servers SHOULD send the PUSH_PROMISE
frame before sending other frames containing references to the resource
(like response data on the original stream), therefore this case will be
reasonably rare anyway.

(The primary reason for the FindAvailableSession() call upon IP
resolution is to check if the newly established IP address allows for
IP-based pooling, in which case connection establishment is skipped.)

Bug: 554220
Change-Id: I6c6b744d21150ad94c5d78b7fccbc64002d5e19b
Reviewed-on: https://chromium-review.googlesource.com/695210
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506039}
[modify] https://crrev.com/4c000bb7ae539258d4b95dff16d540dfa0b9d383/net/http/http_proxy_client_socket_wrapper.cc
[modify] https://crrev.com/4c000bb7ae539258d4b95dff16d540dfa0b9d383/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/4c000bb7ae539258d4b95dff16d540dfa0b9d383/net/http/http_stream_factory_impl_job.h
[modify] https://crrev.com/4c000bb7ae539258d4b95dff16d540dfa0b9d383/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/4c000bb7ae539258d4b95dff16d540dfa0b9d383/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/4c000bb7ae539258d4b95dff16d540dfa0b9d383/net/spdy/chromium/spdy_session_pool.cc
[modify] https://crrev.com/4c000bb7ae539258d4b95dff16d540dfa0b9d383/net/spdy/chromium/spdy_session_pool.h
[modify] https://crrev.com/4c000bb7ae539258d4b95dff16d540dfa0b9d383/net/spdy/chromium/spdy_session_pool_unittest.cc
[modify] https://crrev.com/4c000bb7ae539258d4b95dff16d540dfa0b9d383/net/spdy/chromium/spdy_session_unittest.cc
[modify] https://crrev.com/4c000bb7ae539258d4b95dff16d540dfa0b9d383/net/spdy/chromium/spdy_test_util_common.cc

Project Member Comment 22 by bugdroid1@chromium.org, Oct 5
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c2a82bdd433d93b1b0b80bf2a95f8aae5f1429ef

commit c2a82bdd433d93b1b0b80bf2a95f8aae5f1429ef
Author: Bence Béky <bnc@chromium.org>
Date: Thu Oct 05 13:03:32 2017

Add Http2PushPromiseIndexTest.

Bug: 554220
Change-Id: Id4652e6c43de606dacfb4a32d01d0cb3e057a226
Reviewed-on: https://chromium-review.googlesource.com/700403
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506710}
[modify] https://crrev.com/c2a82bdd433d93b1b0b80bf2a95f8aae5f1429ef/net/BUILD.gn
[add] https://crrev.com/c2a82bdd433d93b1b0b80bf2a95f8aae5f1429ef/net/spdy/chromium/http2_push_promise_index_test.cc

Project Member Comment 23 by bugdroid1@chromium.org, Oct 6
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d914b9cf95668ba61d5e69c90bb3b53c35979805

commit d914b9cf95668ba61d5e69c90bb3b53c35979805
Author: Bence Béky <bnc@chromium.org>
Date: Fri Oct 06 16:24:43 2017

Move pushed url collision detection to UnclaimedPushedStreamContainer.

Move pushed url collision detection from
SpdySession::TryCreatePushStream() to
UnclaimedPushedStreamContainer::insert().  This is a refactoring with
little functional change (request method is now checked before URL
collision, not after).

My plan is to only allow at most one pushed stream per URL across all
SpdySessions, and this CL is in preparation for that change.

Bug: 554220
Change-Id: Ib4db703c6a7b267aceeee0e77ea694d048017cc8
Reviewed-on: https://chromium-review.googlesource.com/701839
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#507090}
[modify] https://crrev.com/d914b9cf95668ba61d5e69c90bb3b53c35979805/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/d914b9cf95668ba61d5e69c90bb3b53c35979805/net/spdy/chromium/spdy_session.h

Buck: does QUIC allow multiple simultaneous pushes across different connections with the same URL?  (Obviously at least one of them have to be cross-origin for this to happen.)  I'd like to disallow it for HTTP/2 (by rejecting the second and every subsequent push for a given URL) and would be curious what QUIC does in this case.
They are disallowed in QUIC.

There is a single index (instance of |QuicClientPushPromiseIndex|) owned by the |QuicStreamFactory|, that is shared across all QUIC connections (specifically in |QuicSpdyClientSessionBase|).   |QuicSpdyClientSessionBase::HandlePromised()| contains logic that checks the index by URL and potentially rejects a new promise with |QUIC_DUPLICATE_PROMISE_URL|.
Project Member Comment 26 by bugdroid1@chromium.org, Oct 11
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7ecaada1ccdb7619aab8fc39a743233aa84bd660

commit 7ecaada1ccdb7619aab8fc39a743233aa84bd660
Author: Bence Béky <bnc@chromium.org>
Date: Wed Oct 11 22:46:16 2017

Cleanup in Http2PushPromiseIndex.

Cleanup like comments, early return, change argument from value to const
ref etc. from https://crrev.com/c/706930 which was abandoned.

Bug: 554220
Change-Id: I8462bdf329c601cb01bd8b7c5ac15dd61ba02ae3
Reviewed-on: https://chromium-review.googlesource.com/713857
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508149}
[modify] https://crrev.com/7ecaada1ccdb7619aab8fc39a743233aa84bd660/net/proxy/proxy_server.h
[modify] https://crrev.com/7ecaada1ccdb7619aab8fc39a743233aa84bd660/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/7ecaada1ccdb7619aab8fc39a743233aa84bd660/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/7ecaada1ccdb7619aab8fc39a743233aa84bd660/net/spdy/chromium/spdy_network_transaction_unittest.cc

Blockedon: 776415
Project Member Comment 28 by bugdroid1@chromium.org, Oct 19
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/762844b44ff2d169fab4bb907fc1853af7688c5c

commit 762844b44ff2d169fab4bb907fc1853af7688c5c
Author: Bence Béky <bnc@chromium.org>
Date: Thu Oct 19 23:50:35 2017

Misc cleanup.

Random small stuff I came across while working on push validation.

Bug: 554220
Change-Id: I58d43860b2f347b5df2833868ad8f906513e8f40
Reviewed-on: https://chromium-review.googlesource.com/728361
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510253}
[modify] https://crrev.com/762844b44ff2d169fab4bb907fc1853af7688c5c/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/762844b44ff2d169fab4bb907fc1853af7688c5c/net/spdy/chromium/spdy_http_stream.cc
[modify] https://crrev.com/762844b44ff2d169fab4bb907fc1853af7688c5c/net/spdy/chromium/spdy_session.h

Project Member Comment 29 by bugdroid1@chromium.org, Oct 31
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/736b413cfbb7f1cc5f380e80b710e2a47ec1284c

commit 736b413cfbb7f1cc5f380e80b710e2a47ec1284c
Author: Bence Béky <bnc@chromium.org>
Date: Tue Oct 31 14:11:33 2017

Remove lazy deletion of WeakPtrs in Http2PushPromiseIndex.

SpdySession closes all active streams before destruction, as documented
by DCHECK(active_streams_.empty()) in DcheckDraining() which is called
in the destructor.  This guarantees that UnclaimedPushedStreamContainer
will be empty by this time (because every entry in that container must
be an active stream), also that there will never be invalidated weak
pointers in Http2PushPromiseIndex (because every entry in that container
must also be in unclaimed_pushed_streams_ of the corresponding
SpdySession).  Therefore lazy deletion in Http2PushPromiseIndex never
happens.

Bug: 554220
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I4bb8c094af8212adcf39e8731698b562f184453e
Reviewed-on: https://chromium-review.googlesource.com/744506
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512822}
[modify] https://crrev.com/736b413cfbb7f1cc5f380e80b710e2a47ec1284c/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/736b413cfbb7f1cc5f380e80b710e2a47ec1284c/net/spdy/chromium/http2_push_promise_index.h

Project Member Comment 30 by bugdroid1@chromium.org, Nov 2
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a205d78c7125749d8352356114beaf7602ef4387

commit a205d78c7125749d8352356114beaf7602ef4387
Author: Bence Béky <bnc@chromium.org>
Date: Thu Nov 02 10:20:26 2017

Revert "Remove lazy deletion of WeakPtrs in Http2PushPromiseIndex."

This reverts commit 736b413cfbb7f1cc5f380e80b710e2a47ec1284c.

Reason for revert: Sorry, I was mistaken: there can be invalid weak pointers in Http2PushPromiseIndex.  I'll address that in a follow-up CL.

Original change's description:
> Remove lazy deletion of WeakPtrs in Http2PushPromiseIndex.
> 
> SpdySession closes all active streams before destruction, as documented
> by DCHECK(active_streams_.empty()) in DcheckDraining() which is called
> in the destructor.  This guarantees that UnclaimedPushedStreamContainer
> will be empty by this time (because every entry in that container must
> be an active stream), also that there will never be invalidated weak
> pointers in Http2PushPromiseIndex (because every entry in that container
> must also be in unclaimed_pushed_streams_ of the corresponding
> SpdySession).  Therefore lazy deletion in Http2PushPromiseIndex never
> happens.
> 
> Bug: 554220
> Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
> Change-Id: I4bb8c094af8212adcf39e8731698b562f184453e
> Reviewed-on: https://chromium-review.googlesource.com/744506
> Reviewed-by: Helen Li <xunjieli@chromium.org>
> Commit-Queue: Bence Béky <bnc@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#512822}

TBR=bnc@chromium.org,xunjieli@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 554220
Change-Id: I1dca915a2a2ed5b4c46b04d409eae91ca2b95cd9
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Reviewed-on: https://chromium-review.googlesource.com/749421
Reviewed-by: Bence Béky <bnc@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513450}
[modify] https://crrev.com/a205d78c7125749d8352356114beaf7602ef4387/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/a205d78c7125749d8352356114beaf7602ef4387/net/spdy/chromium/http2_push_promise_index.h

Project Member Comment 31 by bugdroid1@chromium.org, Nov 3
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6cbe3aa6a04443a342eb9500134e9327eac62ee0

commit 6cbe3aa6a04443a342eb9500134e9327eac62ee0
Author: Bence Béky <bnc@chromium.org>
Date: Fri Nov 03 14:29:07 2017

Make sure to remove unclaimed pushed streams from Http2PushPromiseIndex.

UnclaimedPushedStreamContainer has two erase() methods: one by key,
the other by iterator.  Call the latter from the former to make sure
entries are removed from Http2PushPromiseIndex.

Do not add pushed streams to Http2PushPromiseIndex if they are not added
to SpdySession's local unclaimed_pushed_stream_contrainer_ because there
is already a pushed stream on the same SpdySession for the same URL.

Add DCHECK in Http2PushPromiseIndex destructor to test that container is
empty.  This is expected, as Http2PushPromiseIndex is owned by
SpdySessionPool, which destroys all SpdySessions in its destructor
(before its members are destroyed).

https://crrev.com/c/744506 was incorrect and I reverted it.  This CL
re-lands that change but this time with some added paranoia and the
necessary fixes to make sure there are no invalid weak pointers in
Http2PushPromiseIndex.

Bug: 554220
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I6e902f41686c6f43eda4ecc90c9979677a5b3f27
Reviewed-on: https://chromium-review.googlesource.com/749902
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513766}
[modify] https://crrev.com/6cbe3aa6a04443a342eb9500134e9327eac62ee0/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/6cbe3aa6a04443a342eb9500134e9327eac62ee0/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/6cbe3aa6a04443a342eb9500134e9327eac62ee0/net/spdy/chromium/spdy_session.cc

Project Member Comment 32 by bugdroid1@chromium.org, Nov 3
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b68c9c0cfad50496077d3adff283106215a8eaa1

commit b68c9c0cfad50496077d3adff283106215a8eaa1
Author: Bence Béky <bnc@chromium.org>
Date: Fri Nov 03 22:04:11 2017

Minor refactor in Http2PushPromiseIndex and SpdySession.

* Make the method Http2PushPromiseIndex::Find() const.
* Simplify a DCHECK in Http2PushPromiseIndex::Unregister...().
* Tweak comments.
* Make UnclaimedPushedStreamContainer a private member class of
  SpdySession.

Bug: 554220
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I52c76340087e84d1713e965b0ee2dc2be0bc9146
Reviewed-on: https://chromium-review.googlesource.com/753704
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513925}
[modify] https://crrev.com/b68c9c0cfad50496077d3adff283106215a8eaa1/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/b68c9c0cfad50496077d3adff283106215a8eaa1/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/b68c9c0cfad50496077d3adff283106215a8eaa1/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/b68c9c0cfad50496077d3adff283106215a8eaa1/net/spdy/chromium/spdy_session.h

Project Member Comment 33 by bugdroid1@chromium.org, Nov 13 (5 days ago)
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2292183b34418c0b7522ebae6ce26b223acd95c2

commit 2292183b34418c0b7522ebae6ce26b223acd95c2
Author: Bence Béky <bnc@chromium.org>
Date: Mon Nov 13 23:26:52 2017

Add a histogram for Vary response header in pushed HTTP/2 streams.

This is to assess if pushed responses with Vary header could be rejected
(if proportion is very-very low).  See public discussion at
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/5_aP_stqndw.

Also add unittests.

Bug: 554220
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I8c5c95fc12da056fed7f837c79419e969f991a35
Reviewed-on: https://chromium-review.googlesource.com/755093
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516094}
[modify] https://crrev.com/2292183b34418c0b7522ebae6ce26b223acd95c2/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/2292183b34418c0b7522ebae6ce26b223acd95c2/net/spdy/chromium/spdy_session.h
[modify] https://crrev.com/2292183b34418c0b7522ebae6ce26b223acd95c2/net/spdy/chromium/spdy_session_unittest.cc
[modify] https://crrev.com/2292183b34418c0b7522ebae6ce26b223acd95c2/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/2292183b34418c0b7522ebae6ce26b223acd95c2/tools/metrics/histograms/histograms.xml

Sign in to add a comment