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

net/http/http_stream_factory_impl.cc

 // Copyright (c) 2012 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.h"
 
 #include <string>
 #include <tuple>
 
 #include "base/logging.h"
@@ skipping 232 matching lines @@
     if (!spdy_session)
       break;
     const SpdySessionKey& spdy_session_key = spdy_session->spdy_session_key();
     // Each iteration may empty out the RequestSet for |spdy_session_key| in
     // |spdy_session_request_map_|. So each time, check for RequestSet and use
     // the first one.
     //
     // TODO(willchan): If it's important, switch RequestSet out for a FIFO
     // queue (Order by priority first, then FIFO within same priority). Unclear
     // that it matters here.
     if (!base::ContainsKey(spdy_session_request_map_, spdy_session_key))

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

Once you are here, consider removing one of these two lookups here as well by
using find() directly.

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

Done.

       break;
     Request* request = *spdy_session_request_map_[spdy_session_key].begin();
     request->Complete(was_alpn_negotiated, negotiated_protocol, using_spdy);
+    RemoveRequestFromSpdySessionRequestMap(request);
     if (for_websockets_) {
       // TODO(ricea): Restore this code path when WebSocket over SPDY
       // implementation is ready.
       NOTREACHED();
     } else if (request->stream_type() ==
                HttpStreamRequest::BIDIRECTIONAL_STREAM) {
       request->OnBidirectionalStreamImplReady(
           used_ssl_config, used_proxy_info,
           new BidirectionalStreamSpdyImpl(spdy_session, source_dependency));
     } else {
@@ skipping 139 matching lines @@
   // The number of non-preconnect controllers with a pending main job.
   factory_dump->AddScalar("main_job_count",
                           base::trace_event::MemoryAllocatorDump::kUnitsObjects,
                           main_job_count);
   // The number of preconnect controllers.
   factory_dump->AddScalar("preconnect_count",
                           base::trace_event::MemoryAllocatorDump::kUnitsObjects,
                           preconnect_controller_count);
 }
 
+void HttpStreamFactoryImpl::RemoveRequestFromSpdySessionRequestMap(
+    Request* request) {
+  const SpdySessionKey* spdy_session_key = request->spdy_session_key();
+  if (!spdy_session_key)
+    return;
+  if (!base::ContainsKey(spdy_session_request_map_, *spdy_session_key))

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

Please use |iterator it = find(*spdy_session_key); if (iterator == end())
return; request_set = it.second;| or something similar to avoid doing the lookup
twice.

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

Done.

+    return;
+  RequestSet& request_set = spdy_session_request_map_[*spdy_session_key];
+  if (base::ContainsKey(request_set, request))

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

Skip the ContainsKey() call in order to avoid doing the lookup twice: it is
valid to call erase() even if the key does not exist, in which case it is a
no-op.  Maybe add a comment if you think it is necessary.

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

Done.

+    request_set.erase(request);
+  if (request_set.empty())
+    spdy_session_request_map_.erase(*spdy_session_key);

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

And use the iterator here to avoid the third lookup.

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

Done. Good idea!

+}
+
 }  // namespace net