Retry request if pushed stream has been cancelled. |
||||||
Issue descriptionHttpStreamFactoryImpl::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.
,
Dec 1 2017
,
Dec 1 2017
,
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
,
Jan 2 2018
,
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
,
Jan 3 2018
,
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.
,
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
,
Jan 8 2018
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by b...@chromium.org
, Oct 19 2017