Bug 64522 – WebSocket: Implement hybi framing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yuta Kitamura]

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)

[Yuta Kitamura]

Created attachment 100803 [details]
Patch

[Yuta Kitamura]

This patch is big, but most of the patch is occupied by newly added tests.

[Yuta Kitamura]

Not sure why style bot turned red. When I run check-webkit-style locally, it passes.

[Kent Tamura]

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.

[Yuta Kitamura]

Created attachment 100959 [details]
Patch v2

[Yuta Kitamura]

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.

[Early Warning System Bot]

Comment on attachment 100959 [details]
Patch v2

Attachment 100959 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9095202

[Yuta Kitamura]

Oh no, Qt doesn't have UINT64_C...

[Yuta Kitamura]

(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)"?

[Kent Tamura]

(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).

[Yuta Kitamura]

Created attachment 101269 [details]
Patch v3

[Yuta Kitamura]

(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

[Early Warning System Bot]

Comment on attachment 101269 [details]
Patch v3

Attachment 101269 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9149039

[Yuta Kitamura]

(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...

[Yuta Kitamura]

As discussed off-line, we've decided to go with the first version with a slight modification ("ui64" for #if COMPILER(MSVC), "ull" otherwise).

[Yuta Kitamura]

Created attachment 101281 [details]
Patch v4

[Kent Tamura]

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::.

[Kent Tamura]

(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.

[Yuta Kitamura]

Created attachment 101284 [details]
Patch to land

[Yuta Kitamura]

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.

[WebKit Review Bot]

Comment on attachment 101284 [details]
Patch to land

Clearing flags on attachment: 101284

Committed r91243: <http://trac.webkit.org/changeset/91243>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.