Merge changes to SpdyFramer from server code (#88):

* Improve handling of 0 length frames in SpdyFramer.
  Add checks to ensure that ProcessInput doesn't stall halfway through even
  if there is input remaining.


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

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

3 files changed, 100 insertions, 48 deletions

diff --git a/net/spdy/spdy_framer.cc b/net/spdy/spdy_framer.cc
index 001b652..543703c 100644
--- a/net/spdy/spdy_framer.cc
+++ b/net/spdy/spdy_framer.cc
@@ -66,17 +66,24 @@ const size_t SpdyFramer::kMaxControlFrameSize = 16 * 1024;
 
 
 #ifdef DEBUG_SPDY_STATE_CHANGES
-#define CHANGE_STATE(newstate) \
-{ \
-  do { \
-    LOG(INFO) << "Changing state from: " \
-      << StateToString(state_) \
-      << " to " << StateToString(newstate) << "\n"; \
-    state_ = newstate; \
-  } while (false); \
-}
+#define CHANGE_STATE(newstate)                                  \
+  do {                                                          \
+    LOG(INFO) << "Changing state from: "                        \
+              << StateToString(state_)                          \
+              << " to " << StateToString(newstate) << "\n";     \
+    DCHECK(state_ != SPDY_ERROR);                               \
+    DCHECK_EQ(previous_state_, state_);                         \
+    previous_state_ = state_;                                   \
+    state_ = newstate;                                          \
+  } while (false)
 #else
-#define CHANGE_STATE(newstate) (state_ = newstate)
+#define CHANGE_STATE(newstate)                                  \
+  do {                                                          \
+    DCHECK(state_ != SPDY_ERROR);                               \
+    DCHECK_EQ(previous_state_, state_);                         \
+    previous_state_ = state_;                                   \
+    state_ = newstate;                                          \
+  } while (false)
 #endif
 
 SettingsFlagsAndId SettingsFlagsAndId::FromWireFormat(int version,
@@ -143,6 +150,7 @@ SpdyFramer::~SpdyFramer() {
 
 void SpdyFramer::Reset() {
   state_ = SPDY_RESET;
+  previous_state_ = SPDY_RESET;
   error_code_ = SPDY_NO_ERROR;
   remaining_data_ = 0;
   remaining_control_payload_ = 0;
@@ -273,7 +281,8 @@ size_t SpdyFramer::ProcessInput(const char* data, size_t len) {
   DCHECK(data);
 
   size_t original_len = len;
-  while (len != 0) {
+  do {
+    previous_state_ = state_;
     switch (state_) {
       case SPDY_ERROR:
       case SPDY_DONE:
@@ -282,14 +291,16 @@ size_t SpdyFramer::ProcessInput(const char* data, size_t len) {
       case SPDY_AUTO_RESET:
       case SPDY_RESET:
         Reset();
-        CHANGE_STATE(SPDY_READING_COMMON_HEADER);
-        continue;
+        if (len > 0) {
+          CHANGE_STATE(SPDY_READING_COMMON_HEADER);
+        }
+        break;
 
       case SPDY_READING_COMMON_HEADER: {
         size_t bytes_read = ProcessCommonHeader(data, len);
         len -= bytes_read;
         data += bytes_read;
-        continue;
+        break;
       }
 
       case SPDY_CONTROL_FRAME_BEFORE_HEADER_BLOCK: {
@@ -308,36 +319,37 @@ size_t SpdyFramer::ProcessInput(const char* data, size_t len) {
         int bytes_read = ProcessControlFrameBeforeHeaderBlock(data, len);
         len -= bytes_read;
         data += bytes_read;
-        continue;
+        break;
       }
 
       case SPDY_SETTINGS_FRAME_PAYLOAD: {
         int bytes_read = ProcessSettingsFramePayload(data, len);
         len -= bytes_read;
         data += bytes_read;
-        continue;
+        break;
       }
 
       case SPDY_CONTROL_FRAME_HEADER_BLOCK: {
         int bytes_read = ProcessControlFrameHeaderBlock(data, len);
         len -= bytes_read;
         data += bytes_read;
-        continue;
+        break;
       }
 
       case SPDY_CREDENTIAL_FRAME_PAYLOAD: {
         size_t bytes_read = ProcessCredentialFramePayload(data, len);
         len -= bytes_read;
         data += bytes_read;
-        continue;
+        break;
       }
 
       case SPDY_CONTROL_FRAME_PAYLOAD: {
         size_t bytes_read = ProcessControlFramePayload(data, len);
         len -= bytes_read;
         data += bytes_read;
+        break;
       }
-        // intentional fallthrough
+
       case SPDY_IGNORE_REMAINING_PAYLOAD:
         // control frame has too-large payload
         // intentional fallthrough
@@ -345,7 +357,7 @@ size_t SpdyFramer::ProcessInput(const char* data, size_t len) {
         size_t bytes_read = ProcessDataFramePayload(data, len);
         len -= bytes_read;
         data += bytes_read;
-        continue;
+        break;
       }
       default:
         LOG(ERROR) << "Invalid value for " << display_protocol_
@@ -355,8 +367,17 @@ size_t SpdyFramer::ProcessInput(const char* data, size_t len) {
         // from a callback it calls.
         goto bottom;
     }
-  }
+  } while (state_ != previous_state_);
  bottom:
+  DCHECK(len == 0 || state_ == SPDY_ERROR);
+  if (current_frame_len_ == 0 &&
+      remaining_data_ == 0 &&
+      remaining_control_payload_ == 0 &&
+      remaining_control_header_ == 0) {
+    DCHECK(state_ == SPDY_RESET || state_ == SPDY_ERROR)
+        << "State: " << StateToString(state_);
+  }
+
   return original_len - len;
 }
 
@@ -513,6 +534,10 @@ void SpdyFramer::ProcessControlFrameHeader() {
       break;
   }
 
+  if (state_ == SPDY_ERROR) {
+    return;
+  }
+
   remaining_control_payload_ = current_control_frame.length();
   const size_t total_frame_size =
       remaining_control_payload_ + SpdyFrame::kHeaderSize;
@@ -642,26 +667,25 @@ void SpdyFramer::WriteHeaderBlock(SpdyFrameBuilder* frame,
 size_t SpdyFramer::ProcessControlFrameBeforeHeaderBlock(const char* data,
                                                         size_t len) {
   DCHECK_EQ(SPDY_CONTROL_FRAME_BEFORE_HEADER_BLOCK, state_);
-  DCHECK_GT(remaining_control_header_, 0u);
   size_t original_len = len;
 
-  if (remaining_control_header_) {
+  if (remaining_control_header_ > 0) {
     size_t bytes_read = UpdateCurrentFrameBuffer(&data, &len,
                                                  remaining_control_header_);
     remaining_control_header_ -= bytes_read;
-    if (remaining_control_header_ == 0) {
-      SpdyControlFrame control_frame(current_frame_buffer_.get(), false);
-      DCHECK(control_frame.type() == SYN_STREAM ||
-             control_frame.type() == SYN_REPLY ||
-             control_frame.type() == HEADERS ||
-             control_frame.type() == SETTINGS);
-      visitor_->OnControl(&control_frame);
+  }
+  if (remaining_control_header_ == 0) {
+    SpdyControlFrame control_frame(current_frame_buffer_.get(), false);
+    DCHECK(control_frame.type() == SYN_STREAM ||
+           control_frame.type() == SYN_REPLY ||
+           control_frame.type() == HEADERS ||
+           control_frame.type() == SETTINGS);
+    visitor_->OnControl(&control_frame);
 
-      if (control_frame.type() == SETTINGS) {
-        CHANGE_STATE(SPDY_SETTINGS_FRAME_PAYLOAD);
-      } else {
-        CHANGE_STATE(SPDY_CONTROL_FRAME_HEADER_BLOCK);
-      }
+    if (control_frame.type() == SETTINGS) {
+      CHANGE_STATE(SPDY_SETTINGS_FRAME_PAYLOAD);
+    } else {
+      CHANGE_STATE(SPDY_CONTROL_FRAME_HEADER_BLOCK);
     }
   }
   return original_len - len;
@@ -675,21 +699,24 @@ size_t SpdyFramer::ProcessControlFrameHeaderBlock(const char* data,
                                                   size_t data_len) {
   DCHECK_EQ(SPDY_CONTROL_FRAME_HEADER_BLOCK, state_);
   SpdyControlFrame control_frame(current_frame_buffer_.get(), false);
+
   bool processed_successfully = true;
   DCHECK(control_frame.type() == SYN_STREAM ||
          control_frame.type() == SYN_REPLY ||
          control_frame.type() == HEADERS);
   size_t process_bytes = std::min(data_len, remaining_control_payload_);
-  DCHECK_GT(process_bytes, 0u);
 
-  if (enable_compression_) {
-    processed_successfully = IncrementallyDecompressControlFrameHeaderData(
-        &control_frame, data, process_bytes);
-  } else {
-    processed_successfully = IncrementallyDeliverControlFrameHeaderData(
-        &control_frame, data, process_bytes);
+  if (process_bytes > 0) {
+    if (enable_compression_) {
+      processed_successfully = IncrementallyDecompressControlFrameHeaderData(
+          &control_frame, data, process_bytes);
+    } else {
+      processed_successfully = IncrementallyDeliverControlFrameHeaderData(
+          &control_frame, data, process_bytes);
+    }
+    remaining_control_payload_ -= process_bytes;
+    remaining_data_ -= process_bytes;
   }
-  remaining_control_payload_ -= process_bytes;
 
   // Handle the case that there is no futher data in this frame.
   if (remaining_control_payload_ == 0 && processed_successfully) {
@@ -704,7 +731,7 @@ size_t SpdyFramer::ProcessControlFrameHeaderBlock(const char* data,
                                   NULL, 0);
     }
 
-    CHANGE_STATE(SPDY_RESET);
+    CHANGE_STATE(SPDY_AUTO_RESET);
   }
 
   // Handle error.
@@ -723,7 +750,6 @@ size_t SpdyFramer::ProcessSettingsFramePayload(const char* data,
   DCHECK_EQ(control_frame.type(), SETTINGS);
   size_t unprocessed_bytes = std::min(data_len, remaining_control_payload_);
   size_t processed_bytes = 0;
-  DCHECK_GT(unprocessed_bytes, 0u);
 
   // Loop over our incoming data.
   while (unprocessed_bytes > 0) {
@@ -857,7 +883,7 @@ size_t SpdyFramer::ProcessDataFramePayload(const char* data, size_t len) {
   size_t original_len = len;
 
   SpdyDataFrame current_data_frame(current_frame_buffer_.get(), false);
-  if (remaining_data_) {
+  if (remaining_data_ > 0) {
     size_t amount_to_forward = std::min(remaining_data_, len);
     if (amount_to_forward && state_ != SPDY_IGNORE_REMAINING_PAYLOAD) {
         // Only inform the visitor if there is data.
@@ -876,7 +902,9 @@ size_t SpdyFramer::ProcessDataFramePayload(const char* data, size_t len) {
         current_data_frame.flags() & DATA_FLAG_FIN) {
       visitor_->OnStreamFrameData(current_data_frame.stream_id(), NULL, 0);
     }
-  } else {
+  }
+
+  if (remaining_data_ == 0) {
     CHANGE_STATE(SPDY_AUTO_RESET);
   }
   return original_len - len;
diff --git a/net/spdy/spdy_framer.h b/net/spdy/spdy_framer.h
index 5a31c48..7609491 100644
--- a/net/spdy/spdy_framer.h
+++ b/net/spdy/spdy_framer.h
@@ -520,6 +520,7 @@ class NET_EXPORT_PRIVATE SpdyFramer {
   static const size_t kMaxControlFrameSize;
 
   SpdyState state_;
+  SpdyState previous_state_;
   SpdyError error_code_;
   size_t remaining_data_;
 
diff --git a/net/spdy/spdy_framer_test.cc b/net/spdy/spdy_framer_test.cc
index b0be6b4..b03fcb8 100644
--- a/net/spdy/spdy_framer_test.cc
+++ b/net/spdy/spdy_framer_test.cc
@@ -2631,11 +2631,34 @@ TEST_P(SpdyFramerTest, DataFrameFlags) {
 
     size_t frame_size = frame->length() + SpdyFrame::kHeaderSize;
     framer.ProcessInput(frame->data(), frame_size);
-    EXPECT_EQ(SpdyFramer::SPDY_FORWARD_STREAM_FRAME, framer.state());
+    EXPECT_EQ(SpdyFramer::SPDY_RESET, framer.state());
     EXPECT_EQ(SpdyFramer::SPDY_NO_ERROR, framer.error_code());
   }
 }
 
+TEST_P(SpdyFramerTest, EmptySynStream) {
+  SpdyHeaderBlock headers;
+
+  testing::StrictMock<test::MockVisitor> visitor;
+  SpdyFramer framer(spdy_version_);
+  framer.set_visitor(&visitor);
+
+  scoped_ptr<SpdySynStreamControlFrame>
+      frame(framer.CreateSynStream(1, 0, 1, 0, CONTROL_FLAG_NONE, true,
+                                   &headers));
+  // Adjust size to remove the name/value block.
+  frame->set_length(
+      SpdySynStreamControlFrame::size() - SpdyFrame::kHeaderSize);
+
+  EXPECT_CALL(visitor, OnControl(_));
+  EXPECT_CALL(visitor, OnControlFrameHeaderData(1, NULL, 0));
+
+  size_t frame_size = frame->length() + SpdyFrame::kHeaderSize;
+  framer.ProcessInput(frame->data(), frame_size);
+  EXPECT_EQ(SpdyFramer::SPDY_RESET, framer.state());
+  EXPECT_EQ(SpdyFramer::SPDY_NO_ERROR, framer.error_code());
+}
+
 TEST_P(SpdyFramerTest, SettingsFlagsAndId) {
   const uint32 kId = 0x020304;
   const uint32 kFlags = 0x01;