Bug 64522 - WebSocket: Implement hybi framing
: WebSocket: Implement hybi framing
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Yuta Kitamura
:
Depends on:
Blocks: 50099
  Show dependency treegraph
 
Reported: 2011-07-14 05:21 PDT by Yuta Kitamura
Modified: 2011-07-19 01:57 PDT (History)
4 users (show)

See Also:


Attachments
Patch (68.84 KB, patch)
2011-07-14 07:03 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v2 (71.51 KB, patch)
2011-07-15 05:27 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v3 (71.48 KB, patch)
2011-07-18 22:46 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch v4 (71.55 KB, patch)
2011-07-19 00:06 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff
Patch to land (72.64 KB, patch)
2011-07-19 00:56 PDT, Yuta Kitamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yuta Kitamura 2011-07-14 05:21:13 PDT
Implement hybi WebSocket framing.

http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-10#section-4 (Data Framing)
http://tools.ietf.org/html/draft-ietf-hybi-thewebsocketprotocol-10#section-6 (Sending and Receiving Data)
Comment 1 Yuta Kitamura 2011-07-14 07:03:59 PDT
Created attachment 100803 [details]
Patch
Comment 2 Yuta Kitamura 2011-07-14 07:08:24 PDT
This patch is big, but most of the patch is occupied by newly added tests.
Comment 3 Yuta Kitamura 2011-07-14 07:11:05 PDT
Not sure why style bot turned red. When I run check-webkit-style locally, it passes.
Comment 4 Kent Tamura 2011-07-14 23:02:18 PDT
Comment on attachment 100803 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100803&action=review

> Source/WebCore/websockets/WebSocketChannel.cpp:412
> +const WebSocketChannel::OpCode WebSocketChannel::OpCodeContinuation = 0x0;
> +const WebSocketChannel::OpCode WebSocketChannel::OpCodeText = 0x1;
> +const WebSocketChannel::OpCode WebSocketChannel::OpCodeBinary = 0x2;
> +const WebSocketChannel::OpCode WebSocketChannel::OpCodeClose = 0x8;
> +const WebSocketChannel::OpCode WebSocketChannel::OpCodePing = 0x9;
> +const WebSocketChannel::OpCode WebSocketChannel::OpCodePong = 0xA;
> +

I think we had better move them to the upper place of this file.
These symbols are used before here, and I'm afraid defining them here prevents constant value folding optimization.

> Source/WebCore/websockets/WebSocketChannel.cpp:413
> +bool WebSocketChannel::processFrame()

This function is too large.  Please consider splitting it.

> Source/WebCore/websockets/WebSocketChannel.cpp:417
> +    char* p = m_buffer;
> +    char* end = m_buffer + m_bufferSize;

'p' and 'end' should be 'const char*'.
Use another non-const variable or const_cast<> when we unmask the payload.

'end' should be 'bufferEnd' to avoid confusion with frameEnd'.

> Source/WebCore/websockets/WebSocketChannel.cpp:420
> +    if (m_bufferSize < 2) // Frame incomplete.
> +        return false;

The comment should be a complete sentence as possible.  Probably "The frame is incomplete."

> Source/WebCore/websockets/WebSocketChannel.cpp:434
> +    if (payloadLength64 >= 126) {
> +        int extendedPayloadLengthSize = payloadLength64 == 126 ? 2 : 8;

126 is a magic number.
Please introduce a meaningful name.

> Source/WebCore/websockets/WebSocketChannel.cpp:448
> +#if OS(WINDOWS)
> +    static const uint64_t maxPayloadLength = 0x7FFFFFFFFFFFFFFFui64;
> +#else
> +    static const uint64_t maxPayloadLength = 0x7FFFFFFFFFFFFFFFull;
> +#endif

It's not OS-dependent but compiler-dependent.

You assume uint64_t is available on any platforms.  So can you use UINT64_C(0x7FFFFFFFFFFFFFFF) ?

> Source/WebCore/websockets/WebSocketChannel.cpp:449
> +    size_t maskingKeyLength = masked ? 4 : 0;

4 is a magic number.

> Source/WebCore/websockets/WebSocketChannel.cpp:464
> +    char* payload = p + maskingKeyLength;
> +    char* frameEnd = p + maskingKeyLength + payloadLength;

They should be const char*.

> Source/WebCore/websockets/WebSocketChannel.cpp:677
> +    frame.append(0x80 | opCode); // 0x80 is for "fin" bit; we do not fragment a frame on sending.
> +    if (dataLength < 126)
> +        frame.append(0x80 | dataLength); // 0x80 is for "MASK" bit; a client must mask frames so it is always on.

So we should have FinBit = 0x80 and MaskBit = 0x80, or something like them.

> Source/WebCore/websockets/WebSocketChannel.cpp:703
> +    frame.append("....", 4); // Four-byte placeholder for masking key. Will be overwritten.
> +    size_t payloadStart = frame.size();
> +    frame.append(data, dataLength);
> +
> +    cryptographicallyRandomValues(frame.data() + maskingKeyStart, 4);
> +    for (size_t i = 0; i < dataLength; ++i)
> +        frame[payloadStart + i] ^= frame[maskingKeyStart + i % 4];

4 is a magic number.
Comment 5 Yuta Kitamura 2011-07-15 05:27:41 PDT
Created attachment 100959 [details]
Patch v2
Comment 6 Yuta Kitamura 2011-07-15 05:35:07 PDT
Comment on attachment 100803 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100803&action=review

>> Source/WebCore/websockets/WebSocketChannel.cpp:412
>> +
> 
> I think we had better move them to the upper place of this file.
> These symbols are used before here, and I'm afraid defining them here prevents constant value folding optimization.

Fixed.

>> Source/WebCore/websockets/WebSocketChannel.cpp:413
>> +bool WebSocketChannel::processFrame()
> 
> This function is too large.  Please consider splitting it.

The first half has been moved to a new function parseFrame().

>> Source/WebCore/websockets/WebSocketChannel.cpp:417
>> +    char* end = m_buffer + m_bufferSize;
> 
> 'p' and 'end' should be 'const char*'.
> Use another non-const variable or const_cast<> when we unmask the payload.
> 
> 'end' should be 'bufferEnd' to avoid confusion with frameEnd'.

Fixed.

>> Source/WebCore/websockets/WebSocketChannel.cpp:420
>> +        return false;
> 
> The comment should be a complete sentence as possible.  Probably "The frame is incomplete."

Removed this comment because this "return" statement has been changed to "return FrameIncomplete;" (FrameIncomplete is a member of a new enum ParseFrameResult) and the code looked self-documenting to me.

>> Source/WebCore/websockets/WebSocketChannel.cpp:434
>> +        int extendedPayloadLengthSize = payloadLength64 == 126 ? 2 : 8;
> 
> 126 is a magic number.
> Please introduce a meaningful name.

Fixed. I gave names to 125, 126 and 127.

>> Source/WebCore/websockets/WebSocketChannel.cpp:448
>> +#endif
> 
> It's not OS-dependent but compiler-dependent.
> 
> You assume uint64_t is available on any platforms.  So can you use UINT64_C(0x7FFFFFFFFFFFFFFF) ?

I didn't know UINT64_C, and it seems to work fine. Fixed.

>> Source/WebCore/websockets/WebSocketChannel.cpp:449
>> +    size_t maskingKeyLength = masked ? 4 : 0;
> 
> 4 is a magic number.

Created a new constant.

>> Source/WebCore/websockets/WebSocketChannel.cpp:464
>> +    char* frameEnd = p + maskingKeyLength + payloadLength;
> 
> They should be const char*.

Fixed.

>> Source/WebCore/websockets/WebSocketChannel.cpp:677
>> +        frame.append(0x80 | dataLength); // 0x80 is for "MASK" bit; a client must mask frames so it is always on.
> 
> So we should have FinBit = 0x80 and MaskBit = 0x80, or something like them.

Fixed.

>> Source/WebCore/websockets/WebSocketChannel.cpp:703
>> +        frame[payloadStart + i] ^= frame[maskingKeyStart + i % 4];
> 
> 4 is a magic number.

Fixed.
Comment 7 Early Warning System Bot 2011-07-15 05:35:20 PDT
Comment on attachment 100959 [details]
Patch v2

Attachment 100959 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9095202
Comment 8 Yuta Kitamura 2011-07-15 06:55:39 PDT
Oh no, Qt doesn't have UINT64_C...
Comment 9 Yuta Kitamura 2011-07-15 07:03:52 PDT
(In reply to comment #8)
> Oh no, Qt doesn't have UINT64_C...

I'm not sure about how to resolve this. Maybe I should use "#if PLATFORM(QT)"?
Comment 10 Kent Tamura 2011-07-15 18:24:27 PDT
(In reply to comment #9)
> (In reply to comment #8)
> > Oh no, Qt doesn't have UINT64_C...
> 
> I'm not sure about how to resolve this. Maybe I should use "#if PLATFORM(QT)"?

How about including stdint.h explicitly?
BTW, I wonder whether UINT64_C is available on Chromium-win (VC++ 2008).
Comment 11 Yuta Kitamura 2011-07-18 22:46:59 PDT
Created attachment 101269 [details]
Patch v3
Comment 12 Yuta Kitamura 2011-07-18 22:51:53 PDT
(In reply to comment #11)
> Created an attachment (id=101269) [details]
> Patch v3

Added #include <stdint.h>.

I posted try jobs to Chromium tryserver to see if this patch works (they are not finished yet).

http://build.chromium.org/p/tryserver.chromium/builders/linux_layout/builds/953
http://build.chromium.org/p/tryserver.chromium/builders/mac_layout/builds/764
http://build.chromium.org/p/tryserver.chromium/builders/win_layout/builds/1125
Comment 13 Early Warning System Bot 2011-07-18 22:56:10 PDT
Comment on attachment 101269 [details]
Patch v3

Attachment 101269 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9149039
Comment 14 Yuta Kitamura 2011-07-18 23:21:08 PDT
(In reply to comment #13)
> (From update of attachment 101269 [details])
> Attachment 101269 [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/9149039

Umm, Qt still doesn't compile...
Comment 15 Yuta Kitamura 2011-07-19 00:00:32 PDT
As discussed off-line, we've decided to go with the first version with a slight modification ("ui64" for #if COMPILER(MSVC), "ull" otherwise).
Comment 16 Yuta Kitamura 2011-07-19 00:06:36 PDT
Created attachment 101281 [details]
Patch v4
Comment 17 Kent Tamura 2011-07-19 00:21:00 PDT
Comment on attachment 101281 [details]
Patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=101281&action=review

> Source/WebCore/websockets/WebSocketChannel.cpp:466
> +#if COMPILER(MSVC)
> +    static const uint64_t maxPayloadLength = 0x7FFFFFFFFFFFFFFFui64;
> +#else
> +    static const uint64_t maxPayloadLength = 0x7FFFFFFFFFFFFFFFull;
> +#endif

nit: It's nice to have a comment that we wanted to use UINT64_C.

> Source/WebCore/websockets/WebSocketChannel.cpp:468
> +    if (payloadLength64 > maxPayloadLength || payloadLength64 + maskingKeyLength > std::numeric_limits<size_t>::max()) {

nit: We usually declare "using namespace std;" at the beginning of the file and omit std::.
Comment 18 Kent Tamura 2011-07-19 00:21:33 PDT
(In reply to comment #17)
> nit: It's nice to have a comment that we wanted to use UINT64_C.

I meant a FIXME comment.
Comment 19 Yuta Kitamura 2011-07-19 00:56:52 PDT
Created attachment 101284 [details]
Patch to land
Comment 20 Yuta Kitamura 2011-07-19 00:59:22 PDT
Comment on attachment 101281 [details]
Patch v4

View in context: https://bugs.webkit.org/attachment.cgi?id=101281&action=review

>> Source/WebCore/websockets/WebSocketChannel.cpp:466
>> +#endif
> 
> nit: It's nice to have a comment that we wanted to use UINT64_C.

Added.

>> Source/WebCore/websockets/WebSocketChannel.cpp:468
>> +    if (payloadLength64 > maxPayloadLength || payloadLength64 + maskingKeyLength > std::numeric_limits<size_t>::max()) {
> 
> nit: We usually declare "using namespace std;" at the beginning of the file and omit std::.

Fixed. Removed a couple of occurrences of "std::" in this file.
Comment 21 WebKit Review Bot 2011-07-19 01:57:45 PDT
Comment on attachment 101284 [details]
Patch to land

Clearing flags on attachment: 101284

Committed r91243: <http://trac.webkit.org/changeset/91243>
Comment 22 WebKit Review Bot 2011-07-19 01:57:51 PDT
All reviewed patches have been landed.  Closing bug.