New issue
Advanced search Search tips
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 2
Type: Bug

Blocked on:
issue 791054
issue 791055

Blocking:
issue 554220
issue 798508



Sign in to add a comment

Retry request if pushed stream has been cancelled.

Project Member Reported by b...@chromium.org, Oct 19 2017

Issue description

HttpStreamFactoryImpl::Job calls Http2PushPromiseIndex::Find() to look for an HTTP/2 connection (SpdySession instance) with a matching pushed stream.  Find() can possibly return a connection with a SpdySessionKey and IP address that do not match the request, because cross-origin pushes are allowed.

HttpStreamFactoryIml::Job creates a SpdyHttpStream instance with this SpdySession, then PostTasks OnStreamReadyCallback, which calls Delegate::OnStreamReady(), then the delegate can call Job::ReleaseStream(), and call SpdyHttpStream::InitializeStream(), which calls SpdySession::GetPushStream().  However, since there is a PostTask in this sequence, it is possible that by this time the server cancels the pushed stream.

Currently, at this point, SpdySession dispatches the request.  However, since we are talking about a connection that is outside the normal pooling rules, this potentially allows a server to push a stream to hijack a request, which is undesirable.  Instead, the request should fail.
 

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

Blocking: 554220

Comment 2 by b...@chromium.org, Dec 1 2017

Blockedon: 791054

Comment 3 by b...@chromium.org, Dec 1 2017

Blockedon: 791055
Project Member

Comment 4 by bugdroid1@chromium.org, Dec 6 2017

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

commit a12c770e918a69cc0bbba2d2579c22ff6ae76936
Author: Bence Béky <bnc@chromium.org>
Date: Wed Dec 06 21:07:28 2017

Record stream IDs in Http2PushPromiseIndex.

Stop using SpdySession instances in Http2PushPromiseIndexTests.  This
makes tests much simpler.

Rename SpdySessionKey::Equals() to SpdySessionKey::operator==() so that
it can be tested by gMock.  This conforms to the style guide "prefer to
define == [...] rather than Equals()", see
https://google.github.io/styleguide/cppguide.html#Operator_Overloading

Define struct UnclaimedPushedStream instead of using std::pair or
std::tuple.  Named members are much nicer than first, second, or get<>.

Define CompareByUrl in conformance with the same section of the style
guide "if your type doesn't have a natural ordering [...] use a custom
comparator rather than overloading <".

The next CL will merge SpdySession::UnclaimedPushedStreamContainer into
Http2PushPromiseIndex.  For this, two sets of UnclaimedPushedStream
entries will need to be maintained with identical
contents but different ordering.  The other will be done by
CompareByDelegate and will allow efficient lookup of entries for a given
Delegate.

Bug:  776415 ,  791054 
Change-Id: I0c1912e9932b102e17528f9319163232860a8813
Reviewed-on: https://chromium-review.googlesource.com/809187
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522192}
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/http2_push_promise_index_test.cc
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/spdy_session.h
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/spdy_session_key.cc
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/spdy_session_key.h
[modify] https://crrev.com/a12c770e918a69cc0bbba2d2579c22ff6ae76936/net/spdy/chromium/spdy_session_pool.cc

Comment 5 by b...@chromium.org, Jan 2 2018

Blocking: 798508
Project Member

Comment 6 by bugdroid1@chromium.org, Jan 3 2018

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

commit e8955cf2f5c6603111eb39b34990f26bc053e911
Author: Bence Béky <bnc@chromium.org>
Date: Wed Jan 03 16:43:23 2018

Make HttpStreamFactoryImpl::Job claim pushed streams.

Motivation:

Suppose the server pushes a stream and then cancels it.  Consider the
following three events:
(A) HttpStreamFactoryImpl::Job calls Http2PushPromiseIndex::Find() to
    look for the SpdySession that has an unclaimed pushed stream for the
    request URL.
(B) SpdyHttpStream calls GetPushStream() on that SpdySession instance.
(C) SpdySession deletes the pushed stream when it receives the
    RST_STREAM frame from the server, before the response is pushed.

(B) always happens after (A), and it involves a PostTask (see bug for
details).  If (C) happens after (B), then the request fails with a
user-visible error (that upper layers like HttpNetworkTransaction might
intercept and do a retry).

If (C) happens after (A) but before (B), then currently, the request is
sent out on the SpdySession which might not be used if there was not a
pushed stream on it.  That is, the server is allowed to hijack the
request, and see the request headers, by pushing a stream and then
closing it at the right time.  After this CL, the request will fail in
this case as well, with a new error code.

Details:

Rename Http2PushPromiseIndex::FindSession() to ClaimPushedStream() and
make it unregister the pushed stream if found.  Record pushed stream ID
in HttpStreamFactoryImpl::Job, pass it on to SpdyHttpStream, then back
to SpdySession::GetPushStream().  Fail request with new error code
ERR_SPDY_PUSHED_STREAM_NOT_AVAILABLE if pushed stream was cancelled in
the meanwhile.

Add new SpdySessionTest for modified GetPushStream() behavior.

Add SpdyNetworkTransactionTest to cover the case that the server cancels
the pushed stream after it is claimed but before SpdyHttpStream calls
SpdySession->GetPushStream().

Bug:  776415 
Change-Id: I14150b933b09ee5c96722aa0fb2392afbcbb65b1
Reviewed-on: https://chromium-review.googlesource.com/829993
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526717}
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/base/net_error_list.h
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/http/http_stream_factory_impl_job.h
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/spdy/chromium/http2_push_promise_index_test.cc
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/spdy/chromium/spdy_http_stream.cc
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/spdy/chromium/spdy_http_stream.h
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/spdy/chromium/spdy_http_stream_unittest.cc
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/spdy/chromium/spdy_session.h
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/spdy/chromium/spdy_session_pool.cc
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/spdy/chromium/spdy_session_unittest.cc
[modify] https://crrev.com/e8955cf2f5c6603111eb39b34990f26bc053e911/net/spdy/chromium/spdy_stream_unittest.cc

Comment 7 by b...@chromium.org, Jan 3 2018

Status: Fixed (was: Started)

Comment 8 Deleted

Comment 9 by b...@chromium.org, Jan 4 2018

In fact, similiarily to  issue 798508 , such a request can be safely retried.

The difference is that  issue 798508  relates to the case when a stream is reset after GetPushedStream() is called, whereas this issue relates to the case when a stream is reset before GetPushedStream() is called.  In both cases the stream is already claimed by HttpStreamFactoryImpl::Job calling Http2PushPromiseIndex::ClaimPushedStream(), and server reset is received after this call.
Project Member

Comment 10 by bugdroid1@chromium.org, Jan 5 2018

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

commit 1a5d85630fc04d4af11406bbccc3ef9ab7abb548
Author: Bence Béky <bnc@chromium.org>
Date: Fri Jan 05 20:48:02 2018

Retry request if server resets pushed stream before GetPushedStream call.

This is similar to the retry logic implemented at
https://crrev.com/c/848776, see  https://crbug.com/776415#c9 .

Bug:  776415 
Change-Id: Ifdd25e4b7694ced987bfe3013ce825d408b75040
Reviewed-on: https://chromium-review.googlesource.com/851655
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527375}
[modify] https://crrev.com/1a5d85630fc04d4af11406bbccc3ef9ab7abb548/net/http/http_network_transaction.cc
[modify] https://crrev.com/1a5d85630fc04d4af11406bbccc3ef9ab7abb548/net/spdy/chromium/spdy_network_transaction_unittest.cc

Comment 11 by b...@chromium.org, Jan 8 2018

Status: Fixed (was: Started)

Sign in to add a comment