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

Issue 799937 link

Starred by 0 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug



Sign in to add a comment

Do not call SpdySession::GetPushedStream() from SpdyHttpStream::InitializeStream() if pushed_stream_id_ == kNoPushedStreamFound.

Project Member Reported by b...@chromium.org, Jan 8 2018

Issue description

Http2PushPromiseIndex::ClaimPushedStream() is called earlier and it looks for matching pushed streams.  It seems silly to search in the index again from SpdyHttpStream::InitializeStream() if no pushed stream was found the first time.  Sure enough this will miss pushed streams that have been created between the two calls (there is a PostTask there), but that race condition does not justify the extra complexity.

http resources pushed over a trusted HTTP/2 proxy need to be sorted out though, because currently they are not handled by Http2PushPromiseIndex::ClaimPushedStream().

This is a follow-up to Helen's comment at https://chromium-review.googlesource.com/c/chromium/src/+/829993/2/net/spdy/chromium/spdy_http_stream.cc#85.
 
Project Member

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

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

commit 0337eff3083677c42ee1578fe9f4c9fd14db071c
Author: Bence Béky <bnc@chromium.org>
Date: Mon Jan 08 19:10:35 2018

Simplify logic for http push over HTTP/2 connection to trusted proxy.

This change only affects HTTP/2 pushed resources with http scheme, which
is only allowed on a trusted proxy.  There can only be at most one
trusted proxy, and thus there can only be at most one SpdySession to a
trusted proxy.  Cross-origin push of http resources on this connection
is already allowed, see SpdySession::TryCreatePushStream().  Currently
Http2PushPromiseIndex::ClaimPushedStream() does not return this
SpdySession if the scheme is http, but
SpdySessionPool::FindAvailableSession() finds this SpdySession anyway
(based on the proxy, because if an HTTP/2 proxy is used,
HttpStreamFactoryImpl::Job::GetSpdySessionKey() sets
SpdySessionKey.host_port_pair to the proxy server), therefore
SpdySession::GetPushedStream() finds the pushed stream anyway.

This change makes Http2PushPromiseIndex::ClaimPushedStream() return the
SpdySessionKey and SpdyStreamId corresponding to the matching pushed
stream for a request with http scheme.  Modifications to
SpdySession::ValidatePushedStream() make sure that the proxy server and
privacy mode still matches, so matching logic is identical to before.

This CL will make it possible for a subsequent CL to remove the
SpdySession::GetPushedStream() call from
SpdyHttpStream::InitializeStream() unless pushed_stream_id_ is different
from kNoPushedStreamFound.

This change is not exactly a no-op: in the weird edge case when stream
is pushed first, then Http2PushPromiseIndex::ClaimPushedStream() is
called, then the server resets the pushed stream, then it pushes a new one
for the same URL, finally GetPushedStream() is called.  Before this CL,
the request would bind to the pushed stream, after this CL, it fails
with ERR_SPDY_PUSHED_STREAM_NOT_AVAILABLE and is automatically retried.
But for non-pathological cases, this CL should mean no change.

Bug:  799937 
Change-Id: I6906b6892b73a210dfe6bb7c1f3081435adc6463
Reviewed-on: https://chromium-review.googlesource.com/854020
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527698}
[modify] https://crrev.com/0337eff3083677c42ee1578fe9f4c9fd14db071c/net/spdy/chromium/http2_push_promise_index.cc
[modify] https://crrev.com/0337eff3083677c42ee1578fe9f4c9fd14db071c/net/spdy/chromium/http2_push_promise_index.h
[modify] https://crrev.com/0337eff3083677c42ee1578fe9f4c9fd14db071c/net/spdy/chromium/http2_push_promise_index_test.cc
[modify] https://crrev.com/0337eff3083677c42ee1578fe9f4c9fd14db071c/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/0337eff3083677c42ee1578fe9f4c9fd14db071c/net/spdy/chromium/spdy_session.h

Project Member

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

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

commit 7bf9436874ee25176216edebd0335484759c8e39
Author: Bence Béky <bnc@chromium.org>
Date: Wed Jan 10 13:19:36 2018

Do not call GetPushedStream if there is no pushed stream.

Do not call SpdySession::GetPushedStream() from
SpdyHttpStream::InitializeStream() if
|pushed_stream_id_ == kNoPushedStreamFound|.  Pushed streams are claimed
by Http2PushPromiseIndex::ClaimPushedStream(), there is no need to do an
extra lookup in the index if a pushed stream was not found the first
time.

This is a follow-up to
https://chromium-review.googlesource.com/c/chromium/src/+/829993/2/net/spdy/chromium/spdy_http_stream.cc#85.

Note that it is possible that a stream was created after the
Http2PushPromiseIndex::ClaimPushedStream() call but before
SpdySession::GetPushedStream() is called (they do not happen
synchronously).  In two unittests, mock read data has to be reordered to
make sure pushed stream is claimed.
Http2PushPromiseIndex::ClaimPushedStream() calls are added to some other
unittests to make sure the stream is not in the index by the time
SpdySession::GetPushedStream() is called.

Bug:  799937 
Change-Id: If91fe95fd65362a9c650b502402796deb251e887
Reviewed-on: https://chromium-review.googlesource.com/857278
Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#528290}
[modify] https://crrev.com/7bf9436874ee25176216edebd0335484759c8e39/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/7bf9436874ee25176216edebd0335484759c8e39/net/spdy/chromium/spdy_http_stream.cc
[modify] https://crrev.com/7bf9436874ee25176216edebd0335484759c8e39/net/spdy/chromium/spdy_http_stream_unittest.cc
[modify] https://crrev.com/7bf9436874ee25176216edebd0335484759c8e39/net/spdy/chromium/spdy_network_transaction_unittest.cc
[modify] https://crrev.com/7bf9436874ee25176216edebd0335484759c8e39/net/spdy/chromium/spdy_session.cc
[modify] https://crrev.com/7bf9436874ee25176216edebd0335484759c8e39/net/spdy/chromium/spdy_session_unittest.cc
[modify] https://crrev.com/7bf9436874ee25176216edebd0335484759c8e39/net/spdy/chromium/spdy_stream_unittest.cc

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

Status: Fixed (was: Started)

Sign in to add a comment