Cleanup control_frame_fields_ if coalescer rejected the headers.

net/spdy/buffered_spdy_framer_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/spdy/buffered_spdy_framer.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/logging.h"
@@ skipping 109 matching lines @@
     altsvc_stream_id_ = stream_id;
     origin.CopyToString(&altsvc_origin_);
     altsvc_vector_ = altsvc_vector;
   }
 
   bool OnUnknownFrame(SpdyStreamId stream_id, uint8_t frame_type) override {
     return true;
   }
 
   // Convenience function which runs a framer simulation with particular input.
-  void SimulateInFramer(const unsigned char* input, size_t size) {
+  void SimulateInFramer(const SpdySerializedFrame& frame) {

[Maks Orlovich, 2017/02/07 18:59:48]

This could/should arguably be a separate cleanup CL, please let me know if you
would prefer it that way. 

Speaking of other CLs, header_buffer_, header_buffer_valid_, header_stream_id_
in BufferedSpdyFramer all seem dead to me --- worth a cleanup CL?

[Bence, 2017/02/07 20:01:17]

I really do not mind doing multiple things in the same CL as long as they are
not too complicated.  So this is okay with me.
 
Totally, yes please!

+    const char* input_ptr = frame.data();
+    size_t size = frame.size();

[Bence, 2017/02/07 20:01:17]

No need for named variable |size| which is only used once two lines below. 
Instead write:
size_t input_remaining = frame.size();

[Maks Orlovich, 2017/02/07 20:40:38]

Done.

     buffered_spdy_framer_.set_visitor(this);
     size_t input_remaining = size;
-    const char* input_ptr = reinterpret_cast<const char*>(input);
     while (input_remaining > 0 &&
            buffered_spdy_framer_.spdy_framer_error() ==
                SpdyFramer::SPDY_NO_ERROR) {
       // To make the tests more interesting, we feed random (amd small) chunks
       // into the framer.  This simulates getting strange-sized reads from
       // the socket.
       const size_t kMaxReadSize = 32;
       size_t bytes_read =
           (rand() % std::min(input_remaining, kMaxReadSize)) + 1;
       size_t bytes_processed =
@@ skipping 36 matching lines @@
 class BufferedSpdyFramerTest : public PlatformTest {};
 
 TEST_F(BufferedSpdyFramerTest, OnSetting) {
   SpdyFramer framer(SpdyFramer::ENABLE_COMPRESSION);
   SpdySettingsIR settings_ir;
   settings_ir.AddSetting(SETTINGS_INITIAL_WINDOW_SIZE, 2);
   settings_ir.AddSetting(SETTINGS_MAX_CONCURRENT_STREAMS, 3);
   SpdySerializedFrame control_frame(framer.SerializeSettings(settings_ir));
   TestBufferedSpdyVisitor visitor;
 
-  visitor.SimulateInFramer(
-      reinterpret_cast<unsigned char*>(control_frame.data()),
-      control_frame.size());
+  visitor.SimulateInFramer(control_frame);
   EXPECT_EQ(0, visitor.error_count_);
   EXPECT_EQ(2, visitor.setting_count_);
 }
 
 TEST_F(BufferedSpdyFramerTest, HeaderListTooLarge) {
   SpdyHeaderBlock headers;
   std::string long_header_value(256 * 1024, 'x');
   headers["foo"] = long_header_value;
   BufferedSpdyFramer framer;
   std::unique_ptr<SpdySerializedFrame> control_frame(
       framer.CreateHeaders(1,  // stream_id
                            CONTROL_FLAG_NONE,
                            255,  // weight
                            std::move(headers)));
   EXPECT_TRUE(control_frame);
 
   TestBufferedSpdyVisitor visitor;
-  visitor.SimulateInFramer(
-      reinterpret_cast<unsigned char*>(control_frame.get()->data()),
-      control_frame.get()->size());
+  visitor.SimulateInFramer(*control_frame);
 
   EXPECT_EQ(1, visitor.error_count_);
   EXPECT_EQ(0, visitor.headers_frame_count_);
   EXPECT_EQ(0, visitor.push_promise_frame_count_);
   EXPECT_EQ(SpdyHeaderBlock(), visitor.headers_);
 }
 
+TEST_F(BufferedSpdyFramerTest, ValidHeadersAfterInvalidHeaders) {
+  SpdyHeaderBlock headers;
+  headers["invalid"] = "\r\n\r\n";
+
+  SpdyHeaderBlock headers2;
+  headers["alpha"] = "beta";
+
+  SpdyTestUtil spdy_test_util;
+  SpdySerializedFrame headers_frame(
+      spdy_test_util.ConstructSpdyReply(1, std::move(headers)));
+  SpdySerializedFrame headers_frame2(
+      spdy_test_util.ConstructSpdyReply(2, std::move(headers2)));
+
+  TestBufferedSpdyVisitor visitor;
+  visitor.SimulateInFramer(headers_frame);
+  EXPECT_EQ(1, visitor.error_count_);
+  EXPECT_EQ(0, visitor.headers_frame_count_);
+
+  visitor.SimulateInFramer(headers_frame2);
+  EXPECT_EQ(1, visitor.error_count_);
+  EXPECT_EQ(1, visitor.headers_frame_count_);
+}
+
 TEST_F(BufferedSpdyFramerTest, ReadHeadersHeaderBlock) {
   SpdyHeaderBlock headers;
   headers["alpha"] = "beta";
   headers["gamma"] = "delta";
   BufferedSpdyFramer framer;
   std::unique_ptr<SpdySerializedFrame> control_frame(
       framer.CreateHeaders(1,  // stream_id
                            CONTROL_FLAG_NONE,
                            255,  // weight
                            headers.Clone()));
   EXPECT_TRUE(control_frame.get() != NULL);
 
   TestBufferedSpdyVisitor visitor;
-  visitor.SimulateInFramer(
-      reinterpret_cast<unsigned char*>(control_frame.get()->data()),
-      control_frame.get()->size());
+  visitor.SimulateInFramer(*control_frame);
   EXPECT_EQ(0, visitor.error_count_);
   EXPECT_EQ(1, visitor.headers_frame_count_);
   EXPECT_EQ(0, visitor.push_promise_frame_count_);
   EXPECT_EQ(headers, visitor.headers_);
 }
 
 TEST_F(BufferedSpdyFramerTest, ReadPushPromiseHeaderBlock) {
   SpdyHeaderBlock headers;
   headers["alpha"] = "beta";
   headers["gamma"] = "delta";
   BufferedSpdyFramer framer;
   std::unique_ptr<SpdySerializedFrame> control_frame(
       framer.CreatePushPromise(1, 2, headers.Clone()));
   EXPECT_TRUE(control_frame.get() != NULL);
 
   TestBufferedSpdyVisitor visitor;
-  visitor.SimulateInFramer(
-      reinterpret_cast<unsigned char*>(control_frame.get()->data()),
-      control_frame.get()->size());
+  visitor.SimulateInFramer(*control_frame);
   EXPECT_EQ(0, visitor.error_count_);
   EXPECT_EQ(0, visitor.headers_frame_count_);
   EXPECT_EQ(1, visitor.push_promise_frame_count_);
   EXPECT_EQ(headers, visitor.headers_);
   EXPECT_EQ(1u, visitor.header_stream_id_);
   EXPECT_EQ(2u, visitor.promised_stream_id_);
 }
 
 TEST_F(BufferedSpdyFramerTest, GoAwayDebugData) {
   BufferedSpdyFramer framer;
   std::unique_ptr<SpdySerializedFrame> goaway_frame(
       framer.CreateGoAway(2u, ERROR_CODE_FRAME_SIZE_ERROR, "foo"));
 
   TestBufferedSpdyVisitor visitor;
-  visitor.SimulateInFramer(
-      reinterpret_cast<unsigned char*>(goaway_frame.get()->data()),
-      goaway_frame.get()->size());
+  visitor.SimulateInFramer(*goaway_frame);
   EXPECT_EQ(0, visitor.error_count_);
   EXPECT_EQ(1, visitor.goaway_count_);
   EXPECT_EQ(2u, visitor.goaway_last_accepted_stream_id_);
   EXPECT_EQ(ERROR_CODE_FRAME_SIZE_ERROR, visitor.goaway_error_code_);
   EXPECT_EQ("foo", visitor.goaway_debug_data_);
 }
 
 TEST_F(BufferedSpdyFramerTest, OnAltSvc) {
   const SpdyStreamId altsvc_stream_id(1);
   const char altsvc_origin[] = "https://www.example.org";
   SpdyAltSvcIR altsvc_ir(altsvc_stream_id);
   SpdyAltSvcWireFormat::AlternativeService alternative_service(
       "quic", "alternative.example.org", 443, 86400,
       SpdyAltSvcWireFormat::VersionVector());
   altsvc_ir.add_altsvc(alternative_service);
   altsvc_ir.set_origin(altsvc_origin);
   BufferedSpdyFramer framer;
   SpdySerializedFrame altsvc_frame(framer.SerializeFrame(altsvc_ir));
 
   TestBufferedSpdyVisitor visitor;
-  visitor.SimulateInFramer(
-      reinterpret_cast<unsigned char*>(altsvc_frame.data()),
-      altsvc_frame.size());
+  visitor.SimulateInFramer(altsvc_frame);
   EXPECT_EQ(0, visitor.error_count_);
   EXPECT_EQ(1, visitor.altsvc_count_);
   EXPECT_EQ(altsvc_stream_id, visitor.altsvc_stream_id_);
   EXPECT_EQ(altsvc_origin, visitor.altsvc_origin_);
   ASSERT_EQ(1u, visitor.altsvc_vector_.size());
   EXPECT_EQ(alternative_service, visitor.altsvc_vector_[0]);
 }
 
 }  // namespace net