New issue
Advanced search Search tips

Issue 706974 link

Starred by 1 user
Status: Fixed
Owner:
b...@chromium.org
Closed: Jun 2017
Cc:
mmenke@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocked on:
issue 475060
Blocking:
issue 615413



Sign in to add a comment

HttpStreamFactoryImpl::Request can be completed twice when HTTP/2 is used

Project Member Reported by xunji...@chromium.org, Mar 30 2017 
Steps to repro:
1. Make two stream requests to the same H2 server.
2. DCHECK(!completed_) in HttpStreamFactoryImpl::Request::Complete() is triggered.

When a new SpdySession is established, Job::OnNewSpdySessionReadyCallback calls JobController::OnNewSpdySessionReady(). JobController::OnNewSpdySessionReady() marks the Request as complete via MarkRequestComplete(). The controller then calls Factory::OnNewSpdSessonReady() which iterates through all Requests and mark them as complete again.

Comment 1 by xunji...@chromium.org, Mar 30 2017 

Looks like RemoveRequestFromSpdySessionRequestMap() is not getting called. Once the DCHECK is removed, we will enter an infinite loop in  HttpStreamFactoryImpl::OnNewSpdySessionReady()...

Comment 2 by xunji...@chromium.org, Mar 31 2017 

RemoveRequestFromSpdySessionRequestMap() is called all over the place. It's hard to make sense of the flow. BindJob() will indirectly invokes RemoveRequestFromSpdySessionRequestMap() as well. I find it rather unintuitive. 

void HttpStreamFactoryImpl::JobController::OnNewSpdySessionReady() {
  ...
  // The first case is the usual case.
    if (!job_bound_) {
      BindJob(job);  <-- This calls RemoveRequestFromSpdySessionRequestMap()!
    }

    MarkRequestComplete(was_alpn_negotiated, negotiated_protocol, using_spdy);
  ...

 // Notify |factory_|. |request_| and |bounded_job_| might be deleted already.
  if (spdy_session && spdy_session->IsAvailable()) {
    factory->OnNewSpdySessionReady(spdy_session, direct, used_ssl_config,
                                   used_proxy_info, was_alpn_negotiated,
                                   negotiated_protocol, using_spdy,
                                   source_dependency);
  }
}

Comment 3 by xunji...@chromium.org, Apr 12 2017 

I tried to fix this in https://codereview.chromium.org/2784143003/ but I gave up because it was harder than I thought.

Comment 4 by b...@chromium.org, Apr 12 2017

Blockedon: 475060

Comment 5 by b...@chromium.org, Apr 13 2017

Blocking: 615413
Here's my proposal for tackling this bug:

1. Disable HTTP/2 Alt-Svc for the time being.  HTTP/2 Alt-Svc headers are extremely-extremely rare in the wild.
2. See if SpdySessionKey can be made constant for a given Job after proxy logic is moved from Job to JobController (that refactoring is part of issue 475060).
3. Make alternative Job have a SpdySessionKey that is identical to that of the main Job.
4. Re-enable HTTP/2 Alt-Svc if the hostname is same as that of the origin.

After that, one can enable remote HTTP/2 Alt-Svc, which is issue 615413 (and has dependencies other than this).

Comment 7 by xunji...@chromium.org, Apr 19 2017

Owner: ----
Status: Available (was: Assigned)

Comment 8 by b...@chromium.org, Apr 20 2017

Owner: b...@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 9 by bugdroid1@chromium.org, May 18 2017 

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

commit 1fc76912bc055188dc76c09f6032096c079d4d86
Author: bnc <bnc@chromium.org>
Date: Thu May 18 20:43:24 2017

Fix SpdySessionKey for HTTP/2 alternative Jobs.

Change HttpStreamFactoryImpl::Job::GetSpdySessionKey() to construct a
SpdySessionKey from the request URL, not the destination.  This is the
correct behavior, because the only layers that should worry about the
destination are the socket pool and below.

This does not change behavior in production, because
HostPortPair::FromURL(origin_url_) can only differ from |destination_|
for alternative jobs, and HTTP/2 alternative jobs are disabled in
https://crrev.com/2821463002.

BUG=615413,  706974 

Review-Url: https://codereview.chromium.org/2887773006
Cr-Commit-Position: refs/heads/master@{#472919}

[modify] https://crrev.com/1fc76912bc055188dc76c09f6032096c079d4d86/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/1fc76912bc055188dc76c09f6032096c079d4d86/net/http/http_stream_factory_impl_job_controller_unittest.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Jun 7 2017 

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

commit 253a62d89689b5a61846a38d48037d0ea719f905
Author: xunjieli <xunjieli@chromium.org>
Date: Wed Jun 07 16:25:20 2017

Make SpdySessionKey a const member of HttpStreamFactoryImpl::Job

SpdySessionKey creation depends on proxy resolution result. Since proxy
resolution is now done before Job is created, this allows us to make
SpdySessionKey a const member of HttpStreamFactoryImpl::Job.

BUG=475060,  706974 ,  718576 

Review-Url: https://codereview.chromium.org/2925093002
Cr-Commit-Position: refs/heads/master@{#477674}

[modify] https://crrev.com/253a62d89689b5a61846a38d48037d0ea719f905/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/253a62d89689b5a61846a38d48037d0ea719f905/net/http/http_stream_factory_impl_job.h
[modify] https://crrev.com/253a62d89689b5a61846a38d48037d0ea719f905/net/http/http_stream_factory_impl_job_controller_unittest.cc
Project Member

Comment 12 by bugdroid1@chromium.org, Jun 12 2017 

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

commit dcdaa5f6458c89fee3f6e2a0913707463c05171e
Author: bnc <bnc@chromium.org>
Date: Mon Jun 12 19:02:46 2017

Remove Request from Map in OnNewSpdySessionReady().

SpdySessionPool::OnNewSpdySessionReady() calls
HttpStreamFactoryImpl::Request::Complete() repeatedly while there are
any Requests in the map.  This relies on
HttpStreamRequest::Delegate::OnStreamReady() destroying the Request,
which in its destructor calls
HttpStreamFactoryImpl::JobController::OnRequestComplete(), which calls
HttpStreamFactoryImpl::JobController::CancelJobs(), which calls
HttpStreamFactoryImpl::JobController::RemoveRequestFromSpdySessionRequestMap(),
which calls SpdySessionPool::RemoveRequestFromSpdySessionRequestMap().
However, if HttpStreamRequest::Delegate::OnStreamReady() does not
destroy the Request, that results in DCHECK(!completed_) triggering in
HttpStreamFactoryImpl::Request::Complete() in a debug build, and an
infinite loop in production build.

This CL fixes this by explicitly calling
RemoveRequestFromSpdySessionRequestMap() in
SpdySessionPool::OnNewSpdySessionReady().

This revives https://crrev.com/2784143003, which was abandoned to do all
the necessary cleanup first.  Kudos to xunjieli@ for paving the way.

BUG= 706974 

Review-Url: https://codereview.chromium.org/2930323002
Cr-Commit-Position: refs/heads/master@{#478704}

[modify] https://crrev.com/dcdaa5f6458c89fee3f6e2a0913707463c05171e/net/http/http_stream_factory_impl_unittest.cc
[modify] https://crrev.com/dcdaa5f6458c89fee3f6e2a0913707463c05171e/net/spdy/chromium/spdy_session_pool.cc

Comment 13 by b...@chromium.org, Jun 12 2017

Status: Fixed (was: Assigned)

Comment 14 by xunji...@chromium.org, Jun 13 2017

Status: Assigned (was: Fixed)
There's one more case that I ran into this morning where a request can be completed twice.
Say three requests with the same SpdySessionKey are doing socket connects at the same time, the first one calls SpdySessionPool::OnNewSpdySessionReady() which mark the second and third as complete. If we do not delete the second and third Request objects, when their socket connects completes, JobController::OnStreamReady will mark the requests as complete again.

The following test will trigger the !completed check in HttpStreamFactoryImpl::Request with kNumRequests = 3.

TEST_F(JobControllerLimitMultipleH2Requests, MultipleRequestsFirstRequestHang) {
  // First socket connect hang.
  auto hangdata = base::MakeUnique<SequencedSocketData>(nullptr, 0, nullptr, 0);
  hangdata->set_connect_data(MockConnect(SYNCHRONOUS, ERR_IO_PENDING));
  session_deps_.socket_factory->AddSocketDataProvider(hangdata.get());
  MockRead reads[] = {MockRead(SYNCHRONOUS, ERR_IO_PENDING)};
  std::vector<std::unique_ptr<SequencedSocketData>> socket_data;
  std::vector<std::unique_ptr<SSLSocketDataProvider>> ssl_socket_data;
  // kNumRequests - 1 will resume themselves after a delay. There will be
  // kNumRequests - 1 sockets opened.
  for (int i = 0; i < kNumRequests - 1; i++) {
    // Only the first one needs a MockRead because subsequent sockets are
    // not used to establish a SpdySession.
    auto tcp_data =
        i == 0 ? base::MakeUnique<SequencedSocketData>(reads, arraysize(reads),
                                                       nullptr, 0)
               : base::MakeUnique<SequencedSocketData>(nullptr, 0, nullptr, 0);
    tcp_data->set_connect_data(MockConnect(ASYNC, OK));
    auto ssl_data = base::MakeUnique<SSLSocketDataProvider>(ASYNC, OK);
    ssl_data->next_proto = kProtoHTTP2;
    session_deps_.socket_factory->AddSocketDataProvider(tcp_data.get());
    session_deps_.socket_factory->AddSSLSocketDataProvider(ssl_data.get());
    socket_data.push_back(std::move(tcp_data));
    ssl_socket_data.push_back(std::move(ssl_data));
  }
  HttpRequestInfo request_info;
  request_info.method = "GET";
  request_info.url = GURL("https://www.example.com");
  Initialize(request_info);
  SpdySessionPoolPeer pool_peer(session_->spdy_session_pool());
  pool_peer.SetEnableSendingInitialData(false);

  // Sets server support Http/2.
  url::SchemeHostPort server(request_info.url);
  session_->http_server_properties()->SetSupportsSpdy(server, true);

  std::vector<std::unique_ptr<MockHttpStreamRequestDelegate>> request_delegates;
  std::vector<std::unique_ptr<HttpStreamRequest>> requests;
  for (int i = 0; i < kNumRequests; ++i) {
    request_delegates.push_back(
        base::MakeUnique<MockHttpStreamRequestDelegate>());
    HttpStreamFactoryImpl::JobController* job_controller =
        new HttpStreamFactoryImpl::JobController(
            factory_, request_delegates[i].get(), session_.get(), &job_factory_,
            request_info, is_preconnect_, enable_ip_based_pooling_,
            enable_alternative_services_, SSLConfig(), SSLConfig());
    HttpStreamFactoryImplPeer::AddJobController(factory_, job_controller);
    auto request = job_controller->Start(
        request_delegates[i].get(), nullptr, NetLogWithSource(),
        HttpStreamRequest::HTTP_STREAM, DEFAULT_PRIORITY);
    EXPECT_TRUE(job_controller->main_job());
    EXPECT_FALSE(job_controller->alternative_job());
    requests.push_back(std::move(request));
  }

  for (int i = 0; i < kNumRequests; ++i) {
    EXPECT_CALL(*request_delegates[i].get(), OnStreamReadyImpl(_, _, _));
  }

  base::RunLoop().RunUntilIdle();

  EXPECT_TRUE(HttpStreamFactoryImplPeer::IsJobControllerDeleted(factory_));
  for (const auto& data : socket_data) {
    EXPECT_TRUE(data->AllReadDataConsumed());
    EXPECT_TRUE(data->AllWriteDataConsumed());
  }
}

Comment 15 by b...@chromium.org, Jun 14 2017 

Thank you for including the test subclass, this is super helpful.  I'm able to reproduce this test crashing if I also patch in the changes from https://crrev.com/2932513004 only to http_stream_factory_impl_job_controller_unittest.cc, not to any other files.  (To introduce JobControllerLimitMultipleH2Requests.)
Project Member

Comment 16 by bugdroid1@chromium.org, Jun 28 2017 

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

commit fe7f0f48f41642e5c0427e6384db0b00f9dd50af
Author: Bence Béky <bnc@chromium.org>
Date: Wed Jun 28 13:17:42 2017

Only complete Request once when pooling.

* Do not store Delegate in Request.  Instead, call Delegate directly
  from JobController.
* Handle the case when the Request is pooled to a different SpdySession.
* In this case, clean up in JobController after async callback to Job
  from socket pools.
* Modify tests not to delete requests before running message loop, as
  this is not necessary any longer.
* Modify tests to delete Requests later, before checking for
  JobController to be destroyed.

BUG= 706974 

Change-Id: I88e24771f93ba3d5b19b5e869e2b5f71f9fa9ad2
Reviewed-on: https://chromium-review.googlesource.com/548537
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Cr-Commit-Position: refs/heads/master@{#482962}
[modify] https://crrev.com/fe7f0f48f41642e5c0427e6384db0b00f9dd50af/net/http/http_stream_factory_impl_job.cc
[modify] https://crrev.com/fe7f0f48f41642e5c0427e6384db0b00f9dd50af/net/http/http_stream_factory_impl_job_controller.cc
[modify] https://crrev.com/fe7f0f48f41642e5c0427e6384db0b00f9dd50af/net/http/http_stream_factory_impl_job_controller.h
[modify] https://crrev.com/fe7f0f48f41642e5c0427e6384db0b00f9dd50af/net/http/http_stream_factory_impl_job_controller_unittest.cc
[modify] https://crrev.com/fe7f0f48f41642e5c0427e6384db0b00f9dd50af/net/http/http_stream_factory_impl_request.cc
[modify] https://crrev.com/fe7f0f48f41642e5c0427e6384db0b00f9dd50af/net/http/http_stream_factory_impl_request.h
[modify] https://crrev.com/fe7f0f48f41642e5c0427e6384db0b00f9dd50af/net/spdy/chromium/spdy_session_pool.cc

Comment 17 by b...@chromium.org, Jun 28 2017

Status: Fixed (was: Assigned)
►

Sign in to add a comment