Deprecate instead of close SPDY sessions upon network change.

Deprecate instead of close SPDY sessions upon network change. This brings our SPDY behavior for network changes more in line with our HTTP behavior where we only close idle sockets. The benefit of this is fewer SPDY streams interrupted with ERR_NETWORK_CHANGED while still maintaining the basic principle that after a network change new requests get routed properly to possibly newly available hosts (http://crbug.com/26156). For now this change only affects Android, Windows and iOS as these OSs appear to close TCP connections upon network disconnects.  I'm avoiding applying this change to other OSs because it would lead to more sockets silently dying and waiting indefinitely rather than quickly failing upon network disconnects.

BUG=166593
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249349

[pauljensen, 2014-01-23 15:55:43]

Johnny, WDYT?  I'll add more tests if you like the general idea of this change.

[Johnny, 2014-01-23 16:14:31]

Couple thoughts: I think it'd be better to avoid mixing the GOAWAY transition
with this case. What happens if a session is Deprecate()-ed, and then receives
an actual remote GOAWAY?

Can this be done without SpdySession needing to be aware that a deprecation has
happened? Ideally that's all captured in SpdySessionPool. Skimming, it looks
like this could be done by directly calling MakeSessionUnavailable() instead
(see inline)?

https://codereview.chromium.org/129873010/diff/30001/net/spdy/spdy_session_po...
File net/spdy/spdy_session_pool.cc (right):

https://codereview.chromium.org/129873010/diff/30001/net/spdy/spdy_session_po...
net/spdy/spdy_session_pool.cc:287: (*it)->Deprecate(ERR_NETWORK_CHANGED);
Possible to call SpdySessionPool::MakeSessionUnavailable() here instead?

[pauljensen, 2014-01-23 17:21:25]

On 2014/01/23 16:14:31, Johnny wrote:
> Couple thoughts: I think it'd be better to avoid mixing the GOAWAY transition
> with this case. What happens if a session is Deprecate()-ed, and then receives
> an actual remote GOAWAY?
I think this should work as intended. Correct?

> 
> Can this be done without SpdySession needing to be aware that a deprecation
has
> happened? Ideally that's all captured in SpdySessionPool. Skimming, it looks
> like this could be done by directly calling MakeSessionUnavailable() instead
> (see inline)?
It could.  My proposed change was more of an incremental/smaller change in
behavior; essentially the only major change is the value passed as the first
argument to StartGoingAway().
I'm a little worried that instead using MakeSessionUnavailable() doesn't modify
the session's availability_state_, so if a piece of code still held a pointer to
the session (possible?) SpdySession::CreateStream wouldn't fail.
Actually I think this could cause DCHECK's in SpdySessionPool::UnmapKey() later;
say OnIPAddressChanged() was called which resulted in UnmapKey() being called,
then a GOAWAY packet was received which resulted in another call to UnmapKey()
which might DCHECK.

[Johnny, 2014-01-23 19:39:36]

On 2014/01/23 17:21:25, pauljensen wrote:
> On 2014/01/23 16:14:31, Johnny wrote:
> > Couple thoughts: I think it'd be better to avoid mixing the GOAWAY
transition
> > with this case. What happens if a session is Deprecate()-ed, and then
receives
> > an actual remote GOAWAY?
> I think this should work as intended. Correct?

Agreed. I see now that StartGoingAway() is called on close as well as remote
GOAWAY, so adding another call context isn't as large a conceptual change as I
thought.

Capturing this in SpdySessionPool is nicer IMO, but I agree that this is a
smaller, reasonable change to get the behavior.

[pauljensen, 2014-01-24 17:36:49]

Johnny, PTAL. I reworked my change in a couple ways:
1. I changed the SpdySession::Deprecated() behavior to be simpler and also avoid
needlessly killing created and pending streams.
2. I changed the SpdySessionPool::OnIPAddressChanged() behavior to avoid making
Chrome's network disconnect behavior worse on OSs that don't kill TCP
connections upon network disconnects.

[Johnny, 2014-01-24 20:04:59]

lgtm

https://codereview.chromium.org/129873010/diff/180001/net/spdy/spdy_session_u...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/129873010/diff/180001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:750: SSLSocketDataProvider ssl(SYNCHRONOUS,
OK);
Possible to remove some of the boilerplate here not targeted to what's under
test? Eg, SSLSocketDataProvider

https://codereview.chromium.org/129873010/diff/180001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:767: scoped_ptr<SpdyHeaderBlock> headers(
indent

[pauljensen, 2014-01-25 04:23:16]

Fred, PTAL.

[akalin, 2014-01-27 22:17:44]

https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:358: void Deprecate();
i'm not a huge fan of this name, or of adding a new function. Ideally, we'd be
able to re-use the existing go-away mechanism and avoid introducing a new state
(i.e., going away, but not actually closing any streams). Perhaps expose a
GoAway() function, and call that with the last active stream + 1?

[akalin, 2014-01-27 22:18:24]

On 2014/01/27 22:17:44, akalin wrote:
> https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session.h
> File net/spdy/spdy_session.h (right):
> 
>
https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session.h...
> net/spdy/spdy_session.h:358: void Deprecate();
> i'm not a huge fan of this name, or of adding a new function. Ideally, we'd be
> able to re-use the existing go-away mechanism and avoid introducing a new
state
> (i.e., going away, but not actually closing any streams). Perhaps expose a
> GoAway() function, and call that with the last active stream + 1?

Also, typos in CL description:

principal -> principle
effects -> affects

[akalin, 2014-01-27 22:20:03]

https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session_p...
File net/spdy/spdy_session_pool.cc (right):

https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session_p...
net/spdy/spdy_session_pool.cc:297: DCHECK(!IsSessionAvailable(*it));
this should probably go in the #if block, since from the #else block *it will be
NULL.

[pauljensen, 2014-01-28 19:34:45]

I addressed the typos in the description.  Please see my replies below to your
questions.

https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:358: void Deprecate();
On 2014/01/27 22:17:44, akalin wrote:
> i'm not a huge fan of this name, or of adding a new function. Ideally, we'd be
> able to re-use the existing go-away mechanism and avoid introducing a new
state
> (i.e., going away, but not actually closing any streams). Perhaps expose a
> GoAway() function, and call that with the last active stream + 1?
So Patch Set #1 essentially implemented what you describe, but the problem was
StartGoingAway() unconditionally (i.e. not dependent on last_accepted_stream_id)
closes pending streams and created_streams_ which I wanted to avoid.  Johnny had
the suggestion of not changing SpdySession at all an just having the
SpdySessionPool call MakeSessionUnavailable() but I pointed out that a session's
availability_state_ must match the pool's  available_sessions_ otherwise DCHECKs
can result.  The code I broke out into Deprecate() is from the GOAWAY message
processing and is shared with the GOAWAY processing and I believe represents as
much as is share-able with the GOAWAY processing.  What about changing the name
to something like "MarkAsGoingAway"?  If you feel strongly either way about
this, I'd be more than happy to follow your advice as I'm sure you're more
experienced in this Spdy code.

https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session_p...
File net/spdy/spdy_session_pool.cc (right):

https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session_p...
net/spdy/spdy_session_pool.cc:297: DCHECK(!IsSessionAvailable(*it));
On 2014/01/27 22:20:03, akalin wrote:
> this should probably go in the #if block, since from the #else block *it will
be
> NULL.
That was my thought too, but I wanted to keep in line with the style of
SpdySessionPool::CloseCurrentSessionsHelper() where both DCHECKS() are present
and I figured an extra check couldn't hurt and also conveys that either way
through the #if the session won't be available anymore.  Would you like me to
remove the extra DCHECK from SpdySessionPool::CloseCurrentSessionsHelper too?

[akalin, 2014-01-28 23:23:27]

https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:358: void Deprecate();
On 2014/01/28 19:34:45, pauljensen wrote:
> On 2014/01/27 22:17:44, akalin wrote:
> > i'm not a huge fan of this name, or of adding a new function. Ideally, we'd
be
> > able to re-use the existing go-away mechanism and avoid introducing a new
> state
> > (i.e., going away, but not actually closing any streams). Perhaps expose a
> > GoAway() function, and call that with the last active stream + 1?
> So Patch Set #1 essentially implemented what you describe, but the problem was
> StartGoingAway() unconditionally (i.e. not dependent on
last_accepted_stream_id)
> closes pending streams and created_streams_ which I wanted to avoid.  Johnny
had
> the suggestion of not changing SpdySession at all an just having the
> SpdySessionPool call MakeSessionUnavailable() but I pointed out that a
session's
> availability_state_ must match the pool's  available_sessions_ otherwise
DCHECKs
> can result.  The code I broke out into Deprecate() is from the GOAWAY message
> processing and is shared with the GOAWAY processing and I believe represents
as
> much as is share-able with the GOAWAY processing.  What about changing the
name
> to something like "MarkAsGoingAway"?  If you feel strongly either way about
> this, I'd be more than happy to follow your advice as I'm sure you're more
> experienced in this Spdy code.

Okay, that makes sense. How about naming it "MakeUnavailable()" and adding a
comment about how it differs from StartGoingAway()?

https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session_p...
File net/spdy/spdy_session_pool.cc (right):

https://codereview.chromium.org/129873010/diff/220001/net/spdy/spdy_session_p...
net/spdy/spdy_session_pool.cc:297: DCHECK(!IsSessionAvailable(*it));
On 2014/01/28 19:34:45, pauljensen wrote:
> On 2014/01/27 22:20:03, akalin wrote:
> > this should probably go in the #if block, since from the #else block *it
will
> be
> > NULL.
> That was my thought too, but I wanted to keep in line with the style of
> SpdySessionPool::CloseCurrentSessionsHelper() where both DCHECKS() are present
> and I figured an extra check couldn't hurt and also conveys that either way
> through the #if the session won't be available anymore.  Would you like me to
> remove the extra DCHECK from SpdySessionPool::CloseCurrentSessionsHelper too?

Nevermind I misread the code -- I thought it was a WeakPtr, but it's actually an
iterator that's dereferences to a WeakPtr. It's fine as is!

[pauljensen, 2014-01-29 20:27:06]

Fred, PTAL.  I addressed your comment.

[akalin, 2014-01-29 23:26:15]

change 'Deprecate' in CL description to 'Make unavailable'

LGTM after nits

https://codereview.chromium.org/129873010/diff/330001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/129873010/diff/330001/net/spdy/spdy_session.c...
net/spdy/spdy_session.cc:2297: void SpdySession::MakeUnavailable() {
move this under CloseSessionAndError to match header

https://codereview.chromium.org/129873010/diff/330001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/129873010/diff/330001/net/spdy/spdy_session.h...
net/spdy/spdy_session.h:356: // Mark this session as going away. It will not be
used to service new
I think the following is better wording:

// Mark this session as unavailable, meaning that it will not be used to service
new streams. Unlike when a GOAWAY frame is received, this function will not
close any streams.

https://codereview.chromium.org/129873010/diff/330001/net/spdy/spdy_session_u...
File net/spdy/spdy_session_unittest.cc (left):

https://codereview.chromium.org/129873010/diff/330001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:241: SSLSocketDataProvider ssl(SYNCHRONOUS,
OK);
nice cleanup!

https://codereview.chromium.org/129873010/diff/330001/net/spdy/spdy_session_u...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/129873010/diff/330001/net/spdy/spdy_session_u...
net/spdy/spdy_session_unittest.cc:752: // network changes, see comment in
SpdySessionPool::OnIPAddressChanged().
change , to ;

[pauljensen, 2014-02-04 21:05:56]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2014-02-04 22:33:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/129873010/350002

[commit-bot: I haz the power, 2014-02-05 01:13:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/129873010/350002

[pauljensen, 2014-02-05 15:46:40]

The CQ bit was unchecked by pauljensen@chromium.org

[pauljensen, 2014-02-05 15:46:47]

The CQ bit was checked by pauljensen@chromium.org

[Peter Mayo, 2014-02-05 19:04:15]

The CQ bit was unchecked by petermayo@chromium.org

[Peter Mayo, 2014-02-05 19:04:23]

The CQ bit was checked by petermayo@chromium.org

[Peter Mayo (wrong one), 2014-02-05 22:19:18]

The CQ bit was unchecked by petermayo@google.com

[cmp, 2014-02-05 22:26:20]

The CQ bit was checked by cmp@chromium.org

[commit-bot: I haz the power, 2014-02-05 22:29:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/129873010/350002

[commit-bot: I haz the power, 2014-02-05 22:29:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-05 22:29:53]

Failed to apply patch for net/spdy/spdy_session_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file net/spdy/spdy_session_unittest.cc
  Hunk #20 FAILED at 919.
  Hunk #21 succeeded at 1014 (offset 24 lines).
  Hunk #22 succeeded at 1078 (offset 24 lines).
  Hunk #23 succeeded at 1132 (offset 24 lines).
  Hunk #24 succeeded at 1200 (offset 24 lines).
  Hunk #25 succeeded at 1300 (offset 24 lines).
  Hunk #26 succeeded at 1350 (offset 24 lines).
  Hunk #27 succeeded at 1392 (offset 24 lines).
  Hunk #28 succeeded at 1464 (offset 24 lines).
  Hunk #29 succeeded at 1534 (offset 24 lines).
  Hunk #30 succeeded at 1548 (offset 24 lines).
  Hunk #31 succeeded at 1604 (offset 24 lines).
  Hunk #32 succeeded at 1675 (offset 24 lines).
  Hunk #33 succeeded at 1753 (offset 24 lines).
  Hunk #34 succeeded at 1834 (offset 24 lines).
  Hunk #35 succeeded at 1931 (offset 24 lines).
  Hunk #36 succeeded at 2104 (offset 24 lines).
  Hunk #37 succeeded at 2112 (offset 24 lines).
  Hunk #38 succeeded at 2122 (offset 24 lines).
  Hunk #39 succeeded at 2130 (offset 24 lines).
  Hunk #40 succeeded at 2224 (offset 24 lines).
  Hunk #41 succeeded at 2237 (offset 24 lines).
  Hunk #42 succeeded at 2245 (offset 24 lines).
  Hunk #43 succeeded at 2253 (offset 24 lines).
  Hunk #44 succeeded at 2342 (offset 24 lines).
  Hunk #45 succeeded at 2433 (offset 24 lines).
  Hunk #46 succeeded at 2546 (offset 24 lines).
  Hunk #47 succeeded at 2617 (offset 24 lines).
  Hunk #48 succeeded at 2860 (offset 24 lines).
  Hunk #49 succeeded at 2964 (offset 24 lines).
  Hunk #50 succeeded at 2999 (offset 24 lines).
  Hunk #51 succeeded at 3065 (offset 24 lines).
  Hunk #52 succeeded at 3124 (offset 24 lines).
  Hunk #53 succeeded at 3211 (offset 24 lines).
  Hunk #54 succeeded at 3286 (offset 24 lines).
  Hunk #55 succeeded at 3361 (offset 24 lines).
  Hunk #56 succeeded at 3450 (offset 24 lines).
  1 out of 56 hunks FAILED -- saving rejects to file
net/spdy/spdy_session_unittest.cc.rej

Patch:       net/spdy/spdy_session_unittest.cc
Index: net/spdy/spdy_session_unittest.cc
diff --git a/net/spdy/spdy_session_unittest.cc
b/net/spdy/spdy_session_unittest.cc
index
2f185c7c01a09c6c7828f5acf526f8d8b4384365..b4cee89e4e8d1ece46c7cb978e18d49ea812faf9
100644
--- a/net/spdy/spdy_session_unittest.cc
+++ b/net/spdy/spdy_session_unittest.cc
@@ -238,9 +238,6 @@ TEST_P(SpdySessionTest, PendingStreamCancellingAnother) {
   data.set_connect_data(connect_data);
   session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);
 
-  SSLSocketDataProvider ssl(SYNCHRONOUS, OK);
-  session_deps_.deterministic_socket_factory->AddSSLSocketDataProvider(&ssl);
-
   CreateDeterministicNetworkSession();
 
   base::WeakPtr<SpdySession> session =
@@ -298,9 +295,6 @@ TEST_P(SpdySessionTest, GoAwayWithNoActiveStreams) {
   data.set_connect_data(connect_data);
   session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);
 
-  SSLSocketDataProvider ssl(SYNCHRONOUS, OK);
-  session_deps_.deterministic_socket_factory->AddSSLSocketDataProvider(&ssl);
-
   CreateDeterministicNetworkSession();
 
   base::WeakPtr<SpdySession> session =
@@ -332,9 +326,6 @@ TEST_P(SpdySessionTest,
GoAwayImmediatelyWithNoActiveStreams) {
   data.set_connect_data(connect_data);
   session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);
 
-  SSLSocketDataProvider ssl(SYNCHRONOUS, OK);
-  session_deps_.deterministic_socket_factory->AddSSLSocketDataProvider(&ssl);
-
   CreateDeterministicNetworkSession();
 
   data.StopAfter(1);
@@ -369,9 +360,6 @@ TEST_P(SpdySessionTest, GoAwayWithActiveStreams) {
   data.set_connect_data(connect_data);
   session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);
 
-  SSLSocketDataProvider ssl(SYNCHRONOUS, OK);
-  session_deps_.deterministic_socket_factory->AddSSLSocketDataProvider(&ssl);
-
   CreateDeterministicNetworkSession();
 
   base::WeakPtr<SpdySession> session =
@@ -379,7 +367,7 @@ TEST_P(SpdySessionTest, GoAwayWithActiveStreams) {
 
   EXPECT_EQ(spdy_util_.spdy_version(), session->GetProtocolVersion());
 
-  GURL url("http://www.google.com");
+  GURL url(kDefaultURL);
   base::WeakPtr<SpdyStream> spdy_stream1 =
       CreateStreamSynchronously(SPDY_REQUEST_RESPONSE_STREAM,
                                 session, url, MEDIUM, BoundNetLog());
@@ -453,9 +441,6 @@ TEST_P(SpdySessionTest, GoAwayTwice) {
   data.set_connect_data(connect_data);
   session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);
 
-  SSLSocketDataProvider ssl(SYNCHRONOUS, OK);
-  session_deps_.deterministic_socket_factory->AddSSLSocketDataProvider(&ssl);
-
   CreateDeterministicNetworkSession();
 
   base::WeakPtr<SpdySession> session =
@@ -463,7 +448,7 @@ TEST_P(SpdySessionTest, GoAwayTwice) {
 
   EXPECT_EQ(spdy_util_.spdy_version(), session->GetProtocolVersion());
 
-  GURL url("http://www.google.com");
+  GURL url(kDefaultURL);
   base::WeakPtr<SpdyStream> spdy_stream1 =
       CreateStreamSynchronously(SPDY_REQUEST_RESPONSE_STREAM,
                                 session, url, MEDIUM, BoundNetLog());
@@ -535,9 +520,6 @@ TEST_P(SpdySessionTest, GoAwayWithActiveStreamsThenClose) {
   data.set_connect_data(connect_data);
   session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);
 
-  SSLSocketDataProvider ssl(SYNCHRONOUS, OK);
-  session_deps_.deterministic_socket_factory->AddSSLSocketDataProvider(&ssl);
-
   CreateDeterministicNetworkSession();
 
   base::WeakPtr<SpdySession> session =
@@ -545,7 +527,7 @@ TEST_P(SpdySessionTest, GoAwayWithActiveStreamsThenClose) {
 
   EXPECT_EQ(spdy_util_.spdy_version(), session->GetProtocolVersion());
 
-  GURL url("http://www.google.com");
+  GURL url(kDefaultURL);
   base::WeakPtr<SpdyStream> spdy_stream1 =
       CreateStreamSynchronously(SPDY_REQUEST_RESPONSE_STREAM,
                                 session, url, MEDIUM, BoundNetLog());
@@ -594,7 +576,6 @@ TEST_P(SpdySessionTest, GoAwayWithActiveStreamsThenClose) {
 // Try to create a stream after receiving a GOAWAY frame. It should
 // fail.
 TEST_P(SpdySessionTest, CreateStreamAfterGoAway) {
-  const char kStreamUrl[] = "http://www.google.com";
   session_deps_.host_resolver->set_synchronous_mode(true);
 
   MockConnect connect_data(SYNCHRONOUS, OK);
@@ -613,9 +594,6 @@ TEST_P(SpdySessionTest, CreateStreamAfterGoAway) {
   data.set_connect_data(connect_data);
   session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);
 
-  SSLSocketDataProvider ssl(SYNCHRONOUS, OK);
-  session_deps_.deterministic_socket_factory->AddSSLSocketDataProvider(&ssl);
-
   CreateDeterministicNetworkSession();
 
   base::WeakPtr<SpdySession> session =
@@ -623,7 +601,7 @@ TEST_P(SpdySessionTest, CreateStreamAfterGoAway) {
 
   EXPECT_EQ(spdy_util_.spdy_version(), session->GetProtocolVersion());
 
-  GURL url(kStreamUrl);
+  GURL url(kDefaultURL);
   base::WeakPtr<SpdyStream> spdy_stream =
       CreateStreamSynchronously(SPDY_REQUEST_RESPONSE_STREAM,
                                 session, url, MEDIUM, BoundNetLog());
@@ -662,13 +640,12 @@ TEST_P(SpdySessionTest, CreateStreamAfterGoAway) {
 // Receiving a SYN_STREAM frame after a GOAWAY frame should result in
 // the stream being refused.
 TEST_P(SpdySessionTest, SynStreamAfterGoAway) {
-  const char kStreamUrl[] = "http://www.google.com";
   session_deps_.host_resolver->set_synchronous_mode(true);
 
   MockConnect connect_data(SYNCHRONOUS, OK);
   scoped_ptr<SpdyFrame> goaway(spdy_util_.ConstructSpdyGoAway(1));
   scoped_ptr<SpdyFrame>
-      push(spdy_util_.ConstructSpdyPush(NULL, 0, 2, 1, kStreamUrl));
+      push(spdy_util_.ConstructSpdyPush(NULL, 0, 2, 1, kDefaultURL));
   MockRead reads[] = {
     CreateMockRead(*goaway, 1),
     CreateMockRead(*push, 2),
@@ -687,9 +664,6 @@ TEST_P(SpdySessionTest, SynStreamAfterGoAway) {
   data.set_connect_data(connect_data);
   session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);
 
-  SSLSocketDataProvider ssl(SYNCHRONOUS, OK);
-  session_deps_.deterministic_socket_factory->AddSSLSocketDataProvider(&ssl);
-
   CreateDeterministicNetworkSession();
 
   base::WeakPtr<SpdySession> session =
@@ -697,7 +671,7 @@ TEST_P(SpdySessionTest, SynStreamAfterGoAway) {
 
   EXPECT_EQ(spdy_util_.spdy_version(), session->GetProtocolVersion());
 
-  GURL url(kStreamUrl);
+  GURL url(kDefaultURL);
   base::WeakPtr<SpdyStream> spdy_stream =
       CreateStreamSynchronously(SPDY_REQUEST_RESPONSE_STREAM,
                                 session, url, MEDIUM, BoundNetLog());
@@ -728,6 +702,73 @@ TEST_P(SpdySessionTest, SynStreamAfterGoAway) {
   EXPECT_TRUE(session == NULL);
 }
 
+// A session observing a network change with active streams should close
+// when the last active stream is closed.
+TEST_P(SpdySessionTest, NetworkChangeWithActiveStreams) {
+  session_deps_.host_resolver->set_synchronous_mode(true);
+
+  MockConnect connect_data(SYNCHRONOUS, OK);
+  MockRead reads[] = {
+    MockRead(ASYNC, 0, 1)  // EOF
+  };
+  scoped_ptr<SpdyFrame> req1(
+      spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, MEDIUM, true));
+  MockWrite writes[] = {
+    CreateMockWrite(*req1, 0),
+  };
+  DeterministicSocketData data(reads, arraysize(reads),
+                               writes, arraysize(writes));
+  data.set_connect_data(connect_data);
+  session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);
+
+  CreateDeterministicNetworkSession();
+
+  base::WeakPtr<SpdySession> session =
+      CreateInsecureSpdySession(http_session_, key_, BoundNetLog());
+
+  EXPECT_EQ(spdy_util_.spdy_version(), session->GetProtocolVersion());
+
+  base::WeakPtr<SpdyStream> spdy_stream =
+      CreateStreamSynchronously(SPDY_REQUEST_RESPONSE_STREAM, session,
+                                GURL(kDefaultURL), MEDIUM, BoundNetLog());
+  test::StreamDelegateDoNothing delegate(spdy_stream);
+  spdy_stream->SetDelegate(&delegate);
+
+  scoped_ptr<SpdyHeaderBlock> headers(
+  …
(message too large)

[cmp, 2014-02-05 22:31:37]

Paul, please rebase, re-upload, and let's try again.

[pauljensen, 2014-02-06 01:40:46]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2014-02-06 01:44:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pauljensen@chromium.org/129873010/880001

[commit-bot: I haz the power, 2014-02-06 11:02:59]

Change committed as 249349