Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10796107:
WebSocketFrameChunk should use IOBuffer to hold data (Closed)
|
Created:
8 years, 5 months ago by Takashi Toyoshima Modified:
8 years, 4 months ago CC:
bashi, chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNow WebSocketFrameChunk use std::vector<char> to hold payload data.
But, SpdyStream and StreamSocket use IOBuffer in their interfaces.
To reduce memory copies, we should use IOBuffer to hold data.
BUG=none
TEST=net_unittests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149387
Patch Set 1 #
Total comments: 8
Patch Set 2 : reflect yutak's review #
Total comments: 5
Patch Set 3 : revised #
Total comments: 8
Patch Set 4 : again #Patch Set 5 : handle size 0 body #Patch Set 6 : s/uint64_t/uint64/g #
Total comments: 5
Patch Set 7 : use begin() and end() instead of data() for android #Patch Set 8 : again #Patch Set 9 : fail on large frames #Patch Set 10 : rebase #Patch Set 11 : fix test expectation #
Total comments: 9
Patch Set 12 : style nits #
Messages
Total messages: 28 (0 generated)
Hi Yuta, What do you think about this change? Currently, I use WebSocketFrameChunk in my SpdyWebSocketStream implementation experimentally. There, I notice that I should pass payload data to SpdyStream as a IOBuffer reference. Also StreamSocket seems to have a sending interface which uses IOBuffer. So, how about using IOBuffer instead of using std::vector<char>? I feel it will contribute to reduce memory copy situations.
I think it's fine to use IOBuffer here. Want to hear from Will and Matt. https://chromiumcodereview.appspot.com/10796107/diff/1/net/websockets/websock... File net/websockets/websocket_frame.h (right): https://chromiumcodereview.appspot.com/10796107/diff/1/net/websockets/websock... net/websockets/websocket_frame.h:80: scoped_refptr<IOBuffer> data; You need a field for |size|, too. https://chromiumcodereview.appspot.com/10796107/diff/1/net/websockets/websock... File net/websockets/websocket_frame_parser_unittest.cc (left): https://chromiumcodereview.appspot.com/10796107/diff/1/net/websockets/websock... net/websockets/websocket_frame_parser_unittest.cc:349: EXPECT_EQ(std::vector<char>(), frame->data); Why did you remove this? https://chromiumcodereview.appspot.com/10796107/diff/1/net/websockets/websock... File net/websockets/websocket_frame_parser_unittest.cc (right): https://chromiumcodereview.appspot.com/10796107/diff/1/net/websockets/websock... net/websockets/websocket_frame_parser_unittest.cc:70: EXPECT_TRUE(!memcmp(kHello, frame->data->data(), kHelloLength)); EXPECT_EQ(0, memcmp(...)) might be slightly better (it prints the result of memcmp on failure). (I'd use std::equal instead of memcmp but I'm probably too picky about using C functions) It's kind of sad about losing the ability of dumping every char on failure. There's no easy way to do the same on IOBuffer, though.
Yuta, Thank you for comments. I upload revised version. Matt, and Will, Could you sanctify this change? https://chromiumcodereview.appspot.com/10796107/diff/1/net/websockets/websock... File net/websockets/websocket_frame.h (right): https://chromiumcodereview.appspot.com/10796107/diff/1/net/websockets/websock... net/websockets/websocket_frame.h:80: scoped_refptr<IOBuffer> data; On 2012/07/26 09:48:48, Yuta Kitamura wrote: > You need a field for |size|, too. Done. https://chromiumcodereview.appspot.com/10796107/diff/1/net/websockets/websock... File net/websockets/websocket_frame_parser_unittest.cc (left): https://chromiumcodereview.appspot.com/10796107/diff/1/net/websockets/websock... net/websockets/websocket_frame_parser_unittest.cc:349: EXPECT_EQ(std::vector<char>(), frame->data); I removed it because it didn't have buffer size information. Now, I revive it as a size check. https://chromiumcodereview.appspot.com/10796107/diff/1/net/websockets/websock... File net/websockets/websocket_frame_parser_unittest.cc (right): https://chromiumcodereview.appspot.com/10796107/diff/1/net/websockets/websock... net/websockets/websocket_frame_parser_unittest.cc:70: EXPECT_TRUE(!memcmp(kHello, frame->data->data(), kHelloLength)); Thanks. std::equal seems to work better. I'd use it. So can I still use EXPECT_TRUE for std::equal? Failure results of EXPECT_TRUE and EXPECT_EQ seems to be the same one to std::equal.
http://codereview.chromium.org/10796107/diff/8001/net/websockets/websocket_fr... File net/websockets/websocket_frame.h (right): http://codereview.chromium.org/10796107/diff/8001/net/websockets/websocket_fr... net/websockets/websocket_frame.h:83: uint64_t size; Do we really need a 64-bit unsigned integer for size? My feeling is that unless we're actually going to support chunks that large, and plumb the support all the way through, then we should use use ints. Makes it much clearer that we don't actually support huge chunks, and the limits the code should check for. Our socket classes don't support either reads or writes greater than MAX_INT, anyways, so we'd either have to handle breaking up a huge number into ints when sending it (A waste of time, easy to get wrong, and something we're not going to have unit tests for), or explicitly not support numbers that large further down the pipe, anyways. May just want to use IOBufferWithSize.
http://codereview.chromium.org/10796107/diff/1/net/websockets/websocket_frame... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/10796107/diff/1/net/websockets/websocket_frame... net/websockets/websocket_frame_parser_unittest.cc:70: EXPECT_TRUE(!memcmp(kHello, frame->data->data(), kHelloLength)); On 2012/07/26 12:39:05, toyoshim wrote: > Thanks. > > std::equal seems to work better. I'd use it. > So can I still use EXPECT_TRUE for std::equal? > Failure results of EXPECT_TRUE and EXPECT_EQ seems to be the same one to > std::equal. If you use std::equal, using EXPECT_TRUE is just fine (return value of std::equal is a bool, nothing more). http://codereview.chromium.org/10796107/diff/8001/net/websockets/websocket_fr... File net/websockets/websocket_frame.h (right): http://codereview.chromium.org/10796107/diff/8001/net/websockets/websocket_fr... net/websockets/websocket_frame.h:83: uint64_t size; Agreed, at least |size| should be an int. I think the trade off between IOBuffer+int and IOBufferWithSize here is: - With IOBuffer+int we can use any subclass of IOBuffer for |data|. - With IOBufferWithSize we should always use IOBufferWithSize. I don't think of any use case that requires the flexibility of the former, so IOBufferWithSize would be just fine. http://codereview.chromium.org/10796107/diff/8001/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/10796107/diff/8001/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:71: std::vector<char> expected_data(kHello, kHello + kHelloLength); expected_data isn't used.
Thanks. I fixed the change to use IOBufferWithSize. http://codereview.chromium.org/10796107/diff/1/net/websockets/websocket_frame... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/10796107/diff/1/net/websockets/websocket_frame... net/websockets/websocket_frame_parser_unittest.cc:70: EXPECT_TRUE(!memcmp(kHello, frame->data->data(), kHelloLength)); OK. I use EXPECT_TRUE. http://codereview.chromium.org/10796107/diff/8001/net/websockets/websocket_fr... File net/websockets/websocket_frame.h (right): http://codereview.chromium.org/10796107/diff/8001/net/websockets/websocket_fr... net/websockets/websocket_frame.h:83: uint64_t size; This payload data represents a object in JavaScript, and it's impossible to handle a large object which size is over 32-bit. So, I agreed to use IOBufferWithSize here. http://codereview.chromium.org/10796107/diff/8001/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/10796107/diff/8001/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:71: std::vector<char> expected_data(kHello, kHello + kHelloLength); Oops. Thank you for catching this.
http://codereview.chromium.org/10796107/diff/14001/net/websockets/websocket_f... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/10796107/diff/14001/net/websockets/websocket_f... net/websockets/websocket_frame_parser.cc:181: current_frame_header_->payload_length - frame_offset_); Might want to DCHECK_LE(next_size, std::numeric_limits<int>::max()); http://codereview.chromium.org/10796107/diff/14001/net/websockets/websocket_f... net/websockets/websocket_frame_parser.cc:188: frame_chunk->data = new IOBufferWithSize(next_size); This doesn't give a warning without casting next_size? http://codereview.chromium.org/10796107/diff/14001/net/websockets/websocket_f... net/websockets/websocket_frame_parser.cc:189: DCHECK(frame_chunk->data); Not needed. We crash on any allocation failure. http://codereview.chromium.org/10796107/diff/14001/net/websockets/websocket_f... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/10796107/diff/14001/net/websockets/websocket_f... net/websockets/websocket_frame_parser_unittest.cc:71: EXPECT_EQ(static_cast<int>(kHelloLength), frame->data->size()); Might want to make these ASSERT_EQs instead, since otherwise on failure, the next lines could result in a crash, which will prevent any subsequent tests from running.
Hi Matt. Thank you for quick response. I revised my change again. http://codereview.chromium.org/10796107/diff/14001/net/websockets/websocket_f... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/10796107/diff/14001/net/websockets/websocket_f... net/websockets/websocket_frame_parser.cc:181: current_frame_header_->payload_length - frame_offset_); On 2012/07/27 14:25:55, Matt Menke wrote: > Might want to DCHECK_LE(next_size, std::numeric_limits<int>::max()); Done. http://codereview.chromium.org/10796107/diff/14001/net/websockets/websocket_f... net/websockets/websocket_frame_parser.cc:188: frame_chunk->data = new IOBufferWithSize(next_size); Humm... my local linux build passes it without any warning. But, as you said, I should apply a casting. Thanks. http://codereview.chromium.org/10796107/diff/14001/net/websockets/websocket_f... net/websockets/websocket_frame_parser.cc:189: DCHECK(frame_chunk->data); On 2012/07/27 14:25:55, Matt Menke wrote: > Not needed. We crash on any allocation failure. Done. http://codereview.chromium.org/10796107/diff/14001/net/websockets/websocket_f... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/10796107/diff/14001/net/websockets/websocket_f... net/websockets/websocket_frame_parser_unittest.cc:71: EXPECT_EQ(static_cast<int>(kHelloLength), frame->data->size()); On 2012/07/27 14:25:55, Matt Menke wrote: > Might want to make these ASSERT_EQs instead, since otherwise on failure, the > next lines could result in a crash, which will prevent any subsequent tests from > running. Done.
LGTM
Oops, DCHECK failed in IOBuffer because it doesn't accept size = 0. I need some modification to handle this.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10796107/15002
Try job failure for 10796107-15002 (retry) on android for steps "compile, build" (clobber build). It's a second try, previously, steps "compile, build" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...
drive by security concern http://codereview.chromium.org/10796107/diff/8004/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/10796107/diff/8004/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:183: DCHECK_LE(next_size, static_cast<uint64>(std::numeric_limits<int>::max())); nit: Is this more appropriate as a CHECK, rather than a DCHECK()? Or even better, something that can/should be handled at the protocol layer since it reflects "user" input (well, network input). It seems very dangerous to have this simply be a DCHECK, due to the interaction between line 191 (which allocates clamped on int) and line 193 (which copies clamped on size_t). http://codereview.chromium.org/10796107/diff/8004/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:197: // machine word (instead of each byte). FWIW, when you do go and begin to (micro?)-optimize, it might be worth looking at _mm_xor_si128 , which is an SSE2 instruction that lets you xor based on quadwords, provided the quadwords are aligned properly (support for which is being added by DaleCurtis in http://codereview.chromium.org/10828054/ ). The media/ code already has selective functionality based on CPU discovery (eg: using SSE2 -> MMX -> hand-unrolled). However, if you can guaranteed aligned allocation (~line 191), hand-calling SSE2 will be the most performant unrolling possible, better than the compiler would be legally allowed to do.
http://codereview.chromium.org/10796107/diff/8004/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/10796107/diff/8004/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:183: DCHECK_LE(next_size, static_cast<uint64>(std::numeric_limits<int>::max())); Agreed. RFC defines status code 1009 meaning "Message Too Big". When a client receives a too big frame to receive, we can just dispose it, then close the connection with the code 1009. CHECK looks better than DCHECK here. But if I'd implement graceful error handling, this component should report an error to upper layer component. Currently, this component stop to decode by |failed_| flag. I use this failure status for now, and will introduce more concrete error code handling in another change. http://codereview.chromium.org/10796107/diff/8004/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:197: // machine word (instead of each byte). Wow, he made many interesting optimizations. Thank you for good information. We'll check his changes to support optimization.
After Matt's LGTM, I update this change at following points. - IOBuffer doesn't allow 0 size body, so I decide to keep null pointer on |data| for 0 size body. - use begin() and end() for vector<> in tests because android port doesn't allow to use data() for vector<>. - add payload_length check to assure that decoding body data size is always less than or equal to int32max (reflects Ryan's comment). Matt, could you check this change again?
http://codereview.chromium.org/10796107/diff/8004/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/10796107/diff/8004/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:183: DCHECK_LE(next_size, static_cast<uint64>(std::numeric_limits<int>::max())); On 2012/07/30 08:01:21, toyoshim wrote: > Agreed. > > RFC defines status code 1009 meaning "Message Too Big". > When a client receives a too big frame to receive, we can just dispose it, then > close the connection with the code 1009. > > CHECK looks better than DCHECK here. But if I'd implement graceful error > handling, this component should report an error to upper layer component. > > Currently, this component stop to decode by |failed_| flag. I use this failure > status for now, and will introduce more concrete error code handling in another > change. Just thought I'd point out that these don't correspond to over the wire frames, but rather Chrome-created chunks which are subsets of a frame's body, so this will not prevent us from handling large frames. Since these are only created by the browser process from data received from the stream (Where we use integer-sized read buffers), or received from the renderer process (And there's a max IPC length), this code should be fine. That having been said, I think a CHECK is reasonable.
In my last change (Patch Set 11), DCHECK failure case is blocked in DecodeFrameHeader() line 147. There, WebSocketFrameParser set |failed_| flag and stop decode forever for large frames. Thus |payload_length| is always less than or equal to kint32max at DecodeFramePayload(). and DCHECK at line 188 is never failed. This is as safe as using CHECK and leave a chance to send back closing handshake with 'message too big' error code to the server. But, int32max is still large. We should define proper size later to avoid OOM crash and send back error gracefully.
http://codereview.chromium.org/10796107/diff/5008/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/10796107/diff/5008/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:125: bool message_too_big = false; Why is this needed? We don't read the entire message at once, but rather the headers, and then one chunk at a time, where a chunk is some subset of the frame's body.
Thank you for comments. And I'm sorry if I misunderstood your comments. The next reply will be after one hour because I'll board a train to go home now. https://chromiumcodereview.appspot.com/10796107/diff/5008/net/websockets/webs... File net/websockets/websocket_frame_parser.cc (right): https://chromiumcodereview.appspot.com/10796107/diff/5008/net/websockets/webs... net/websockets/websocket_frame_parser.cc:125: bool message_too_big = false; One WebSocket frame represents one JavaScript object, which is one of DOMString, ArrayBuffer, and Blob. So, I think supporting over 31bit frames is meaningless. memo: To be precise, one JavaScript object may be splited into multiple WebSocket frames. Anyway, original object should be larger or equal to a frame payload length. https://chromiumcodereview.appspot.com/10796107/diff/5008/net/websockets/webs... net/websockets/websocket_frame_parser.cc:144: if (payload_length > static_cast<uint64>(kint32max)) So I think we should set proper value here. kint32max is not realistic to create a JavaScript object. https://chromiumcodereview.appspot.com/10796107/diff/5008/net/websockets/webs... net/websockets/websocket_frame_parser.cc:188: DCHECK_LE(next_size, static_cast<uint64>(kint32max)); But, splitting incoming large frames into multiple int32max size chunks is another possible approach. We can handle unexpected large message at upper layer by checking the fist chunk's payload_length header.
https://chromiumcodereview.appspot.com/10796107/diff/5008/net/websockets/webs... File net/websockets/websocket_frame_parser.cc (right): https://chromiumcodereview.appspot.com/10796107/diff/5008/net/websockets/webs... net/websockets/websocket_frame_parser.cc:144: if (payload_length > static_cast<uint64>(kint32max)) On 2012/07/30 14:42:42, toyoshim wrote: > So I think we should set proper value here. > kint32max is not realistic to create a JavaScript object. Err...Sorry, I was thinking WebSockets were streaming sockets, in which case we could pass the data along to Javascript in byte-sized chunks, but looks like they're message/datagram-base, correct?
On 2012/07/30 14:55:53, Matt Menke wrote: > https://chromiumcodereview.appspot.com/10796107/diff/5008/net/websockets/webs... > File net/websockets/websocket_frame_parser.cc (right): > > https://chromiumcodereview.appspot.com/10796107/diff/5008/net/websockets/webs... > net/websockets/websocket_frame_parser.cc:144: if (payload_length > > static_cast<uint64>(kint32max)) > On 2012/07/30 14:42:42, toyoshim wrote: > > So I think we should set proper value here. > > kint32max is not realistic to create a JavaScript object. > > Err...Sorry, I was thinking WebSockets were streaming sockets, in which case we > could pass the data along to Javascript in byte-sized chunks, but looks like > they're message/datagram-base, correct? Err...The Javascript interface is message/datagram based, that is. The network connection itself is over TCP, of course.
Yes. WebSocket is not a stream based protocol, but a message oriented one. Also JavaScript interface is message based one and it's impossible to receive a big object step by step. One message appears as one object. So finally we gather these chunks and build one object for JavaScript onreceive event.
On 2012/07/30 15:57:59, toyoshim wrote: > Yes. WebSocket is not a stream based protocol, but a message oriented one. > Also JavaScript interface is message based one and it's impossible to receive a > big object step by step. One message appears as one object. So finally we gather > these chunks and build one object for JavaScript onreceive event. Thanks for the clarification, and I apologize for the confusion - been a while since I read the docs. I should carefully re-read them. Setting a maximum message length makes a lot of sense, then.
Memo: At least, I'll reuse these components in following implementation. - flip server to support WebSocket over SPDY as a reverse proxy. I'll reuse this parser to convert frames from backend servers to SPDY frames for clients. - Chrome to support WebSocket over SPDY with old WebSocket codes. I'll reuse this parser to convert sending WebSocket frames to SPDY frames in SpdyWebSocketStream. Anyway, it's not important to support large frames having over 31bit length.
Code still LGTM. http://codereview.chromium.org/10796107/diff/5008/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/10796107/diff/5008/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:48: bool WebSocketFrameParser::Decode( Just a random note for the future: At some point we may want to return net error codes, for logging, histograms, and maybe use with devtools. http://codereview.chromium.org/10796107/diff/5008/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/10796107/diff/5008/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:336: EXPECT_EQ(1u, frames.size()); Suggest you use braces around these code blocks. Not strictly necessary, but with macros, seems like a good idea to play it safe.
Thank you, Matt. I'll land this CL with fixing nits you found. http://codereview.chromium.org/10796107/diff/5008/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser.cc (right): http://codereview.chromium.org/10796107/diff/5008/net/websockets/websocket_fr... net/websockets/websocket_frame_parser.cc:48: bool WebSocketFrameParser::Decode( I'm going to use WebSocket status code for failure reason in the next change. But, I don't stick on that. Reusing net error codes, or adding some proper code to them looks fine. Let's discuss this in the next CL. http://codereview.chromium.org/10796107/diff/5008/net/websockets/websocket_fr... File net/websockets/websocket_frame_parser_unittest.cc (right): http://codereview.chromium.org/10796107/diff/5008/net/websockets/websocket_fr... net/websockets/websocket_frame_parser_unittest.cc:336: EXPECT_EQ(1u, frames.size()); Thank you for catching this. I agreed. I fixed this and other same errors in this file.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/toyoshim@chromium.org/10796107/13016
Change committed as 149387 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
