Revert 133226 - 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

TBR=rch@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10162014

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

3 files changed, 48 insertions, 100 deletions

diff --git a/net/spdy/spdy_framer.cc b/net/spdy/spdy_framer.cc
index 543703c..001b652 100644
--- a/net/spdy/spdy_framer.cc
+++ b/net/spdy/spdy_framer.cc
@@ -66,24 +66,17 @@ 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";     \
-    DCHECK(state_ != SPDY_ERROR);                               \
-    DCHECK_EQ(previous_state_, state_);                         \
-    previous_state_ = state_;                                   \
-    state_ = newstate;                                          \
-  } while (false)
+#define CHANGE_STATE(newstate) \
+{ \
+  do { \
+    LOG(INFO) << "Changing state from: " \
+      << StateToString(state_) \
+      << " to " << StateToString(newstate) << "\n"; \
+    state_ = newstate; \
+  } while (false); \
+}
 #else
-#define CHANGE_STATE(newstate)                                  \
-  do {                                                          \
-    DCHECK(state_ != SPDY_ERROR);                               \
-    DCHECK_EQ(previous_state_, state_);                         \
-    previous_state_ = state_;                                   \
-    state_ = newstate;                                          \
-  } while (false)
+#define CHANGE_STATE(newstate) (state_ = newstate)
 #endif
 
 SettingsFlagsAndId SettingsFlagsAndId::FromWireFormat(int version,
@@ -150,7 +143,6 @@ SpdyFramer::~SpdyFramer() {
 
 void SpdyFramer::Reset() {
   state_ = SPDY_RESET;
-  previous_state_ = SPDY_RESET;
   error_code_ = SPDY_NO_ERROR;
   remaining_data_ = 0;
   remaining_control_payload_ = 0;
@@ -281,8 +273,7 @@ size_t SpdyFramer::ProcessInput(const char* data, size_t len) {
   DCHECK(data);
 
   size_t original_len = len;
-  do {
-    previous_state_ = state_;
+  while (len != 0) {
     switch (state_) {
       case SPDY_ERROR:
       case SPDY_DONE:
@@ -291,16 +282,14 @@ size_t SpdyFramer::ProcessInput(const char* data, size_t len) {
       case SPDY_AUTO_RESET:
       case SPDY_RESET:
         Reset();
-        if (len > 0) {
-          CHANGE_STATE(SPDY_READING_COMMON_HEADER);
-        }
-        break;
+        CHANGE_STATE(SPDY_READING_COMMON_HEADER);
+        continue;
 
       case SPDY_READING_COMMON_HEADER: {
         size_t bytes_read = ProcessCommonHeader(data, len);
         len -= bytes_read;
         data += bytes_read;
-        break;
+        continue;
       }
 
       case SPDY_CONTROL_FRAME_BEFORE_HEADER_BLOCK: {
@@ -319,37 +308,36 @@ size_t SpdyFramer::ProcessInput(const char* data, size_t len) {
         int bytes_read = ProcessControlFrameBeforeHeaderBlock(data, len);
         len -= bytes_read;
         data += bytes_read;
-        break;
+        continue;
       }
 
       case SPDY_SETTINGS_FRAME_PAYLOAD: {
         int bytes_read = ProcessSettingsFramePayload(data, len);
         len -= bytes_read;
         data += bytes_read;
-        break;
+        continue;
       }
 
       case SPDY_CONTROL_FRAME_HEADER_BLOCK: {
         int bytes_read = ProcessControlFrameHeaderBlock(data, len);
         len -= bytes_read;
         data += bytes_read;
-        break;
+        continue;
       }
 
       case SPDY_CREDENTIAL_FRAME_PAYLOAD: {
         size_t bytes_read = ProcessCredentialFramePayload(data, len);
         len -= bytes_read;
         data += bytes_read;
-        break;
+        continue;
       }
 
       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
@@ -357,7 +345,7 @@ size_t SpdyFramer::ProcessInput(const char* data, size_t len) {
         size_t bytes_read = ProcessDataFramePayload(data, len);
         len -= bytes_read;
         data += bytes_read;
-        break;
+        continue;
       }
       default:
         LOG(ERROR) << "Invalid value for " << display_protocol_
@@ -367,17 +355,8 @@ 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_);
   }
-
+ bottom:
   return original_len - len;
 }
 
@@ -534,10 +513,6 @@ 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;
@@ -667,25 +642,26 @@ 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_ > 0) {
+  if (remaining_control_header_) {
     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;
@@ -699,24 +675,21 @@ 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 (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;
+  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;
 
   // Handle the case that there is no futher data in this frame.
   if (remaining_control_payload_ == 0 && processed_successfully) {
@@ -731,7 +704,7 @@ size_t SpdyFramer::ProcessControlFrameHeaderBlock(const char* data,
                                   NULL, 0);
     }
 
-    CHANGE_STATE(SPDY_AUTO_RESET);
+    CHANGE_STATE(SPDY_RESET);
   }
 
   // Handle error.
@@ -750,6 +723,7 @@ 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) {
@@ -883,7 +857,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_ > 0) {
+  if (remaining_data_) {
     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.
@@ -902,9 +876,7 @@ 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);
     }
-  }
-
-  if (remaining_data_ == 0) {
+  } else {
     CHANGE_STATE(SPDY_AUTO_RESET);
   }
   return original_len - len;
diff --git a/net/spdy/spdy_framer.h b/net/spdy/spdy_framer.h
index 7609491..5a31c48 100644
--- a/net/spdy/spdy_framer.h
+++ b/net/spdy/spdy_framer.h
@@ -520,7 +520,6 @@ 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 b03fcb8..b0be6b4 100644
--- a/net/spdy/spdy_framer_test.cc
+++ b/net/spdy/spdy_framer_test.cc
@@ -2631,34 +2631,11 @@ TEST_P(SpdyFramerTest, DataFrameFlags) {
 
     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_FORWARD_STREAM_FRAME, 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;