Disallow multiple HEADERS frames on pushed streams.

net/spdy/spdy_stream.h

 // 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.
 
 #ifndef NET_SPDY_SPDY_STREAM_H_
 #define NET_SPDY_SPDY_STREAM_H_
 
 #include <stddef.h>
 #include <stdint.h>
 
@@ skipping 38 matching lines @@
   SPDY_PUSH_STREAM
 };
 
 // Passed to some SpdyStream functions to indicate whether there's
 // more data to send.
 enum SpdySendStatus {
   MORE_DATA_TO_SEND,
   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
 // are initiated by the client, both the SpdySession and client object (such as
 // a SpdyNetworkTransaction) will maintain a reference to the stream.  When
 // initiated by the server, only the SpdySession will maintain any reference,
 // until such a time as a client object requests a stream for the path.
 class NET_EXPORT_PRIVATE SpdyStream {
  public:
   // Delegate handles protocol specific behavior of spdy stream.
   class NET_EXPORT_PRIVATE Delegate {
    public:
     Delegate() {}
 
     // 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;
+    // 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 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.
-    //
+    // 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.
     virtual void OnClose(int status) = 0;
 
    protected:
     virtual ~Delegate() {}
 
    private:
     DISALLOW_COPY_AND_ASSIGN(Delegate);
@@ skipping 118 matching lines @@
 
   // Returns true if the underlying transport socket ever had any reads or
   // writes.
   bool WasEverUsed() const;
 
   const NetLogWithSource& net_log() const { return net_log_; }
 
   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.
   void OnPushPromiseHeadersReceived(SpdyHeaderBlock headers);
 
   // Called by the SpdySession when response data has been received
   // for this stream.  This callback may be called multiple times as
   // data arrives from the network, and will never be called prior to
   // OnResponseHeadersReceived.
   //
   // |buffer| contains the data received, or NULL if the stream is
   //          being closed.  The stream must copy any data from this
   //          buffer before returning from this callback.
   //
   // |length| is the number of bytes received (at most 2^24 - 1) or 0 if
   //          the stream is being closed.
   void OnDataReceived(std::unique_ptr<SpdyBuffer> buffer);
 
   // Called by the SpdySession when padding is consumed to allow for the stream
   // receiving window to be updated.
   void OnPaddingConsumed(size_t len);
 
   // Called by the SpdySession when a frame has been successfully and completely
   // written. |frame_size| is the total size of the logical frame in bytes,
   // including framing overhead.  For fragmented headers, this is the total size
   // of the HEADERS or PUSH_PROMISE frame and subsequent CONTINUATION frames.
   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
   // queued and ERR_IO_PENDING is returned. Returns OK otherwise.
   int OnDataSent(size_t frame_size);
 
   // Called by the SpdySession when the request is finished.  This callback
   // will always be called at the end of the request and signals to the
   // stream that the stream has no more network events.  No further callbacks
   // to the stream will be made after this call.
@@ skipping 12 matching lines @@
   void Close();
 
   // Must be used only by |session_|.
   base::WeakPtr<SpdyStream> GetWeakPtr();
 
   // Interface for the delegate to use.
 
   // 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);
 
   // Sends a DATA frame. The delegate will be notified via
   // OnDataSent() when the send is complete. |send_status| must be
   // MORE_DATA_TO_SEND for bidirectional streams; for request/response
   // streams, it must be MORE_DATA_TO_SEND if there is more data to
   // upload, or NO_MORE_DATA_TO_SEND if not.
   void SendData(IOBuffer* data, int length, SpdySendStatus send_status);
@@ skipping 63 matching lines @@
   enum State {
     STATE_IDLE,
     STATE_OPEN,
     STATE_HALF_CLOSED_LOCAL_UNCLAIMED,
     STATE_HALF_CLOSED_LOCAL,
     STATE_HALF_CLOSED_REMOTE,
     STATE_RESERVED_REMOTE,
     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();
 
   // When a server-push stream is claimed by SetDelegate(), this function is
   // posted on the current MessageLoop to replay everything the server has sent.
   // From the perspective of SpdyStream's state machine, headers, data, and
   // FIN states received prior to the delegate being attached have not yet been
   // read. While buffered by |pending_recv_data_| it's not until
   // PushedStreamReplay() is invoked that reads are considered
   // to have occurred, driving the state machine forward.
   void PushedStreamReplay();
 
   // Produces the HEADERS frame for the stream. The stream must
   // already be activated.
   std::unique_ptr<SpdySerializedFrame> ProduceHeadersFrame();
 
   // Queues the send for next frame of the remaining data in
   // |pending_send_data_|. Must be called only when
   // |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);
 
   const SpdyStreamType type_;
 
   SpdyStreamId stream_id_;
   const GURL url_;
   const RequestPriority priority_;
 
   bool send_stalled_by_flow_control_;
@@ skipping 77 matching lines @@
   bool write_handler_guard_;
 
   base::WeakPtrFactory<SpdyStream> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SpdyStream);
 };
 
 }  // namespace net
 
 #endif  // NET_SPDY_SPDY_STREAM_H_