Bug 64522 - WebSocket: Implement hybi framing
: WebSocket: Implement hybi framing
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
: 50099
  Show dependency treegraph
 
Reported: 2011-07-14 05:21 PST by
Modified: 2011-07-19 01:57 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


------- Comment #1 From 2011-07-14 07:03:59 PST -------
Created an attachment (id=100803) [details]
Patch
------- Comment #2 From 2011-07-14 07:08:24 PST -------
This patch is big, but most of the patch is occupied by newly added tests.
------- Comment #3 From 2011-07-14 07:11:05 PST -------
Not sure why style bot turned red. When I run check-webkit-style locally, it passes.
------- Comment #4 From 2011-07-14 23:02:18 PST -------
(From update of attachment 100803 [details])
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 From 2011-07-15 05:27:41 PST -------
Created an attachment (id=100959) [details]
Patch v2
------- Comment #6 From 2011-07-15 05:35:07 PST -------
(From update of attachment 100803 [details])
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 From 2011-07-15 05:35:20 PST -------
(From update of attachment 100959 [details])
Attachment 100959 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9095202
------- Comment #8 From 2011-07-15 06:55:39 PST -------
Oh no, Qt doesn't have UINT64_C...
------- Comment #9 From 2011-07-15 07:03:52 PST -------
(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 From 2011-07-15 18:24:27 PST -------
(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 From 2011-07-18 22:46:59 PST -------
Created an attachment (id=101269) [details]
Patch v3
------- Comment #12 From 2011-07-18 22:51:53 PST -------
(In reply to comment #11)
> Created an attachment (id=101269) [details] [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 From 2011-07-18 22:56:10 PST -------
(From update of attachment 101269 [details])
Attachment 101269 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9149039
------- Comment #14 From 2011-07-18 23:21:08 PST -------
(In reply to comment #13)
> (From update of attachment 101269 [details] [details])
> Attachment 101269 [details] [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/9149039

Umm, Qt still doesn't compile...
------- Comment #15 From 2011-07-19 00:00:32 PST -------
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 From 2011-07-19 00:06:36 PST -------
Created an attachment (id=101281) [details]
Patch v4
------- Comment #17 From 2011-07-19 00:21:00 PST -------
(From update of attachment 101281 [details])
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 From 2011-07-19 00:21:33 PST -------
(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 From 2011-07-19 00:56:52 PST -------
Created an attachment (id=101284) [details]
Patch to land
------- Comment #20 From 2011-07-19 00:59:22 PST -------
(From update of attachment 101281 [details])
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 From 2011-07-19 01:57:45 PST -------
(From update of attachment 101284 [details])
Clearing flags on attachment: 101284

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