Speculative fix for SpdySettingsStorage crasher.

I've added a unit test to exercise the code, but it didn't trigger the crash.

BUG=57331
TEST=none

Review URL: http://codereview.chromium.org/3517012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@61880 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 138 insertions, 3 deletions

diff --git a/net/spdy/spdy_session.cc b/net/spdy/spdy_session.cc
index b00e938..a360c52 100644
--- a/net/spdy/spdy_session.cc
+++ b/net/spdy/spdy_session.cc
@@ -362,7 +362,11 @@ void SpdySession::ProcessPendingCreateStreams() {
                                      pending_create.priority,
                                      pending_create.spdy_stream,
                                      *pending_create.stream_net_log);
-        pending_create.callback->Run(error);
+        MessageLoop::current()->PostTask(
+            FROM_HERE,
+            method_factory_.NewRunnableMethod(
+                &SpdySession::InvokeUserStreamCreationCallback,
+                pending_create.callback, error));
         break;
       }
     }
@@ -1298,4 +1302,9 @@ void SpdySession::RecordHistograms() {
   }
 }
 
+void SpdySession::InvokeUserStreamCreationCallback(
+    CompletionCallback* callback, int rv) {
+  callback->Run(rv);
+}
+
 }  // namespace net
diff --git a/net/spdy/spdy_session.h b/net/spdy/spdy_session.h
index 8ef6543..de5f0e2 100644
--- a/net/spdy/spdy_session.h
+++ b/net/spdy/spdy_session.h
@@ -295,6 +295,10 @@ class SpdySession : public base::RefCounted<SpdySession>,
   // Closes all streams.  Used as part of shutdown.
   void CloseAllStreams(net::Error status);
 
+  // Invokes a user callback for stream creation.  We provide this method so it
+  // can be deferred to the MessageLoop, so we avoid re-entrancy problems.
+  void InvokeUserStreamCreationCallback(CompletionCallback* callback, int rv);
+
   // Callbacks for the Spdy session.
   CompletionCallbackImpl<SpdySession> read_callback_;
   CompletionCallbackImpl<SpdySession> write_callback_;
@@ -311,7 +315,7 @@ class SpdySession : public base::RefCounted<SpdySession>,
   // |spdy_session_pool_| owns us, therefore its lifetime must exceed ours.  We
   // set this to NULL after we are removed from the pool.
   SpdySessionPool* spdy_session_pool_;
-  SpdySettingsStorage* spdy_settings_;
+  SpdySettingsStorage* const spdy_settings_;
 
   // The socket handle for this session.
   scoped_ptr<ClientSocketHandle> connection_;
diff --git a/net/spdy/spdy_session_unittest.cc b/net/spdy/spdy_session_unittest.cc
index 38de37a..4a66406 100644
--- a/net/spdy/spdy_session_unittest.cc
+++ b/net/spdy/spdy_session_unittest.cc
@@ -2,8 +2,9 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "net/spdy/spdy_io_buffer.h"
 #include "net/spdy/spdy_session.h"
+
+#include "net/spdy/spdy_io_buffer.h"
 #include "net/spdy/spdy_stream.h"
 #include "net/spdy/spdy_test_util.h"
 #include "testing/platform_test.h"
@@ -20,6 +21,7 @@ class SpdySessionTest : public PlatformTest {
 };
 
 namespace {
+
 // Test the SpdyIOBuffer class.
 TEST_F(SpdySessionTest, SpdyIOBuffer) {
   std::priority_queue<SpdyIOBuffer> queue_;
@@ -115,5 +117,125 @@ TEST_F(SpdySessionTest, GoAway) {
   session2 = NULL;
 }
 
+class StreamReleaserCallback : public CallbackRunner<Tuple1<int> > {
+ public:
+  StreamReleaserCallback(SpdySession* session,
+                         SpdyStream* first_stream)
+      : session_(session), first_stream_(first_stream) {}
+  ~StreamReleaserCallback() {}
+
+  int WaitForResult() { return callback_.WaitForResult(); }
+
+  virtual void RunWithParams(const Tuple1<int>& params) {
+    session_->CloseSessionOnError(ERR_FAILED, false);
+    session_ = NULL;
+    first_stream_->Cancel();
+    first_stream_ = NULL;
+    stream_->Cancel();
+    stream_ = NULL;
+    callback_.RunWithParams(params);
+  }
+
+  scoped_refptr<SpdyStream>* stream() { return &stream_; }
+
+ private:
+  scoped_refptr<SpdySession> session_;
+  scoped_refptr<SpdyStream> first_stream_;
+  scoped_refptr<SpdyStream> stream_;
+  TestCompletionCallback callback_;
+};
+
+// Start with max concurrent streams set to 1.  Request two streams.  Receive a
+// settings frame setting max concurrent streams to 2.  Have the callback
+// release the stream, which releases its reference (the last) to the session.
+// Make sure nothing blows up.
+// http://crbug.com/57331
+TEST_F(SpdySessionTest, OnSettings) {
+  SpdySessionDependencies session_deps;
+  session_deps.host_resolver->set_synchronous_mode(true);
+
+  spdy::SpdySettings new_settings;
+  spdy::SettingsFlagsAndId id(spdy::SETTINGS_MAX_CONCURRENT_STREAMS);
+  id.set_id(spdy::SETTINGS_MAX_CONCURRENT_STREAMS);
+  const size_t max_concurrent_streams = 2;
+  new_settings.push_back(spdy::SpdySetting(id, max_concurrent_streams));
+
+  // Set up the socket so we read a SETTINGS frame that raises max concurrent
+  // streams to 2.
+  MockConnect connect_data(false, OK);
+  scoped_ptr<spdy::SpdyFrame> settings_frame(
+      ConstructSpdySettings(new_settings));
+  MockRead reads[] = {
+    CreateMockRead(*settings_frame),
+    MockRead(false, 0, 0)  // EOF
+  };
+
+  StaticSocketDataProvider data(reads, arraysize(reads), NULL, 0);
+  data.set_connect_data(connect_data);
+  session_deps.socket_factory->AddSocketDataProvider(&data);
+
+  SSLSocketDataProvider ssl(false, OK);
+  session_deps.socket_factory->AddSSLSocketDataProvider(&ssl);
+
+  scoped_refptr<HttpNetworkSession> http_session(
+      SpdySessionDependencies::SpdyCreateSession(&session_deps));
+
+  const std::string kTestHost("www.foo.com");
+  const int kTestPort = 80;
+  HostPortPair test_host_port_pair(kTestHost, kTestPort);
+  HostPortProxyPair pair(test_host_port_pair, ProxyServer::Direct());
+
+  // Initialize the SpdySettingsStorage with 1 max concurrent streams.
+  spdy::SpdySettings old_settings;
+  id.set_flags(spdy::SETTINGS_FLAG_PLEASE_PERSIST);
+  old_settings.push_back(spdy::SpdySetting(id, 1));
+  http_session->mutable_spdy_settings()->Set(
+      test_host_port_pair, old_settings);
+
+  // Create a session.
+  SpdySessionPool* spdy_session_pool(http_session->spdy_session_pool());
+  EXPECT_FALSE(spdy_session_pool->HasSession(pair));
+  scoped_refptr<SpdySession> session =
+      spdy_session_pool->Get(pair, http_session->mutable_spdy_settings(),
+                             BoundNetLog());
+  ASSERT_TRUE(spdy_session_pool->HasSession(pair));
+
+  scoped_refptr<TCPSocketParams> tcp_params =
+      new TCPSocketParams(kTestHost, kTestPort, MEDIUM, GURL(), false);
+  scoped_ptr<ClientSocketHandle> connection(new ClientSocketHandle);
+  EXPECT_EQ(OK,
+            connection->Init(test_host_port_pair.ToString(), tcp_params, MEDIUM,
+                              NULL, http_session->tcp_socket_pool(),
+                              BoundNetLog()));
+  EXPECT_EQ(OK, session->InitializeWithSocket(connection.release(), false, OK));
+
+  // Create 2 streams.  First will succeed.  Second will be pending.
+  scoped_refptr<SpdyStream> spdy_stream1;
+  TestCompletionCallback callback1;
+  GURL url("http://www.google.com");
+  EXPECT_EQ(OK,
+            session->CreateStream(url,
+                                  MEDIUM, /* priority, not important */
+                                  &spdy_stream1,
+                                  BoundNetLog(),
+                                  &callback1));
+
+  StreamReleaserCallback stream_releaser(session, spdy_stream1);
+
+  ASSERT_EQ(ERR_IO_PENDING,
+            session->CreateStream(url,
+                                  MEDIUM, /* priority, not important */
+                                  stream_releaser.stream(),
+                                  BoundNetLog(),
+                                  &stream_releaser));
+
+  // Make sure |stream_releaser| holds the last refs.
+  session = NULL;
+  spdy_stream1 = NULL;
+
+  EXPECT_EQ(OK, stream_releaser.WaitForResult());
+}
+
 }  // namespace
+
 }  // namespace net