New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 554220 link

Starred by 11 users

Issue metadata

Status: Fixed
Owner:
Closed: May 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 776415
issue 831536

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

Issue description

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 (was: Untriaged)
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.

Comment 9 by bol...@chromium.org, May 24 2017

Cc: bol...@chromium.org
 Issue 727655  has been merged into this issue.
Cc: b...@chromium.org
+bnc@chromium.org

Comment 12 by b...@chromium.org, Jul 24 2017

Cc: -b...@chromium.org
Owner: b...@chromium.org
Status: Started (was: Available)
HTTP/2 specs: http://httpwg.org/specs/rfc7540.html#PushRequests

See also blog post at https://jakearchibald.com/2017/h2-push-tougher-than-i-thought#items-in-the-push-cache-should-be-matched-using-http-semantics-aside-from-freshness
Project Member

Comment 13 by bugdroid1@chromium.org, Jul 26 2017

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

Comment 14 by b...@chromium.org, Jul 28 2017

Blocking: 750107

Comment 15 by b...@chromium.org, Aug 24 2017

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 2017

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

Comment 18 by b...@chromium.org, Aug 28 2017

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.

Comment 19 by b...@chromium.org, Sep 18 2017

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 2017

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 2017

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 2017

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 2017

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

Comment 24 by b...@chromium.org, Oct 10 2017

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 2017

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

Comment 27 by b...@chromium.org, Oct 19 2017

Blockedon: 776415
Project Member

Comment 28 by bugdroid1@chromium.org, Oct 19 2017

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 2017

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 2017

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 2017

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 2017

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 2017

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

Project Member

Comment 34 by bugdroid1@chromium.org, Nov 29 2017

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

commit 15470d8e9e704861ee794ccb54bcda73ce0f68d1
Author: Bence Béky <bnc@chromium.org>
Date: Wed Nov 29 04:52:51 2017

Cancel expired HTTP/2 pushed streams even if no other stream is pushed.

The current method for cancelling HTTP/2 pushed streams is only executed
when a new pushed stream arrives, that is, a pushed stream can hang
around forever if the server sends no other pushed streams.  Also,
calling DeleteExpiredPushedStreams() from a method like
TryCreatePushStream() that manipulates pushed stream iterators is
potentially error-prone, see  https://crbug.com/443490 .  This CL changes
the expiration method to reset a single pushed stream if unclaimed, and
posttasks that method with a delay.

Also, increase |bytes_pushed_and_unclaimed_count_| in
CloseActiveStreamIterator() instead of in the method that closes an
expired stream, so that unclaimed pushed bytes are accounted for from a
pushed stream which is unclaimed and unexpired when SpdySession is
closed.  (There was no test coverage for this, but now
CancelPushAfterSessionGoesAway covers this, because the pushed stream is
not expired in this test any longer.)

Also add a NOTREACHED() in CancelPushedStreamIfUnclaimed(), because if
stream is in unclaimed_pushed_streams_, then it must also be in
active_streams_.

Bug:  554220 
Change-Id: Ica5b7de8d580effcec05710e90ccbcc4eb2c1889
Reviewed-on: https://chromium-review.googlesource.com/776155
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520008}
[modify] https://crrev.com/15470d8e9e704861ee794ccb54bcda73ce0f68d1/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/15470d8e9e704861ee794ccb54bcda73ce0f68d1/net/spdy/chromium/spdy_session.h
[modify] https://crrev.com/15470d8e9e704861ee794ccb54bcda73ce0f68d1/net/spdy/chromium/spdy_session_unittest.cc

Project Member

Comment 35 by bugdroid1@chromium.org, Jan 8 2018

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

commit 499b1c279c6816c2bfdaad2ad1cfd2c246ca9514
Author: Bence Béky <bnc@chromium.org>
Date: Mon Jan 08 19:10:11 2018

Only allow http and https schemes for HTTP/2 push.

Trusted proxy only pushes http scheme resources according to
the data reduction proxy team.  Cross-origin push in other
cases is only allowed for https scheme.  Even in the same origin
case, no other scheme should be accepted for pushes.

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

Project Member

Comment 36 by bugdroid1@chromium.org, Jan 11 2018

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

commit 5b1e4ce7d3be7e1239e23651abe17748e08db451
Author: Bence Béky <bnc@chromium.org>
Date: Thu Jan 11 14:55:15 2018

Accept HEAD method for HTTP/2 push.

Currently, SpdyHttpStream::Initialize() requires that method is GET for
a request to be matched to a pushed stream, but
SpdySession::TryCreatePushedStream() allows both GET and HEAD methods.

This change removes the requirement from SpdyHttpStream::Initialize(),
and changes SpdySession::ValidatePushedStream() to require that request
method matches that of the pushed stream in ValidatePushedStream().

In order to do this, HttpRequestInfo is passed through
ClaimPushedStream() to ValidatePushedStream().  This will be useful in
subsequent CLs when the pushed request and response headers will be
validated against the request headers.

Bug:  554220 
Change-Id: I37c502881a782b05e01c012dbb98352eb48fbed7
Reviewed-on: https://chromium-review.googlesource.com/860480
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528622}
[modify] https://crrev.com/5b1e4ce7d3be7e1239e23651abe17748e08db451/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/5b1e4ce7d3be7e1239e23651abe17748e08db451/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/5b1e4ce7d3be7e1239e23651abe17748e08db451/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/5b1e4ce7d3be7e1239e23651abe17748e08db451/net/spdy/chromium/http2_push_promise_index_test.cc
[modify] https://crrev.com/5b1e4ce7d3be7e1239e23651abe17748e08db451/net/spdy/chromium/spdy_http_stream.cc
[modify] https://crrev.com/5b1e4ce7d3be7e1239e23651abe17748e08db451/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/5b1e4ce7d3be7e1239e23651abe17748e08db451/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/5b1e4ce7d3be7e1239e23651abe17748e08db451/net/spdy/chromium/spdy_session.h
[modify] https://crrev.com/5b1e4ce7d3be7e1239e23651abe17748e08db451/net/spdy/chromium/spdy_session_unittest.cc
[modify] https://crrev.com/5b1e4ce7d3be7e1239e23651abe17748e08db451/net/spdy/chromium/spdy_stream.h
[modify] https://crrev.com/5b1e4ce7d3be7e1239e23651abe17748e08db451/net/spdy/chromium/spdy_stream_unittest.cc

Comment 37 by b...@chromium.org, Apr 11 2018

Re comment #19: latest data from this histogram still show that in 75% to 80% of all cases (depending on platform), push response headers are already available when request is matched.  This is consistent with earlier numbers.  (Note that this histogram was removed last November, so at this point it is only collected from old deployments.)  However, after careful consideration, it has been decided that async header matching should not be implemented at this time, because it might not be worth the complexity.

Therefore the remaining items to wrap up this issue are the following:
* reject pushed stream if response headers are not available at the time request is generated;
* reject range requests and range responses;
* match Vary header;
* add histogram (I'm filing separate issue for that).

Comment 38 by b...@chromium.org, Apr 11 2018

Blockedon: 831536
Project Member

Comment 39 by bugdroid1@chromium.org, Apr 19 2018

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

commit 04268b0269f119153d3193e9b3e40f3e48cf01c8
Author: Bence Béky <bnc@chromium.org>
Date: Thu Apr 19 01:17:41 2018

Validate Vary header for HTTP/2 pushed stream.

Validate request against Vary header for HTTP/2 pushed stream if
response headers are available at the time of request matching.

Bug:  554220 
Change-Id: I0b7404b0157f35673a7709008d71a33f9a127377
Reviewed-on: https://chromium-review.googlesource.com/1015512
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551906}
[modify] https://crrev.com/04268b0269f119153d3193e9b3e40f3e48cf01c8/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/04268b0269f119153d3193e9b3e40f3e48cf01c8/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/04268b0269f119153d3193e9b3e40f3e48cf01c8/net/spdy/chromium/spdy_stream.h

Project Member

Comment 40 by bugdroid1@chromium.org, Apr 20 2018

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

commit c4caf07da881f23ee1f1a99f93f6f59cf9f2cf0c
Author: Bence Béky <bnc@chromium.org>
Date: Fri Apr 20 22:27:30 2018

Match Range header for HTTP/2 pushed range responses.

Also write parametrized unittest to cover new functionality as well
as a number of existing test cases.

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

Project Member

Comment 41 by bugdroid1@chromium.org, May 10

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

commit 6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6
Author: Bence Béky <bnc@chromium.org>
Date: Thu May 10 11:51:25 2018

Close HTTP/2 stream with proper error code.

Currently when SpdyStream::Delegate calls SpdyStream::Cancel(), that
calls SpdySession::ResetStream(), which calls
SpdySession::ResetStreamIterator(), which calls
SpdySession::CloseActiveStreamIterator() with ERR_SPDY_PROTOCOL_ERROR,
which calls SpdySession::DeleteStream() with ERR_SPDY_PROTOCOL_ERROR,
which calls SpdyStream::OnClose() with ERR_SPDY_PROTOCOL_ERROR, which
calls SpdyStream::Delegate::OnClose() with ERR_SPDY_PROTOCOL_ERROR.

After this CL, SpdyStream::Cancel() takes an int error argument, which
is passed to SpdySession::ResetStream() then to
SpdySession::ResetStreamIterator() then to
SpdySession::CloseActiveStreamIterator() then to
SpdySession::DeleteStream() then to SpdyStream::OnClose() then to
SpdyStream::Delegate::OnClose().  This way Delegate can directly control
what error its own OnClose() method will be called with.

This will be useful if I end up moving HTTP/2 pushed header validation
to SpdyHttpStream, so that it can use a dedicated error code to signal
to HttpNetworkTransaction if the pushed stream does not match.

Bug:  554220 
Change-Id: Id6395d834395d182b7af4a45e1bb1186c1b82feb
Reviewed-on: https://chromium-review.googlesource.com/1051007
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557493}
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/base/net_error_list.h
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_http_stream.cc
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_http_stream.h
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_proxy_client_socket.cc
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_session.h
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_session_pool_unittest.cc
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_session_unittest.cc
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_stream.cc
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_stream.h
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_stream_test_util.cc
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_stream_unittest.cc
[modify] https://crrev.com/6b9c135cd9e20cabe3d2d4bc0bb7ee3fc33392a6/net/spdy/chromium/spdy_test_util_common.cc

Project Member

Comment 42 by bugdroid1@chromium.org, May 11

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

commit 49db0e2601786baa8a50ebf96db262e095abb7d3
Author: Bence Béky <bnc@chromium.org>
Date: Fri May 11 00:54:05 2018

Validate HTTP/2 push headers even if they are late.

Validate response headers of an HTTP/2 pushed stream against the client
request headers even if the response headers only arrive after the
pushed stream has been claimed by a request.  Cancel stream and retry
request in HttpNetworkTransaction if headers do not match.

Bug:  554220 
Change-Id: I5e1a42583b941faef5529b50178fe00d8c864976
Reviewed-on: https://chromium-review.googlesource.com/1053429
Reviewed-by: Ryan Hamilton <rch@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557745}
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/base/net_error_list.h
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/http/http_network_transaction.cc
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/bidirectional_stream_spdy_impl.cc
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/bidirectional_stream_spdy_impl.h
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/spdy_http_stream.cc
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/spdy_http_stream.h
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/spdy_proxy_client_socket.cc
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/spdy_proxy_client_socket.h
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/spdy_session_fuzzer.cc
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/spdy_session_pool_unittest.cc
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/spdy_stream.cc
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/spdy_stream.h
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/spdy_stream_test_util.cc
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/spdy/chromium/spdy_stream_test_util.h
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/websockets/websocket_basic_stream_adapters.cc
[modify] https://crrev.com/49db0e2601786baa8a50ebf96db262e095abb7d3/net/websockets/websocket_basic_stream_adapters.h

Status: Fixed (was: Started)

Sign in to add a comment