Add a new SpdyStream::Delegate method to handle trailers

This CL adds a new SpdyStream::Delegate method to surface trailers to
Delegate's implementations. Before this CL, receiving trailers will trigger
a spdy protocol error. However, SpdyHttpStream does not implement
this delegate method as of this CL, so trailers are ignored.

BUG=481033

Review URL: https://codereview.chromium.org/1272283003

Cr-Commit-Position: refs/heads/master@{#342859}

13 files changed, 159 insertions, 16 deletions

diff --git a/net/spdy/spdy_http_stream.cc b/net/spdy/spdy_http_stream.cc
index f3da77b..fd708c8 100644
--- a/net/spdy/spdy_http_stream.cc
+++ b/net/spdy/spdy_http_stream.cc
@@ -365,6 +365,9 @@ void SpdyHttpStream::OnDataSent() {
   ReadAndSendRequestBodyData();
 }
 
+// TODO(xunjieli): Maybe do something with the trailers. crbug.com/422958.
+void SpdyHttpStream::OnTrailers(const SpdyHeaderBlock& trailers) {}
+
 void SpdyHttpStream::OnClose(int status) {
   if (stream_.get()) {
     stream_closed_ = true;
diff --git a/net/spdy/spdy_http_stream.h b/net/spdy/spdy_http_stream.h
index 333454b..0aa54f0 100644
--- a/net/spdy/spdy_http_stream.h
+++ b/net/spdy/spdy_http_stream.h
@@ -79,6 +79,7 @@ class NET_EXPORT_PRIVATE SpdyHttpStream : public SpdyStream::Delegate,
       const SpdyHeaderBlock& response_headers) override;
   void OnDataReceived(scoped_ptr<SpdyBuffer> buffer) override;
   void OnDataSent() override;
+  void OnTrailers(const SpdyHeaderBlock& trailers) override;
   void OnClose(int status) override;
 
  private:
diff --git a/net/spdy/spdy_network_transaction_unittest.cc b/net/spdy/spdy_network_transaction_unittest.cc
index 4ff51bd..7525f0f 100644
--- a/net/spdy/spdy_network_transaction_unittest.cc
+++ b/net/spdy/spdy_network_transaction_unittest.cc
@@ -5467,13 +5467,16 @@ TEST_P(SpdyNetworkTransactionTest, SynReplyWithHeaders) {
   EXPECT_EQ(ERR_SPDY_PROTOCOL_ERROR, out.rv);
 }
 
-TEST_P(SpdyNetworkTransactionTest, SynReplyWithLateHeaders) {
+// Tests that receiving HEADERS, DATA, HEADERS, and DATA in that sequence will
+// trigger a ERR_SPDY_PROTOCOL_ERROR because trailing HEADERS must not be
+// followed by any DATA frames.
+TEST_P(SpdyNetworkTransactionTest, SyncReplyDataAfterTrailers) {
   scoped_ptr<SpdyFrame> req(
       spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, LOWEST, true));
   scoped_ptr<SpdyFrame> rst(
       spdy_util_.ConstructSpdyRstStream(1, RST_STREAM_PROTOCOL_ERROR));
   MockWrite writes[] = {
-      CreateMockWrite(*req, 0), CreateMockWrite(*rst, 4),
+      CreateMockWrite(*req, 0), CreateMockWrite(*rst, 5),
   };
 
   scoped_ptr<SpdyFrame> stream1_reply(
@@ -5494,10 +5497,8 @@ TEST_P(SpdyNetworkTransactionTest, SynReplyWithLateHeaders) {
   scoped_ptr<SpdyFrame> stream1_body2(
       spdy_util_.ConstructSpdyBodyFrame(1, true));
   MockRead reads[] = {
-      CreateMockRead(*stream1_reply, 1),
-      CreateMockRead(*stream1_body, 2),
-      CreateMockRead(*stream1_headers, 3),
-      CreateMockRead(*stream1_body2, 5),
+      CreateMockRead(*stream1_reply, 1), CreateMockRead(*stream1_body, 2),
+      CreateMockRead(*stream1_headers, 3), CreateMockRead(*stream1_body2, 4),
       MockRead(ASYNC, 0, 6)  // EOF
   };
 
diff --git a/net/spdy/spdy_proxy_client_socket.cc b/net/spdy/spdy_proxy_client_socket.cc
index 39fbd60..e6d521e 100644
--- a/net/spdy/spdy_proxy_client_socket.cc
+++ b/net/spdy/spdy_proxy_client_socket.cc
@@ -496,6 +496,12 @@ void SpdyProxyClientSocket::OnDataSent()  {
                             ResetAndReturn(&write_callback_), rv));
 }
 
+void SpdyProxyClientSocket::OnTrailers(const SpdyHeaderBlock& trailers) {
+  // |spdy_stream_| is of type SPDY_BIDIRECTIONAL_STREAM, so trailers are
+  // combined with response headers and this method will not be calld.
+  NOTREACHED();
+}
+
 void SpdyProxyClientSocket::OnClose(int status)  {
   was_ever_used_ = spdy_stream_->WasEverUsed();
   spdy_stream_.reset();
diff --git a/net/spdy/spdy_proxy_client_socket.h b/net/spdy/spdy_proxy_client_socket.h
index 30861ee..af38767 100644
--- a/net/spdy/spdy_proxy_client_socket.h
+++ b/net/spdy/spdy_proxy_client_socket.h
@@ -95,6 +95,7 @@ class NET_EXPORT_PRIVATE SpdyProxyClientSocket : public ProxyClientSocket,
       const SpdyHeaderBlock& response_headers) override;
   void OnDataReceived(scoped_ptr<SpdyBuffer> buffer) override;
   void OnDataSent() override;
+  void OnTrailers(const SpdyHeaderBlock& trailers) override;
   void OnClose(int status) override;
 
  private:
diff --git a/net/spdy/spdy_session_pool_unittest.cc b/net/spdy/spdy_session_pool_unittest.cc
index 3f12e61..8f397ea 100644
--- a/net/spdy/spdy_session_pool_unittest.cc
+++ b/net/spdy/spdy_session_pool_unittest.cc
@@ -75,6 +75,8 @@ class SessionOpeningDelegate : public SpdyStream::Delegate {
 
   void OnDataSent() override {}
 
+  void OnTrailers(const SpdyHeaderBlock& trailers) override {}
+
   void OnClose(int status) override {
     ignore_result(CreateFakeSpdySession(spdy_session_pool_, key_));
   }
diff --git a/net/spdy/spdy_stream.cc b/net/spdy/spdy_stream.cc
index a78a78f..812d520 100644
--- a/net/spdy/spdy_stream.cc
+++ b/net/spdy/spdy_stream.cc
@@ -57,6 +57,8 @@ bool ContainsUppercaseAscii(const std::string& str) {
 
 }  // namespace
 
+void SpdyStream::Delegate::OnTrailers(const SpdyHeaderBlock& trailers) {}
+
 // A wrapper around a stream that calls into ProduceSynStreamFrame().
 class SpdyStream::SynStreamBufferProducer : public SpdyBufferProducer {
  public:
@@ -440,12 +442,18 @@ int SpdyStream::OnInitialResponseHeadersReceived(
 int SpdyStream::OnAdditionalResponseHeadersReceived(
     const SpdyHeaderBlock& additional_response_headers) {
   if (type_ == SPDY_REQUEST_RESPONSE_STREAM) {
-    session_->ResetStream(
-        stream_id_, RST_STREAM_PROTOCOL_ERROR,
-        "Additional headers received for request/response stream");
-    return ERR_SPDY_PROTOCOL_ERROR;
-  } else if (type_ == SPDY_PUSH_STREAM &&
-             response_headers_status_ == RESPONSE_HEADERS_ARE_COMPLETE) {
+    if (response_headers_status_ != RESPONSE_HEADERS_ARE_COMPLETE) {
+      session_->ResetStream(
+          stream_id_, RST_STREAM_PROTOCOL_ERROR,
+          "Additional headers received for request/response stream");
+      return ERR_SPDY_PROTOCOL_ERROR;
+    }
+    response_headers_status_ = TRAILERS_RECEIVED;
+    delegate_->OnTrailers(additional_response_headers);
+    return OK;
+  }
+  if (type_ == SPDY_PUSH_STREAM &&
+      response_headers_status_ == RESPONSE_HEADERS_ARE_COMPLETE) {
     session_->ResetStream(
         stream_id_, RST_STREAM_PROTOCOL_ERROR,
         "Additional headers received for push stream");
@@ -484,6 +492,14 @@ void SpdyStream::OnDataReceived(scoped_ptr<SpdyBuffer> buffer) {
     return;
   }
 
+  if (response_headers_status_ == TRAILERS_RECEIVED && buffer) {
+    // TRAILERS_RECEIVED is only used in SPDY_REQUEST_RESPONSE_STREAM.
+    DCHECK_EQ(type_, SPDY_REQUEST_RESPONSE_STREAM);
+    session_->ResetStream(stream_id_, RST_STREAM_PROTOCOL_ERROR,
+                          "Data received after trailers");
+    return;
+  }
+
   // If we have response headers but the delegate has indicated that
   // it's still incomplete, then that's a protocol error.
   if (response_headers_status_ == RESPONSE_HEADERS_ARE_INCOMPLETE) {
diff --git a/net/spdy/spdy_stream.h b/net/spdy/spdy_stream.h
index 4c01e0c..f9e29aa 100644
--- a/net/spdy/spdy_stream.h
+++ b/net/spdy/spdy_stream.h
@@ -54,10 +54,15 @@ enum SpdySendStatus {
 };
 
 // Returned by SpdyStream::OnResponseHeadersUpdated() to indicate
-// whether the current response headers are complete or not.
+// whether the current response headers are complete or not, or whether
+// trailers have been received. TRAILERS_RECEIVED denotes the state where
+// headers are received after DATA frames. TRAILERS_RECEIVED is only used for
+// SPDY_REQUEST_RESPONSE_STREAM, and this state also implies that the response
+// headers are complete.
 enum SpdyResponseHeadersStatus {
   RESPONSE_HEADERS_ARE_INCOMPLETE,
-  RESPONSE_HEADERS_ARE_COMPLETE
+  RESPONSE_HEADERS_ARE_COMPLETE,
+  TRAILERS_RECEIVED,
 };
 
 // The SpdyStream is used by the SpdySession to represent each stream known
@@ -119,8 +124,6 @@ class NET_EXPORT_PRIVATE SpdyStream {
     //     before any data is received; any deviation from this is
     //     treated as a protocol error.
     //
-    // TODO(akalin): Treat headers received after data has been
-    // received as a protocol error for non-bidirectional streams.
     // TODO(jgraettinger): This should be at the semantic (HTTP) rather
     // than stream layer. Streams shouldn't have a notion of header
     // completeness. Move to SpdyHttpStream/SpdyWebsocketStream.
@@ -139,6 +142,11 @@ class NET_EXPORT_PRIVATE SpdyStream {
     // closed.
     virtual void OnDataSent() = 0;
 
+    // Called when trailers are received. Note that trailers HEADER frame will
+    // have END_STREAM flag set according to section 8.1 of the HTTP/2 RFC,
+    // so this will be followed by OnClose.
+    virtual void OnTrailers(const SpdyHeaderBlock& trailers) = 0;
+
     // Called when SpdyStream is closed. No other delegate functions
     // will be called after this is called, and the delegate must not
     // access the stream after this is called. Must not cause the
diff --git a/net/spdy/spdy_stream_test_util.cc b/net/spdy/spdy_stream_test_util.cc
index 4cd5264..7e10e1c 100644
--- a/net/spdy/spdy_stream_test_util.cc
+++ b/net/spdy/spdy_stream_test_util.cc
@@ -33,6 +33,8 @@ void ClosingDelegate::OnDataReceived(scoped_ptr<SpdyBuffer> buffer) {}
 
 void ClosingDelegate::OnDataSent() {}
 
+void ClosingDelegate::OnTrailers(const SpdyHeaderBlock& trailers) {}
+
 void ClosingDelegate::OnClose(int status) {
   DCHECK(stream_);
   stream_->Close();
@@ -69,6 +71,8 @@ void StreamDelegateBase::OnDataReceived(scoped_ptr<SpdyBuffer> buffer) {
 
 void StreamDelegateBase::OnDataSent() {}
 
+void StreamDelegateBase::OnTrailers(const SpdyHeaderBlock& trailers) {}
+
 void StreamDelegateBase::OnClose(int status) {
   if (!stream_.get())
     return;
diff --git a/net/spdy/spdy_stream_test_util.h b/net/spdy/spdy_stream_test_util.h
index 66db00c..9c260ea 100644
--- a/net/spdy/spdy_stream_test_util.h
+++ b/net/spdy/spdy_stream_test_util.h
@@ -31,6 +31,7 @@ class ClosingDelegate : public SpdyStream::Delegate {
       const SpdyHeaderBlock& response_headers) override;
   void OnDataReceived(scoped_ptr<SpdyBuffer> buffer) override;
   void OnDataSent() override;
+  void OnTrailers(const SpdyHeaderBlock& trailers) override;
   void OnClose(int status) override;
 
   // Returns whether or not the stream is closed.
@@ -52,6 +53,7 @@ class StreamDelegateBase : public SpdyStream::Delegate {
       const SpdyHeaderBlock& response_headers) override;
   void OnDataReceived(scoped_ptr<SpdyBuffer> buffer) override;
   void OnDataSent() override;
+  void OnTrailers(const SpdyHeaderBlock& trailers) override;
   void OnClose(int status) override;
 
   // Waits for the stream to be closed and returns the status passed
diff --git a/net/spdy/spdy_stream_unittest.cc b/net/spdy/spdy_stream_unittest.cc
index b87634f..af606b3 100644
--- a/net/spdy/spdy_stream_unittest.cc
+++ b/net/spdy/spdy_stream_unittest.cc
@@ -175,6 +175,92 @@ TEST_P(SpdyStreamTest, SendDataAfterOpen) {
   EXPECT_TRUE(data.AllWriteDataConsumed());
 }
 
+// Delegate that receives trailers.
+class StreamDelegateWithTrailers : public test::StreamDelegateWithBody {
+ public:
+  StreamDelegateWithTrailers(const base::WeakPtr<SpdyStream>& stream,
+                             base::StringPiece data)
+      : StreamDelegateWithBody(stream, data) {}
+
+  ~StreamDelegateWithTrailers() override {}
+
+  void OnTrailers(const SpdyHeaderBlock& trailers) override {
+    trailers_ = trailers;
+  }
+
+  const SpdyHeaderBlock& trailers() const { return trailers_; }
+
+ private:
+  SpdyHeaderBlock trailers_;
+};
+
+// Regression test for crbug.com/481033.
+TEST_P(SpdyStreamTest, Trailers) {
+  GURL url(kStreamUrl);
+
+  session_ =
+      SpdySessionDependencies::SpdyCreateSessionDeterministic(&session_deps_);
+
+  scoped_ptr<SpdyFrame> req(spdy_util_.ConstructSpdyPost(
+      kStreamUrl, 1, kPostBodyLength, LOWEST, NULL, 0));
+  AddWrite(*req);
+
+  scoped_ptr<SpdyFrame> msg(
+      spdy_util_.ConstructSpdyBodyFrame(1, kPostBody, kPostBodyLength, true));
+  AddWrite(*msg);
+
+  scoped_ptr<SpdyFrame> resp(spdy_util_.ConstructSpdyPostSynReply(NULL, 0));
+  AddRead(*resp);
+
+  scoped_ptr<SpdyFrame> echo(
+      spdy_util_.ConstructSpdyBodyFrame(1, kPostBody, kPostBodyLength, false));
+  AddRead(*echo);
+
+  const char* const kExtraHeaders[] = {"foo", "bar"};
+  scoped_ptr<SpdyFrame> trailers(
+      spdy_util_.ConstructSpdyHeaderFrame(1, kExtraHeaders, 1));
+  AddRead(*trailers);
+
+  AddReadEOF();
+
+  DeterministicSocketData data(GetReads(), GetNumReads(), GetWrites(),
+                               GetNumWrites());
+  MockConnect connect_data(SYNCHRONOUS, OK);
+  data.set_connect_data(connect_data);
+
+  session_deps_.deterministic_socket_factory->AddSocketDataProvider(&data);
+
+  base::WeakPtr<SpdySession> session(CreateDefaultSpdySession());
+
+  base::WeakPtr<SpdyStream> stream = CreateStreamSynchronously(
+      SPDY_REQUEST_RESPONSE_STREAM, session, url, LOWEST, BoundNetLog());
+  ASSERT_TRUE(stream.get() != NULL);
+
+  StreamDelegateWithTrailers delegate(stream, kPostBodyStringPiece);
+  stream->SetDelegate(&delegate);
+
+  EXPECT_FALSE(stream->HasUrlFromHeaders());
+
+  scoped_ptr<SpdyHeaderBlock> headers(
+      spdy_util_.ConstructPostHeaderBlock(kStreamUrl, kPostBodyLength));
+  EXPECT_EQ(ERR_IO_PENDING,
+            stream->SendRequestHeaders(headers.Pass(), MORE_DATA_TO_SEND));
+  EXPECT_TRUE(stream->HasUrlFromHeaders());
+  EXPECT_EQ(kStreamUrl, stream->GetUrlFromHeaders().spec());
+
+  data.RunFor(GetNumReads() + GetNumWrites());
+  EXPECT_EQ(ERR_CONNECTION_CLOSED, delegate.WaitForClose());
+
+  EXPECT_TRUE(delegate.send_headers_completed());
+  EXPECT_EQ("200", delegate.GetResponseHeaderValue(spdy_util_.GetStatusKey()));
+  const SpdyHeaderBlock& received_trailers = delegate.trailers();
+  SpdyHeaderBlock::const_iterator it = received_trailers.find("foo");
+  EXPECT_EQ("bar", it->second);
+  EXPECT_EQ(std::string(kPostBody, kPostBodyLength),
+            delegate.TakeReceivedData());
+  EXPECT_TRUE(data.AllWriteDataConsumed());
+}
+
 TEST_P(SpdyStreamTest, PushedStream) {
   session_ =
       SpdySessionDependencies::SpdyCreateSessionDeterministic(&session_deps_);
diff --git a/net/spdy/spdy_test_util_common.cc b/net/spdy/spdy_test_util_common.cc
index 0d3e6b2..a7faa3a 100644
--- a/net/spdy/spdy_test_util_common.cc
+++ b/net/spdy/spdy_test_util_common.cc
@@ -1105,6 +1105,15 @@ SpdyFrame* SpdyTestUtil::ConstructSpdyPushHeaders(
   return CreateFramer(false)->SerializeFrame(headers);
 }
 
+SpdyFrame* SpdyTestUtil::ConstructSpdyHeaderFrame(int stream_id,
+                                                  const char* const headers[],
+                                                  int header_count) {
+  SpdyHeadersIR spdy_headers(stream_id);
+  AppendToHeaderBlock(headers, header_count,
+                      spdy_headers.mutable_header_block());
+  return CreateFramer(false)->SerializeFrame(spdy_headers);
+}
+
 SpdyFrame* SpdyTestUtil::ConstructSpdySyn(int stream_id,
                                           const SpdyHeaderBlock& block,
                                           RequestPriority priority,
diff --git a/net/spdy/spdy_test_util_common.h b/net/spdy/spdy_test_util_common.h
index ae49bde..76e0f71 100644
--- a/net/spdy/spdy_test_util_common.h
+++ b/net/spdy/spdy_test_util_common.h
@@ -463,6 +463,10 @@ class SpdyTestUtil {
                                       const char* const extra_headers[],
                                       int extra_header_count);
 
+  SpdyFrame* ConstructSpdyHeaderFrame(int stream_id,
+                                      const char* const headers[],
+                                      int header_count);
+
   // Construct a SPDY syn (HEADERS or SYN_STREAM, depending on protocol
   // version) carrying exactly the given headers and priority.
   SpdyFrame* ConstructSpdySyn(int stream_id,