Bug 73852 – Expose reusable WebSocket code for WebSocketServer.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jocelyn Turcotte]

Expose reusable WebSocket code for WebSocketServer.

[Jocelyn Turcotte]

Created attachment 117914 [details]
Patch

[Hajime Morrita]

Comment on attachment 117914 [details]
Patch

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

> Source/WebCore/websockets/WebSocketChannel.cpp:959
> +void WebSocketChannel::buildFrame(OpCode opCode, bool maskPayload, const char* data, size_t dataLength, Vector<char>& frame)

Could you elaborate this change?
I couldn't see any reason to have an option to allow non-masked frames.

[Hajime Morrita]

Hi yutak@, toyoshim@, do you have any opinion about this?
Because this change is superficial and I'm OK to rubber-stamping this
Once you guys agree this.

[Takashi Toyoshima]

Comment on attachment 117914 [details]
Patch

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

>> Source/WebCore/websockets/WebSocketChannel.cpp:959
>> +void WebSocketChannel::buildFrame(OpCode opCode, bool maskPayload, const char* data, size_t dataLength, Vector<char>& frame)
> 
> Could you elaborate this change?
> I couldn't see any reason to have an option to allow non-masked frames.

WebSocket's spec say only upstream frames must be masked.
I guess that's why he added this option and agree with it.

[Takashi Toyoshima]

I have no objection to this change.

[Kent Tamura]

Comment on attachment 117914 [details]
Patch

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

>>> Source/WebCore/websockets/WebSocketChannel.cpp:959
>>> -bool WebSocketChannel::sendFrame(OpCode opCode, const char* data, size_t dataLength)
>>> +void WebSocketChannel::buildFrame(OpCode opCode, bool maskPayload, const char* data, size_t dataLength, Vector<char>& frame)

Adding a bool argument is not good.
We had better update the following functions:

* Adding buildFrameHeader()
* Adding buildUnmaskedFrame()
  calling buildFrameHeader() and frame.append(data, dataLength)
* Updating sendFrame()
  calling buildFrameHeader(), masking the payload, and sending it.

[Jocelyn Turcotte]

Created attachment 118841 [details]
Patch

Thanks for having a look. I checked and I would still need a boolean flag in buildFrameHeader to set the mask bit and append the making key. Here is a patch that replaces the boolean flag by an option enum that contains all the flag bit for the frame and use it in FrameData as well instead of individual booleans.

Another option would be to keep it that way and add an enum flag just for the masking, or tell me if you have another idea.

[Jocelyn Turcotte]

By the way, there is a patch in bug #73092 that goes along the same line as this one. If one of you guys could have a look that would probably be the best.

[WebKit Review Bot]

Comment on attachment 118841 [details]
Patch

Attachment 118841 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10848074

New failing tests:
http/tests/websocket/tests/hybi/reserved-bits.html

[Yuta Kitamura]

Comment on attachment 118841 [details]
Patch

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

The patch generally looks fine to me. If the failure from EWS is real, you need to investigate and fix it. [Note: I'm not a WebKit reviewer.]

> Source/WebCore/websockets/WebSocketChannel.h:145
> +    static ParseFrameResult parseFrame(const char* data, size_t dataLength, FrameData&, String& errorString); // May modify part of "data" to unmask the frame.

The type of |data| should be char* (non-const) to indicate the content of |data| may be modified.

[Yuta Kitamura]

Comment on attachment 118841 [details]
Patch

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

> Source/WebCore/websockets/WebSocketChannel.cpp:634
> +    if (isControlOpCode(frame.opCode) && !frame.options & FrameOptionFinal) {

Do you mean !(frame.options & FrameOptionFinal) ?

[Kent Tamura]

(In reply to comment #7)
> Thanks for having a look. I checked and I would still need a boolean flag in buildFrameHeader to set the mask bit and append the making key. Here is a patch that replaces the boolean flag by an option enum that contains all the flag bit for the frame and use it in FrameData as well instead of individual booleans.
> 
> Another option would be to keep it that way and add an enum flag just for the masking, or tell me if you have another idea.

ok, I see.  The FrameOptions idea looks nice.

[Jocelyn Turcotte]

(In reply to comment #10)
> The patch generally looks fine to me. If the failure from EWS is real, you need to investigate and fix it. [Note: I'm not a WebKit reviewer.]

This test was passing on other platform since WTF String would convert the bit-ANDed value automatically to bool for some reason. On chromium the error message was something like "One or more reserved bits are on: reserved1 = 2, reserved2 = 4, reserved3 = 8" instead of "...= 1, ...= 1, ...= 1". It should work now.

> The type of |data| should be char* (non-const) to indicate the content of |data| may be modified.

Good idea.

(In reply to comment #11)
> Do you mean !(frame.options & FrameOptionFinal) ?

This is embarrassing, nice catch.
A new patch is following.

[Jocelyn Turcotte]

Created attachment 119061 [details]
Patch

[Kent Tamura]

Comment on attachment 119061 [details]
Patch

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

> Source/WebCore/websockets/WebSocketChannel.cpp:629
> -        fail("One or more reserved bits are on: reserved1 = " + String::number(frame.reserved1) + ", reserved2 = " + String::number(frame.reserved2) + ", reserved3 = " + String::number(frame.reserved3));
> +    if (frame.options & (FrameOptionReserved1 | FrameOptionReserved2 | FrameOptionReserved3)) {
> +        fail("One or more reserved bits are on: reserved1 = " + String::number(!!(frame.options & FrameOptionReserved1)) + ", reserved2 = " + String::number(!!(frame.options & FrameOptionReserved2)) + ", reserved3 = " + String::number(!!(frame.options & FrameOptionReserved3)));

nit: We have String::format().

[Jocelyn Turcotte]

(In reply to comment #15)
> nit: We have String::format().

Thanks! Will do.

[Jocelyn Turcotte]

Created attachment 134613 [details]
Patch

Taking some time to iterate over the inspector server patches again.
I had to rebase this patch. The code has now been moved to the new WebSocketFrame instead of exposing straight static methods.

[Kent Tamura]

Comment on attachment 134613 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocketFrame.cpp:2
> + * Copyright (C) 2012 Nokia Corporation and/or its subsidiary(-ies)

Please respect the copyright of the original code.

[Jocelyn Turcotte]

Created attachment 134749 [details]
Patch

Preserve the copyrights for WebSocketFrame.cpp

[Kent Tamura]

Comment on attachment 134749 [details]
Patch

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

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:589
> -    if (WebSocketFrame::isControlOpCode(frame.opCode) && frame.payloadLength > maxPayloadLengthWithoutExtendedLengthField) {
> +    if (WebSocketFrame::isControlOpCode(frame.opCode) && frame.payloadLength > WebSocketFrame::maxPayloadLengthWithoutExtendedLengthField) {

I recommend adding WebSocketFrame::isControlFrameWithTooLongPayload(), and hide maxPayloadLengthWithoutExtendedLengthField.

[Jocelyn Turcotte]

Created attachment 135010 [details]
Patch

Expose maxPayloadLengthWithoutExtendedLengthField as WebSocketFrame::needsExtendedLengthField to make it consistent with the other struct helpers. Tell me if that's alright.
WebSocketFrame might also look better as a proper class instead of those helpers.

[Kent Tamura]

Comment on attachment 135010 [details]
Patch

ok

[Jocelyn Turcotte]

Committed r113025: <http://trac.webkit.org/changeset/113025>