There was a crash under certain flip protocol errors when the session is torn

down.  Basically, an error occurred, and we had cleaned up all streams and
removed our last reference from the FlipSessionPool, causing |this| to be
destroyed.  But on the stack we came through and issued another read off the
now defunct FlipSession.

Added two new tests for FlipSession errors - one when frame decompression fails,
and one when we get a full protocol error on the session.

NOTE: Had to remove DCHECK from flip_framer; I'm intentionally triggering that
DCHECK, but it fires on our unit tests and windows doesn't support death tests
in any reasonable way.  Since the check was an arbitrary assist for early
debugging, this should be okay.

BUG=none
TEST=flip_network_transaction_unittest.cc

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

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

4 files changed, 131 insertions, 16 deletions

diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h
index 4003ee0..81ca271 100644
--- a/net/base/net_error_list.h
+++ b/net/base/net_error_list.h
@@ -291,6 +291,9 @@ NET_ERROR(INVALID_FLIP_STREAM, -335)
 // There are no supported proxies in the provided list.
 NET_ERROR(NO_SUPPORTED_PROXIES, -336)
 
+// There is a FLIP protocol framing error.
+NET_ERROR(FLIP_PROTOCOL_ERROR, -337)
+
 // The cache does not have the requested entry.
 NET_ERROR(CACHE_MISS, -400)
 
diff --git a/net/flip/flip_framer.cc b/net/flip/flip_framer.cc
index 69d25c8..5332b25 100644
--- a/net/flip/flip_framer.cc
+++ b/net/flip/flip_framer.cc
@@ -123,9 +123,9 @@ size_t FlipFramer::BytesSafeToRead() const {
 }
 
 void FlipFramer::set_error(FlipError error) {
-  DCHECK(false);
   DCHECK(visitor_);
   error_code_ = error;
+  CHANGE_STATE(FLIP_ERROR);
   visitor_->OnError(this);
 }
 
@@ -233,7 +233,13 @@ size_t FlipFramer::ProcessCommonHeader(const char* data, size_t len) {
       break;
     }
     remaining_payload_ = current_frame.length();
-    DCHECK_LE(remaining_payload_, 1000000u);  // Sanity!
+
+    // This is just a sanity check for help debugging early frame errors.
+    if (remaining_payload_ > 1000000u) {
+      LOG(ERROR) <<
+          "Unexpectedly large frame.  Flip session is likely corrupt.";
+    }
+
     // if we're here, then we have the common header all received.
     if (!current_frame.is_control_frame())
       CHANGE_STATE(FLIP_FORWARD_STREAM_FRAME);
@@ -278,17 +284,13 @@ void FlipFramer::ProcessControlFrameHeader() {
   if (current_control_frame.version() != kFlipProtocolVersion)
     set_error(FLIP_UNSUPPORTED_VERSION);
 
-  if (error_code_) {
-    CHANGE_STATE(FLIP_ERROR);
-    return;
-  }
-
   remaining_control_payload_ = current_control_frame.length();
-  if (remaining_control_payload_ > kControlFrameBufferMaxSize) {
+  if (remaining_control_payload_ > kControlFrameBufferMaxSize)
     set_error(FLIP_CONTROL_PAYLOAD_TOO_LARGE);
-    CHANGE_STATE(FLIP_ERROR);
+
+  if (error_code_)
     return;
-  }
+
   ExpandControlFrameBuffer(remaining_control_payload_);
   CHANGE_STATE(FLIP_CONTROL_FRAME_PAYLOAD);
 }
diff --git a/net/flip/flip_network_transaction_unittest.cc b/net/flip/flip_network_transaction_unittest.cc
index 0a6a8f2..1abcdc3 100644
--- a/net/flip/flip_network_transaction_unittest.cc
+++ b/net/flip/flip_network_transaction_unittest.cc
@@ -112,6 +112,19 @@ static const unsigned char kGetSyn[] = {
   0x00, 0x08, 'H', 'T', 'T', 'P', '/', '1', '.', '1',
 };
 
+static const unsigned char kGetSynCompressed[] = {
+  0x80, 0x01, 0x00, 0x01, 0x01, 0x00, 0x00, 0x43,
+  0x00, 0x00, 0x00, 0x01, 0xc0, 0x00, 0x78, 0xbb,
+  0xdf, 0xa2, 0x51, 0xb2, 0x62, 0x60, 0x66, 0x60,
+  0xcb, 0x05, 0xe6, 0xc3, 0xfc, 0x14, 0x06, 0x66,
+  0x77, 0xd7, 0x10, 0x06, 0x66, 0x90, 0xa0, 0x58,
+  0x46, 0x49, 0x49, 0x81, 0x95, 0xbe, 0x3e, 0x30,
+  0xe2, 0xf5, 0xd2, 0xf3, 0xf3, 0xd3, 0x73, 0x52,
+  0xf5, 0x92, 0xf3, 0x73, 0xf5, 0x19, 0xd8, 0xa1,
+  0x1a, 0x19, 0x38, 0x60, 0xe6, 0x01, 0x00, 0x00,
+  0x00, 0xff, 0xff
+};
+
 static const unsigned char kGetSynReply[] = {
   0x80, 0x01, 0x00, 0x02,                                        // header
   0x00, 0x00, 0x00, 0x45,
@@ -249,8 +262,8 @@ class DelayedSocketData : public StaticSocketDataProvider,
 class FlipNetworkTransactionTest : public PlatformTest {
  protected:
   virtual void SetUp() {
-    // Disable compression on this test.
-    flip::FlipFramer::set_enable_compression_default(false);
+    // By default, all tests turn off compression.
+    EnableCompression(false);
   }
 
   virtual void TearDown() {
@@ -268,6 +281,10 @@ class FlipNetworkTransactionTest : public PlatformTest {
     HttpResponseInfo response_info;
   };
 
+  void EnableCompression(bool enabled) {
+    flip::FlipFramer::set_enable_compression_default(enabled);
+  }
+
   TransactionHelperResult TransactionHelper(const HttpRequestInfo& request,
                                             DelayedSocketData* data,
                                             LoadLog* log) {
@@ -674,6 +691,52 @@ TEST_F(FlipNetworkTransactionTest, InvalidSynReply) {
   }
 }
 
+// Verify that we don't crash on some corrupt frames.
+TEST_F(FlipNetworkTransactionTest, CorruptFrameSessionError) {
+  static const unsigned char kSynReplyMassiveLength[] = {
+    0x80, 0x01, 0x00, 0x02,
+    0x0f, 0x11, 0x11, 0x26,   // This is the length field with a big number
+    0x00, 0x00, 0x00, 0x01,
+    0x00, 0x00, 0x00, 0x04,
+    0x00, 0x06, 's', 't', 'a', 't', 'u', 's',
+    0x00, 0x03, '2', '0', '0',
+    0x00, 0x03, 'u', 'r', 'l',
+    0x00, 0x0a, '/', 'i', 'n', 'd', 'e', 'x', '.', 'p', 'h', 'p',
+  };
+
+  struct SynReplyTests {
+    const unsigned char* syn_reply;
+    int syn_reply_length;
+  } test_cases[] = {
+    { kSynReplyMassiveLength, arraysize(kSynReplyMassiveLength) }
+  };
+
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(test_cases); ++i) {
+    MockWrite writes[] = {
+      MockWrite(true, reinterpret_cast<const char*>(kGetSyn),
+                arraysize(kGetSyn)),
+      MockWrite(true, 0, 0)  // EOF
+    };
+
+    MockRead reads[] = {
+      MockRead(true, reinterpret_cast<const char*>(test_cases[i].syn_reply),
+               test_cases[i].syn_reply_length),
+      MockRead(true, reinterpret_cast<const char*>(kGetBodyFrame),
+               arraysize(kGetBodyFrame)),
+      MockRead(true, 0, 0)  // EOF
+    };
+
+    HttpRequestInfo request;
+    request.method = "GET";
+    request.url = GURL("http://www.google.com/");
+    request.load_flags = 0;
+    scoped_refptr<DelayedSocketData> data(
+        new DelayedSocketData(reads, 1, writes));
+    TransactionHelperResult out = TransactionHelper(request, data.get(), NULL);
+    EXPECT_EQ(ERR_FLIP_PROTOCOL_ERROR, out.rv);
+  }
+}
+
 TEST_F(FlipNetworkTransactionTest, DISABLED_ServerPush) {
   // Reply with the X-Associated-Content header.
   static const unsigned char syn_reply[] = {
@@ -921,6 +984,39 @@ TEST_F(FlipNetworkTransactionTest, DISABLED_ConnectFailure) {
   }
 }
 
+// In this test, we enable compression, but get a uncompressed SynReply from
+// the server.  Verify that teardown is all clean.
+TEST_F(FlipNetworkTransactionTest, DecompressFailureOnSynReply) {
+  MockWrite writes[] = {
+    MockWrite(true, reinterpret_cast<const char*>(kGetSynCompressed),
+              arraysize(kGetSynCompressed)),
+    MockWrite(true, 0, 0)  // EOF
+  };
+
+  MockRead reads[] = {
+    MockRead(true, reinterpret_cast<const char*>(kGetSynReply),
+             arraysize(kGetSynReply)),
+    MockRead(true, reinterpret_cast<const char*>(kGetBodyFrame),
+             arraysize(kGetBodyFrame)),
+    MockRead(true, 0, 0)  // EOF
+  };
+
+  // For this test, we turn on the normal compression.
+  EnableCompression(true);
+
+  HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("http://www.google.com/");
+  request.load_flags = 0;
+  scoped_refptr<DelayedSocketData> data(
+      new DelayedSocketData(reads, 1, writes));
+  TransactionHelperResult out = TransactionHelper(request, data.get(), NULL);
+  EXPECT_EQ(ERR_SYN_REPLY_NOT_RECEIVED, out.rv);
+  data->Reset();
+
+  EnableCompression(false);
+}
+
 // Test that the LoadLog contains good data for a simple GET request.
 TEST_F(FlipNetworkTransactionTest, LoadLog) {
   MockWrite writes[] = {
diff --git a/net/flip/flip_session.cc b/net/flip/flip_session.cc
index b5226b3..d91c37c 100644
--- a/net/flip/flip_session.cc
+++ b/net/flip/flip_session.cc
@@ -538,6 +538,12 @@ void FlipSession::OnReadComplete(int bytes_read) {
     return;
   }
 
+  // The FlipFramer will use callbacks onto |this| as it parses frames.
+  // When errors occur, those callbacks can lead to teardown of all references
+  // to |this|, so maintain a reference to self during this call for safe
+  // cleanup.
+  scoped_refptr<FlipSession> self(this);
+
   char *data = read_buffer_->data();
   while (bytes_read &&
          flip_framer_.error_code() == flip::FlipFramer::FLIP_NO_ERROR) {
@@ -548,7 +554,8 @@ void FlipSession::OnReadComplete(int bytes_read) {
       flip_framer_.Reset();
   }
 
-  ReadSocket();
+  if (state_ != CLOSED)
+    ReadSocket();
 }
 
 void FlipSession::OnWriteComplete(int result) {
@@ -607,9 +614,16 @@ void FlipSession::ReadSocket() {
   if (read_pending_)
     return;
 
+  if (state_ == CLOSED) {
+    NOTREACHED();
+    return;
+  }
+
+  CHECK(connection_.get());
+  CHECK(connection_->socket());
   int bytes_read = connection_->socket()->Read(read_buffer_.get(),
-                                              kReadBufferSize,
-                                              &read_callback_);
+                                               kReadBufferSize,
+                                               &read_callback_);
   switch (bytes_read) {
     case 0:
       // Socket is closed!
@@ -819,7 +833,7 @@ void FlipSession::GetSSLInfo(SSLInfo* ssl_info) {
 
 void FlipSession::OnError(flip::FlipFramer* framer) {
   LOG(ERROR) << "FlipSession error: " << framer->error_code();
-  CloseSessionOnError(net::ERR_UNEXPECTED);
+  CloseSessionOnError(net::ERR_FLIP_PROTOCOL_ERROR);
 }
 
 void FlipSession::OnStreamFrameData(flip::FlipStreamId stream_id,