Disallow multiple HEADERS frames on pushed streams.

net/spdy/spdy_stream.h

Index: net/spdy/spdy_stream.h
diff --git a/net/spdy/spdy_stream.h b/net/spdy/spdy_stream.h
index b2d6fbbf0d264c9f62778ea9eef4c7f5dcc6712e..a13ef32b37d72a5eda44d10bd051d35369c6346f 100644
--- a/net/spdy/spdy_stream.h
+++ b/net/spdy/spdy_stream.h
@@ -56,18 +56,6 @@ enum SpdySendStatus {
   NO_MORE_DATA_TO_SEND
 };
 
-// Returned by SpdyStream::OnResponseHeadersUpdated() to indicate
-// 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,
-  TRAILERS_RECEIVED,
-};
-
 // The SpdyStream is used by the SpdySession to represent each stream known
 // on the SpdySession.  This class provides interfaces for SpdySession to use.
 // Streams can be created either by the client or by the server.  When they
@@ -84,76 +72,32 @@ class NET_EXPORT_PRIVATE SpdyStream {
 
     // Called when the request headers have been sent. Never called
     // for push streams. Must not cause the stream to be closed.
-    virtual void OnRequestHeadersSent() = 0;
+    virtual void OnHeadersSent() = 0;
 
-    // WARNING: This function is complicated! Be sure to read the
-    // whole comment below if you're working with code that implements
-    // or calls this function.
-    //
-    // Called when the response headers are updated from the
-    // server. |response_headers| contains the set of all headers
-    // received up to this point; delegates can assume that any
-    // headers previously received remain unchanged.
-    //
-    // This is called at least once before any data is received. If
-    // RESPONSE_HEADERS_ARE_INCOMPLETE is returned, this will be
-    // called again when more headers are received until
-    // RESPONSE_HEADERS_ARE_COMPLETE is returned, and any data
-    // received before then will be treated as a protocol error.
-    //
-    // If RESPONSE_HEADERS_ARE_INCOMPLETE is returned, the delegate
-    // must not have closed the stream. Otherwise, if
-    // RESPONSE_HEADERS_ARE_COMPLETE is returned, the delegate has
-    // processed the headers successfully. However, it still may have
-    // closed the stream, e.g. if the headers indicated an error
-    // condition.
-    //
-    // Some type-specific behavior:
-    //
-    //   - For bidirectional streams, this may be called even after
-    //     data is received, but it is expected that
-    //     RESPONSE_HEADERS_ARE_COMPLETE is always returned. If
-    //     RESPONSE_HEADERS_ARE_INCOMPLETE is returned, this is
-    //     treated as a protocol error.
-    //
-    //   - For request/response streams, this function is called
-    //     exactly once before data is received, and it is expected
-    //     that RESPONSE_HEADERS_ARE_COMPLETE is returned. If
-    //     RESPONSE_HEADERS_ARE_INCOMPLETE is returned, this is
-    //     treated as a protocol error.
-    //
-    //   - For push streams, it is expected that this function will be
-    //     called until RESPONSE_HEADERS_ARE_COMPLETE is returned
-    //     before any data is received; any deviation from this is
-    //     treated as a protocol error.
-    //
-    // 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.
-    virtual SpdyResponseHeadersStatus OnResponseHeadersUpdated(
-        const SpdyHeaderBlock& response_headers) = 0;
-
-    // Called when data is received after all required response
-    // headers have been received. |buffer| may be NULL, which signals
-    // EOF.  Must return OK if the data was received successfully, or
-    // a network error code otherwise.
-    //
+    // OnHeadersReceived(), OnDataReceived(), OnTrailers(), and OnClose()
+    // are guaranteed to be called in the following order:
+    //   - OnHeadersReceived() exactly once;
+    //   - OnDataReceived() zero or more times;
+    //   - OnTrailers() zero or one times;
+    //   - OnClose() exactly once.
+
+    // Called when headers have been received.

[Ryan Hamilton, 2016/11/23 20:58:43]

This comment is far to easy to understand compared to the previous comment!

:)

+    virtual void OnHeadersReceived(const SpdyHeaderBlock& response_headers) = 0;
+
+    // Called when data is received.  |buffer| may be NULL, which signals EOF.
     // May cause the stream to be closed.
     virtual void OnDataReceived(std::unique_ptr<SpdyBuffer> buffer) = 0;
 
-    // Called when data is sent. Must not cause the stream to be
-    // closed.
+    // Called when data is sent.  Must not cause the stream to be 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.
+    // Called when trailers are received.
     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
-    // stream to be be (re-)closed.
+    // stream to be (re-)closed.
     //
     // TODO(akalin): Allow this function to re-close the stream and
     // handle it gracefully.
@@ -292,19 +236,11 @@ class NET_EXPORT_PRIVATE SpdyStream {
   base::Time GetRequestTime() const;
   void SetRequestTime(base::Time t);
 
-  // Called at most once by the SpdySession when the initial response headers
-  // have been received for this stream. Returns a status code; if it is an
-  // error, the stream was closed by this function.
-  int OnInitialResponseHeadersReceived(const SpdyHeaderBlock& response_headers,
-                                       base::Time response_time,
-                                       base::TimeTicks recv_first_byte_time);
-
-  // Called by the SpdySession (only after
-  // OnInitialResponseHeadersReceived() has been called) when
-  // late-bound headers are received for a stream. Returns a status
-  // code; if it is an error, the stream was closed by this function.
-  int OnAdditionalResponseHeadersReceived(
-      const SpdyHeaderBlock& additional_response_headers);
+  // Called by SpdySession when headers are received for this stream.  May close
+  // the stream.
+  void OnHeadersReceived(const SpdyHeaderBlock& response_headers,
+                         base::Time response_time,
+                         base::TimeTicks recv_first_byte_time);
 
   // Called by the SpdySession when a frame carrying request headers opening a
   // push stream is received. Stream transits to STATE_RESERVED_REMOTE state.
@@ -334,7 +270,7 @@ class NET_EXPORT_PRIVATE SpdyStream {
   void OnFrameWriteComplete(SpdyFrameType frame_type, size_t frame_size);
 
   // HEADERS-specific write handler invoked by OnFrameWriteComplete().
-  int OnRequestHeadersSent();
+  int OnHeadersSent();
 
   // DATA-specific write handler invoked by OnFrameWriteComplete().
   // If more data is already available to be written, the next write is
@@ -367,11 +303,10 @@ class NET_EXPORT_PRIVATE SpdyStream {
   // Only one send can be in flight at a time, except for push
   // streams, which must not send anything.
 
-  // Sends the request headers. The delegate is called back via
-  // OnRequestHeadersSent() when the request headers have completed
-  // sending. |send_status| must be MORE_DATA_TO_SEND for
-  // bidirectional streams; for request/response streams, it must be
-  // MORE_DATA_TO_SEND if the request has data to upload, or
+  // Sends the request headers. The delegate is called back via OnHeadersSent()
+  // when the request headers have completed sending. |send_status| must be
+  // MORE_DATA_TO_SEND for bidirectional streams; for request/response streams,
+  // it must be MORE_DATA_TO_SEND if the request has data to upload, or
   // NO_MORE_DATA_TO_SEND if not.
   int SendRequestHeaders(SpdyHeaderBlock request_headers,
                          SpdySendStatus send_status);
@@ -455,6 +390,20 @@ class NET_EXPORT_PRIVATE SpdyStream {
     STATE_CLOSED,
   };
 
+  // Per RFC 7540 Section 8.1, an HTTP response consists of:
+  // * zero or more header blocks with informational (1xx) HTTP status,
+  // * one header block,
+  // * zero or more DATA frames,
+  // * zero or one header block ("trailers").
+  // Each header block must have a ":status" header field.  SpdyStream enforces
+  // these requirements, and resets the stream if they are not met.
+  // TODO(bnc) https://crbug.com/662197 Add support for informational headers.
+  enum SpdyResponseHeadersStatus {
+    READY_FOR_HEADERS,
+    READY_FOR_DATA,

[Ryan Hamilton, 2016/11/23 20:58:43]

nit: I think this should be renamed to:
READY_FOR_TRAILERS
if it's just the headers status, or
READY_FOR_DATA_OR_TRAILERS
if this enum is about both body and headers (in which case, perhaps the enum
should be renamed).  Also, since this is an inner-calls, I think you can drop
the "Spdy" prefix.

[Bence, 2016/11/23 21:15:58]

Okay, I'll do this.

+    TRAILERS_RECEIVED
+  };
+
   // Update the histograms.  Can safely be called repeatedly, but should only
   // be called after the stream has completed.
   void UpdateHistograms();
@@ -477,11 +426,9 @@ class NET_EXPORT_PRIVATE SpdyStream {
   // |pending_send_data_| is set.
   void QueueNextDataFrame();
 
-  // Merge the given headers into |response_headers_| and calls
-  // OnResponseHeadersUpdated() on the delegate (if attached).
-  // Returns a status code; if it is an error, the stream was closed
-  // by this function.
-  int MergeWithResponseHeaders(const SpdyHeaderBlock& new_response_headers);
+  // Saves the given headers into |response_headers_| and calls
+  // OnHeadersReceived() on the delegate if attached.
+  void SaveResponseHeaders(const SpdyHeaderBlock& response_headers);
 
   static std::string DescribeState(State state);