Fix a potential infinite loop in HttpStreamFactoryImpl when a new SpdySession is established

net/http/http_stream_factory_impl_job_controller.cc

 // Copyright (c) 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_stream_factory_impl_job_controller.h"
 
 #include <memory>
 #include <string>
 #include <utility>
 
@@ skipping 607 matching lines @@
   return true;
 }
 
 void HttpStreamFactoryImpl::JobController::SetSpdySessionKey(
     Job* job,
     const SpdySessionKey& spdy_session_key) {
   if (is_preconnect_ || (job_bound_ && bound_job_ != job))
     return;
 
   DCHECK(request_);
   if (!request_->HasSpdySessionKey()) {

[Bence, 2017/03/31 16:21:27]

Is it possible that |request_| already has a SpdySessionKey at this point?  And
if it is, will a pointer to |request_| live forever under the old, hereby
overwritten key, in |factory_->spdy_session_request_map_|?

If it's not possible, maybe remove HasSpdySessionKey() altogether, and add a
DCHECK(!spdy_session_key_) within SetSpdySessionKey().

[xunjieli, 2017/03/31 17:45:26]

Good catch! It's possible that |request| already has a SpdySessionKey. The Job
can be restarted (e.g. for proxy auth). In that case, this method will be called
again with the same key.

I don't see a path where this is called with a different key though. I added a
check to remove the old key.Done.

     RequestSet& request_set =
         factory_->spdy_session_request_map_[spdy_session_key];
     DCHECK(!base::ContainsKey(request_set, request_));
     request_set.insert(request_);
     request_->SetSpdySessionKey(spdy_session_key);
   }
 }
 
 void HttpStreamFactoryImpl::JobController::
     RemoveRequestFromSpdySessionRequestMapForJob(Job* job) {
   if (is_preconnect_ || (job_bound_ && bound_job_ != job))
     return;
   DCHECK(request_);
 
-  RemoveRequestFromSpdySessionRequestMap();
-}
-
-void HttpStreamFactoryImpl::JobController::
-    RemoveRequestFromSpdySessionRequestMap() {
-  const SpdySessionKey* spdy_session_key = request_->spdy_session_key();
-  if (spdy_session_key) {
-    SpdySessionRequestMap& spdy_session_request_map =
-        factory_->spdy_session_request_map_;
-    DCHECK(base::ContainsKey(spdy_session_request_map, *spdy_session_key));
-    RequestSet& request_set = spdy_session_request_map[*spdy_session_key];
-    DCHECK(base::ContainsKey(request_set, request_));
-    request_set.erase(request_);
-    if (request_set.empty())
-      spdy_session_request_map.erase(*spdy_session_key);
-    request_->ResetSpdySessionKey();
-  }
+  factory_->RemoveRequestFromSpdySessionRequestMap(request_);
 }
 
 const NetLogWithSource* HttpStreamFactoryImpl::JobController::GetNetLog(
     Job* job) const {
   return &net_log_;
 }
 
 void HttpStreamFactoryImpl::JobController::MaybeSetWaitTimeForMainJob(
     const base::TimeDelta& delay) {
   if (main_job_is_blocked_) {
@@ skipping 96 matching lines @@
       job->net_log().source().ToEventParametersCallback());
   job->net_log().AddEvent(
       NetLogEventType::HTTP_STREAM_JOB_BOUND_TO_REQUEST,
       request_->net_log().source().ToEventParametersCallback());
 
   OrphanUnboundJob();
 }
 
 void HttpStreamFactoryImpl::JobController::CancelJobs() {
   DCHECK(request_);
-  RemoveRequestFromSpdySessionRequestMap();
+  factory_->RemoveRequestFromSpdySessionRequestMap(request_);
   if (job_bound_)
     return;
   if (alternative_job_) {
     factory_->request_map_.erase(alternative_job_.get());
     alternative_job_.reset();
   }
   if (main_job_) {
     factory_->request_map_.erase(main_job_.get());
     main_job_.reset();
   }
 }
 
 void HttpStreamFactoryImpl::JobController::OrphanUnboundJob() {
   DCHECK(request_);
-  RemoveRequestFromSpdySessionRequestMap();
+  factory_->RemoveRequestFromSpdySessionRequestMap(request_);
 
   DCHECK(bound_job_);
   if (bound_job_->job_type() == MAIN && alternative_job_) {
     factory_->request_map_.erase(alternative_job_.get());
     alternative_job_->Orphan();
   } else if (bound_job_->job_type() == ALTERNATIVE && main_job_) {
     // Orphan main job.
     factory_->request_map_.erase(main_job_.get());
     // If ResumeMainJob() is not executed, reset |main_job_|. Otherwise,
     // OnOrphanedJobComplete() will clean up |this| when the job completes.
@@ skipping 371 matching lines @@
     return;
   DCHECK(alternative_job_->alternative_proxy_server().is_valid());
   alternative_job_->Start(request_->stream_type());
 }
 
 bool HttpStreamFactoryImpl::JobController::IsJobOrphaned(Job* job) const {
   return !request_ || (job_bound_ && bound_job_ != job);
 }
 
 }  // namespace net