Avoid DCHECK on SpdySessionPool destruction.

It is possible that a DCHECK in SpdySession::DcheckDraining() is
triggered when SpdySessionPool destructor call
SpdySessionPool::RemoveUnavailableSession() which destroys the
SpdySession, if the SpdySession is in STATE_GOING_AWAY.

One way for this is that on certain platforms,
SpdySessionPool::OnIPAddressChanged() may leave a SpdySession with an
active stream in STATE_GOING_AWAY.

Another possible way is receiving a "graceful GOAWAY" from the server,
which also puts a SpdySession with active streams in STATE_GOING_AWAY.

Patch set 1 introduces two unit tests, one for each of the above
scenarios, that trigger this DCHECK.  See trybot outputs for failing
tests.

Patch set 2 fixes the bug by ensuring in
SpdySessionPool::CloseAllSessions() that every owned SpdySession is
draining, even if there are no currently active sessions. This method
is not only called by SpdySessionPool destructor, but also by
HttpNetworkSession destructor (also adding a TODO for that).

Also include build/build_config.h for OS_ANDROID, OS_WIN, OS_IOS macros.

Bug: 789791
Change-Id: Idc3e2350b2ac6354be5c5e6b32ea8e0227f3e2d6
Reviewed-on: https://chromium-review.googlesource.com/810889Reviewed-by: Helen Li <xunjieli@chromium.org>
Commit-Queue: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523186}

net/http/http_network_session.cc

@@ -256,6 +256,8 @@ HttpNetworkSession::HttpNetworkSession(const Params& params,
 HttpNetworkSession::~HttpNetworkSession() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   response_drainers_.clear();
+  // TODO(bnc): CloseAllSessions() is also called in SpdySessionPool destructor,
+  // one of the two calls should be removed.
   spdy_session_pool_.CloseAllSessions();
   base::MemoryCoordinatorClientRegistry::GetInstance()->Unregister(this);
 }

net/spdy/chromium/spdy_session_pool.cc

@@ -4,6 +4,7 @@
 
 #include "net/spdy/chromium/spdy_session_pool.h"
 
+#include <algorithm>
 #include <utility>
 
 #include "base/logging.h"
@@ -13,6 +14,7 @@
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_event.h"
 #include "base/values.h"
+#include "build/build_config.h"
 #include "net/base/address_list.h"
 #include "net/base/trace_constants.h"
 #include "net/http/http_network_session.h"
@@ -77,6 +79,8 @@ SpdySessionPool::SpdySessionPool(
 
 SpdySessionPool::~SpdySessionPool() {
   DCHECK(spdy_session_request_map_.empty());
+  // TODO(bnc): CloseAllSessions() is also called in HttpNetworkSession
+  // destructor, one of the two calls should be removed.
   CloseAllSessions();
 
   while (!sessions_.empty()) {
@@ -270,7 +274,9 @@ void SpdySessionPool::CloseCurrentIdleSessions() {
 }
 
 void SpdySessionPool::CloseAllSessions() {
-  while (!available_sessions_.empty()) {
+  auto is_draining = [](const SpdySession* s) { return s->IsDraining(); };
+  // Repeat until every SpdySession owned by |this| is draining.
+  while (!std::all_of(sessions_.begin(), sessions_.end(), is_draining)) {
     CloseCurrentSessionsHelper(ERR_ABORTED, "Closing all sessions.",
                                false /* idle_only */);
   }
@@ -521,16 +527,20 @@ void SpdySessionPool::CloseCurrentSessionsHelper(Error error,
                                                  const SpdyString& description,
                                                  bool idle_only) {
   WeakSessionList current_sessions = GetCurrentSessions();
-  for (WeakSessionList::const_iterator it = current_sessions.begin();
-       it != current_sessions.end(); ++it) {
-    if (!*it)
+  for (base::WeakPtr<SpdySession>& session : current_sessions) {
+    if (!session)
       continue;
 
-    if (idle_only && (*it)->is_active())
+    if (idle_only && session->is_active())
       continue;
 
-    (*it)->CloseSessionOnError(error, description);
-    DCHECK(!IsSessionAvailable(*it));
+    if (session->IsDraining())
+      continue;
+
+    session->CloseSessionOnError(error, description);
+
+    DCHECK(!IsSessionAvailable(session));
+    DCHECK(!session || session->IsDraining());
   }
 }
 

net/spdy/chromium/spdy_session_pool.h

@@ -111,6 +111,10 @@ class NET_EXPORT SpdySessionPool
   void RemoveUnavailableSession(
       const base::WeakPtr<SpdySession>& unavailable_session);
 
+  // Note that the next three methods close sessions, potentially notifing
+  // delegates of error or synchronously invoking callbacks, which might trigger
+  // retries, thus opening new sessions.
+
   // Close only the currently existing SpdySessions with |error|.
   // Let any new ones created while this method is running continue to
   // live.
@@ -121,8 +125,9 @@ class NET_EXPORT SpdySessionPool
   // live.
   void CloseCurrentIdleSessions();
 
-  // Close all SpdySessions, including any new ones created in the process of
-  // closing the current ones.
+  // Repeatedly close all SpdySessions until all of them (including new ones
+  // created in the process of closing the current ones, and new ones created in
+  // the process of closing those new ones, etc.) are unavailable.
   void CloseAllSessions();
 
   // Creates a Value summary of the state of the spdy session pool.

net/spdy/chromium/spdy_session_pool_unittest.cc

@@ -13,6 +13,7 @@
 #include "base/trace_event/memory_allocator_dump.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_event_argument.h"
+#include "build/build_config.h"
 #include "net/dns/host_cache.h"
 #include "net/http/http_network_session.h"
 #include "net/log/net_log_with_source.h"
@@ -806,6 +807,114 @@ TEST_F(SpdySessionPoolTest, IPAddressChanged) {
 #endif  // defined(OS_ANDROID) || defined(OS_WIN) || defined(OS_IOS)
 }
 
+// Regression test for https://crbug.com/789791.
+TEST_F(SpdySessionPoolTest, HandleIPAddressChangeThenShutdown) {
+  MockRead reads[] = {MockRead(SYNCHRONOUS, ERR_IO_PENDING)};
+  SpdyTestUtil spdy_util;
+  SpdySerializedFrame req(spdy_util.ConstructSpdyGet(kDefaultUrl, 1, MEDIUM));
+  MockWrite writes[] = {CreateMockWrite(req, 1)};
+  StaticSocketDataProvider data(reads, arraysize(reads), writes,
+                                arraysize(writes));
+
+  MockConnect connect_data(SYNCHRONOUS, OK);
+  data.set_connect_data(connect_data);
+
+  session_deps_.socket_factory->AddSocketDataProvider(&data);
+  AddSSLSocketData();
+
+  CreateNetworkSession();
+
+  const GURL url(kDefaultUrl);
+  SpdySessionKey key(HostPortPair::FromURL(url), ProxyServer::Direct(),
+                     PRIVACY_MODE_DISABLED);
+  base::WeakPtr<SpdySession> session =
+      CreateSpdySession(http_session_.get(), key, NetLogWithSource());
+
+  base::WeakPtr<SpdyStream> spdy_stream = CreateStreamSynchronously(
+      SPDY_BIDIRECTIONAL_STREAM, session, url, MEDIUM, NetLogWithSource());
+  test::StreamDelegateDoNothing delegate(spdy_stream);
+  spdy_stream->SetDelegate(&delegate);
+
+  SpdyHeaderBlock headers(spdy_util.ConstructGetHeaderBlock(url.spec()));
+  spdy_stream->SendRequestHeaders(std::move(headers), NO_MORE_DATA_TO_SEND);
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(delegate.send_headers_completed());
+
+  spdy_session_pool_->OnIPAddressChanged();
+
+#if defined(OS_ANDROID) || defined(OS_WIN) || defined(OS_IOS)
+  EXPECT_EQ(1u, num_active_streams(session));
+  EXPECT_TRUE(session->IsGoingAway());
+  EXPECT_FALSE(session->IsDraining());
+#else
+  EXPECT_EQ(0u, num_active_streams(session));
+  EXPECT_FALSE(session->IsGoingAway());
+  EXPECT_TRUE(session->IsDraining());
+#endif  // defined(OS_ANDROID) || defined(OS_WIN) || defined(OS_IOS)
+
+  http_session_.reset();
+
+  data.AllReadDataConsumed();
+  data.AllWriteDataConsumed();
+}
+
+// Regression test for https://crbug.com/789791.
+TEST_F(SpdySessionPoolTest, HandleGracefulGoawayThenShutdown) {
+  SpdyTestUtil spdy_util;
+  SpdySerializedFrame goaway(spdy_util.ConstructSpdyGoAway(
+      0x7fffffff, ERROR_CODE_NO_ERROR, "Graceful shutdown."));
+  MockRead reads[] = {
+      MockRead(ASYNC, ERR_IO_PENDING, 1), CreateMockRead(goaway, 2),
+      MockRead(ASYNC, ERR_IO_PENDING, 3), MockRead(ASYNC, OK, 4)};
+  SpdySerializedFrame req(spdy_util.ConstructSpdyGet(kDefaultUrl, 1, MEDIUM));
+  MockWrite writes[] = {CreateMockWrite(req, 0)};
+  SequencedSocketData data(reads, arraysize(reads), writes, arraysize(writes));
+
+  MockConnect connect_data(SYNCHRONOUS, OK);
+  data.set_connect_data(connect_data);
+
+  session_deps_.socket_factory->AddSocketDataProvider(&data);
+  AddSSLSocketData();
+
+  CreateNetworkSession();
+
+  const GURL url(kDefaultUrl);
+  SpdySessionKey key(HostPortPair::FromURL(url), ProxyServer::Direct(),
+                     PRIVACY_MODE_DISABLED);
+  base::WeakPtr<SpdySession> session =
+      CreateSpdySession(http_session_.get(), key, NetLogWithSource());
+
+  base::WeakPtr<SpdyStream> spdy_stream = CreateStreamSynchronously(
+      SPDY_BIDIRECTIONAL_STREAM, session, url, MEDIUM, NetLogWithSource());
+  test::StreamDelegateDoNothing delegate(spdy_stream);
+  spdy_stream->SetDelegate(&delegate);
+
+  SpdyHeaderBlock headers(spdy_util.ConstructGetHeaderBlock(url.spec()));
+  spdy_stream->SendRequestHeaders(std::move(headers), NO_MORE_DATA_TO_SEND);
+
+  // Send headers.
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(delegate.send_headers_completed());
+
+  EXPECT_EQ(1u, num_active_streams(session));
+  EXPECT_FALSE(session->IsGoingAway());
+  EXPECT_FALSE(session->IsDraining());
+
+  // Read GOAWAY.
+  data.Resume();
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(1u, num_active_streams(session));
+  EXPECT_TRUE(session->IsGoingAway());
+  EXPECT_FALSE(session->IsDraining());
+
+  http_session_.reset();
+
+  data.AllReadDataConsumed();
+  data.AllWriteDataConsumed();
+}
+
 class SpdySessionMemoryDumpTest
     : public SpdySessionPoolTest,
       public testing::WithParamInterface<