SPDY: Add WriteHeaders interface to SpdySession and SpdyStream

Add WriteHeaders interface to SpdySession and SpdyStream.

To support WebSocket over SPDY, SpdyStream should provide a interface to
send HEADERS control frames for SpdyWebSocketStream.

This is because WebSocket over SPDY requires to convert a WebSocket frame
into one HEADERS frame and subsequent DATA frame(s) in the spec.

See also http://goo.gl/mJCrx the spec draft 9, "Frame mapping" section.

BUG=42320
TEST=net_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149808

[Takashi Toyoshima, 2012-07-24 11:30:14]

Hi Will and Matt,

Now I'm implementing the latest plan of WebSocket over SPDY.
Opening handshakes already work, and now I'm trying to support
new framing which use one HEADERS frame and subsequent DATA frames
for one WebSocket frame.

Could you review this change to provide a interface to send HEADERS frames
from SpdyWebSocketStream?

Thanks.

[mmenke, 2012-07-24 16:42:41]

While the code looks fine to me, I'm not sufficiently familiar with SPDY to sign
off on anything.

http://codereview.chromium.org/10810069/diff/2002/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/10810069/diff/2002/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:677: NetLog::TYPE_SPDY_SESSION_HEADERS,
This is what's logged when we receive headers before, right?  Think we probably
want a new event for sending headers.

[willchan no longer on Chromium, 2012-07-24 17:45:43]

rch/rtenneti will be better than mmenke for spdy code. Lemme send it their way.

[Ryan Hamilton, 2012-07-24 17:53:03]

https://chromiumcodereview.appspot.com/10810069/diff/2002/net/spdy/spdy_sessi...
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/10810069/diff/2002/net/spdy/spdy_sessi...
net/spdy/spdy_session.cc:671: buffered_spdy_framer_->CreateHeaders(stream_id,
flags, true, &headers));
is "true" the the boolean for compressed?  If so, I think there's a bug here. 
(I'm working on a CL to re-jigger this, but until then...)  QueueFrame sticks a
frame into a queue ordered by priority.  As such, it is possible to enqueue a
HEADERS frame now, then a higher priority SYN_STREAM frame later.  But the
SYN_STREAM frame will be removed from the queue first (because of priority).  We
need to compress the frames in the order they are written to the wire, not the
order they are created.  I think you want compressed to be false here, and let
SpdySession::WriteSocket() actually do the compression.

https://chromiumcodereview.appspot.com/10810069/diff/2002/net/spdy/spdy_sessi...
net/spdy/spdy_session.cc:677: NetLog::TYPE_SPDY_SESSION_HEADERS,
On 2012/07/24 16:42:41, Matt Menke wrote:
> This is what's logged when we receive headers before, right?  Think we
probably
> want a new event for sending headers.

Yes, I think we should add a new TYPE_SPDY_SESSION_SEND_HEADERS

[willchan no longer on Chromium, 2012-07-24 20:58:16]

Is there an updated doc with the new WebSocket over SPDY layering?

[Takashi Toyoshima, 2012-07-25 08:44:24]

> Is there an updated doc with the new WebSocket over SPDY layering?

Sorry for missing information.
I revise CL description to point original bug number and the latest spec.

the spec draft 9 http://goo.gl/mJCrx

https://chromiumcodereview.appspot.com/10810069/diff/2002/net/spdy/spdy_sessi...
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/10810069/diff/2002/net/spdy/spdy_sessi...
net/spdy/spdy_session.cc:671: buffered_spdy_framer_->CreateHeaders(stream_id,
flags, true, &headers));
OK, I see the current issue.
I change this flag to false.

https://chromiumcodereview.appspot.com/10810069/diff/2002/net/spdy/spdy_sessi...
net/spdy/spdy_session.cc:677: NetLog::TYPE_SPDY_SESSION_HEADERS,
On 2012/07/24 17:53:03, Ryan Hamilton wrote:
> On 2012/07/24 16:42:41, Matt Menke wrote:
> > This is what's logged when we receive headers before, right?  Think we
> probably
> > want a new event for sending headers.
> 
> Yes, I think we should add a new TYPE_SPDY_SESSION_SEND_HEADERS
> 

Done.

[Takashi Toyoshima, 2012-07-27 10:34:02]

Hi Ryan,
Could you review this change again?

[Ryan Hamilton, 2012-07-27 17:01:37]

Hm...  Alas, the ground has shifted out from under you somewhat.  Yesterday I
landed this CL:

https://chromiumcodereview.appspot.com/10815074/

Which switches from queueing up frames, to queueing up (basically) streams that
are ready to produce frames.  I think you're going to need to adapt your CL to
work in this new world.  To do this, you'll need to tweak
SpdyStream::ProduceNextFrame() to produce a SpdyHeadersFrame when needed. 
(Possibly by queueing up the header block and flags).  Sorry to change the
landscape.

[Takashi Toyoshima, 2012-07-30 14:47:40]

Hi Ryan,

I revised my change to follow your CL.
Patch Set 4 is a simple version which use uncompressed headers to be queued into
pending queue as a SpdyFrame.

Patch Set 5 is a enhanced version which queues headers structure into pending
queue and create headers frame in ProduceNextFrame().

I think Patch Set 5 is a right approach, so could you review this latest patch
set?

[Takashi Toyoshima, 2012-07-31 12:16:52]

Oops, Patch Set 5 fired DCHECK when window is exhausted.
I fixed this issue at Patch Set 8.

Ryan, could you review this again?

[Ryan Hamilton, 2012-07-31 14:40:12]

Will do

On Tuesday, July 31, 2012, wrote:

> Oops, Patch Set 5 fired DCHECK when window is exhausted.
> I fixed this issue at Patch Set 8.
>
> Ryan, could you review this again?
>
>
https://chromiumcodereview.**appspot.com/10810069/<https://chromiumcodereview...
>

[Ryan Hamilton, 2012-07-31 15:46:24]

Looking reasonable.  Please add tests.

http://codereview.chromium.org/10810069/diff/13012/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

http://codereview.chromium.org/10810069/diff/13012/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:91: typedef std::pair<SpdyHeaderBlock*, SpdyDataFrame*>
PendingFrame;
Instead of storing a pair of pointers, how about storing the two pointers in a
union and using a boolen (or an enum) to determine which one is valid.

[Takashi Toyoshima, 2012-08-01 13:40:00]

I added a simple test which is based onWebSocket over SPDY use case.
In this test, I notice that I should modify OnDataSent() callback invocation.
OnDataSent should be called with actual user data size. I'll work on fixing this
in another CL.

http://codereview.chromium.org/10810069/diff/13012/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

http://codereview.chromium.org/10810069/diff/13012/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:91: typedef std::pair<SpdyHeaderBlock*, SpdyDataFrame*>
PendingFrame;
Thanks.
That's nice. I can remove complicated DCHECK(!headers != !data_frame) from my
change :-)

[Takashi Toyoshima, 2012-08-02 09:11:57]

I upload another CL to handle OnDataSent callback invocation correctly.
https://chromiumcodereview.appspot.com/10828129/

That change also include this CL, so please review the difference between patch
set 1 and 2.
patch set 1 is the same with this CL's patch set 12.

[Ryan Hamilton, 2012-08-02 16:32:22]

lgtm

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:145: if (frame.type == TYPE_DATA)
This code is much cleaner now.  Nice!

nit:  I would probably write:
if (...) {
  // do data stuff
} else {
  DCHECK(frame.type == TYPE_HEADERS)
  // do headers stuff
}

but it's your call.

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:103: } data;
nit:  you should be able to use an anonymous union here.  (Just leave off the
"data").  This will let you refer to 

  pending_frame.header_block and pending_frame.data_frame

without needing to stick .data in there

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_stream_spdy2...
File net/spdy/spdy_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_stream_spdy2...
net/spdy/spdy_stream_spdy2_unittest.cc:271: // But current implementation
invokes also when each HEADERS frame is sent.
Hm...  I'm not sure about this comment.  I think the delegate definitely should
be notified when the HEADERS frame is sent.  I'm fine with OnDataSent being that
mechanism, but if you'd like to add a new OnHeadersSent, that's cool too.

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_test_util_sp...
File net/spdy/spdy_test_util_spdy2.h (right):

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_test_util_sp...
net/spdy/spdy_test_util_spdy2.h:120: const char* const extra_headers[],
THanks for all the whitespace cleanups!

[Takashi Toyoshima, 2012-08-03 05:11:12]

Thanks!
I'll commit this CL.

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:145: if (frame.type == TYPE_DATA)
looks safer.
I follow your idea.

Also, I added NOTREACHED() at the end of this function.

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:103: } data;
Ah, anonymous looks better here.
Thanks!

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_stream_spdy2...
File net/spdy/spdy_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10810069/diff/17006/net/spdy/spdy_stream_spdy2...
net/spdy/spdy_stream_spdy2_unittest.cc:271: // But current implementation
invokes also when each HEADERS frame is sent.
OK, I'll add OnHeadersSent.
WriteData returns user payload size, so caller can do flow control by this
value. But, HEADERS size is unknown to caller, so returning sent HEADERS frame
size via OnDataSent looks confusing.
By the way, I'm now sure what is the right approach to know if WriteData cause
window overflow. Data frames should be limited by window size, but WriteData
always returns ERR_IO_PENDING. I guess caller should retry to call with the same
failed data, right?

[commit-bot: I haz the power, 2012-08-03 05:11:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10810069/20004

[commit-bot: I haz the power, 2012-08-03 07:31:28]

Change committed as 149808