Bug 78682 – [WebSocket] Move WebSocketChannel::FrameData into a separate header file

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

[Kenichi Ishibashi]

This brings flexibility to add classes which want to do something for incoming/outgoing frames (e.g. compression/decompression).

[Kenichi Ishibashi]

Created attachment 127133 [details]
WIP patch

[Kenichi Ishibashi]

(In reply to comment #1)
> Created an attachment (id=127133) [details]
> WIP patch

Yuta-san, could you take a look?

Some notes:
- Should WebSocketFrame be a class (not struct) and should I add setter/getter methods?
- I removed frameEnd field and added m_currentFrameEnd to WebSocketChannel. I ran layout tests, but not sure it is safe or not.

[Yuta Kitamura]

Comment on attachment 127133 [details]
WIP patch

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

I think your patch is in the right direction.

Re struct vs class, I guess current WebSocketFrame is simple enough to leave it as a struct.

> Source/WebCore/websockets/WebSocketChannel.cpp:606
> +    m_currentFrameEnd = p + maskingKeyLength + payloadLength;

Ending position of a frame has nothing to do with the channel's state, so it sounds strange to have it as a member variable.

Rather, I prefer to have it as an output parameter of parseFrame() function:

    parseFrame(WebSocketFrame& frame, const char*& frameEnd) { ... }

> Source/WebCore/websockets/WebSocketChannel.cpp:978
> +    frame.opCode = opCode;
> +    frame.final = true;
> +    frame.masked = true;
> +    frame.payload = data;
> +    frame.payloadLength = dataLength;

It's a bit risky to initialize a struct this way (consider the situation where you have added a new member but forget to fix here).

I think it's okay for structs to have a constructor. There are many examples in WebKit code base.

> Source/WebCore/websockets/WebSocketFrame.h:46
> +    static const OpCode OpCodeContinuation = 0x0;
> +    static const OpCode OpCodeText = 0x1;
> +    static const OpCode OpCodeBinary = 0x2;
> +    static const OpCode OpCodeClose = 0x8;
> +    static const OpCode OpCodePing = 0x9;
> +    static const OpCode OpCodePong = 0xA;

You probably need to have defitions of these variables in .cpp file.

(According to my vague memory, Visual Studio didn't like to have a constant definition in class declaration, but I may be wrong. Try EWS if you are curious.)

[Kenichi Ishibashi]

Thank you for comments. I'll address your comment next week.

(In reply to comment #3)
> (From update of attachment 127133 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127133&action=review
> 
> I think your patch is in the right direction.
> 
> Re struct vs class, I guess current WebSocketFrame is simple enough to leave it as a struct.
> 
> > Source/WebCore/websockets/WebSocketChannel.cpp:606
> > +    m_currentFrameEnd = p + maskingKeyLength + payloadLength;
> 
> Ending position of a frame has nothing to do with the channel's state, so it sounds strange to have it as a member variable.
> 
> Rather, I prefer to have it as an output parameter of parseFrame() function:
> 
>     parseFrame(WebSocketFrame& frame, const char*& frameEnd) { ... }
> 
> > Source/WebCore/websockets/WebSocketChannel.cpp:978
> > +    frame.opCode = opCode;
> > +    frame.final = true;
> > +    frame.masked = true;
> > +    frame.payload = data;
> > +    frame.payloadLength = dataLength;
> 
> It's a bit risky to initialize a struct this way (consider the situation where you have added a new member but forget to fix here).
> 
> I think it's okay for structs to have a constructor. There are many examples in WebKit code base.
> 
> > Source/WebCore/websockets/WebSocketFrame.h:46
> > +    static const OpCode OpCodeContinuation = 0x0;
> > +    static const OpCode OpCodeText = 0x1;
> > +    static const OpCode OpCodeBinary = 0x2;
> > +    static const OpCode OpCodeClose = 0x8;
> > +    static const OpCode OpCodePing = 0x9;
> > +    static const OpCode OpCodePong = 0xA;
> 
> You probably need to have defitions of these variables in .cpp file.
> 
> (According to my vague memory, Visual Studio didn't like to have a constant definition in class declaration, but I may be wrong. Try EWS if you are curious.)

[Kenichi Ishibashi]

Created attachment 127749 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 127133 [details]
WIP patch

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

>>> Source/WebCore/websockets/WebSocketChannel.cpp:606
>>> +    m_currentFrameEnd = p + maskingKeyLength + payloadLength;
>> 
>> Ending position of a frame has nothing to do with the channel's state, so it sounds strange to have it as a member variable.
>> 
>> Rather, I prefer to have it as an output parameter of parseFrame() function:
>> 
>>     parseFrame(WebSocketFrame& frame, const char*& frameEnd) { ... }
> 
> 

Done.

>> Source/WebCore/websockets/WebSocketChannel.cpp:978
>> +    frame.payloadLength = dataLength;
> 
> It's a bit risky to initialize a struct this way (consider the situation where you have added a new member but forget to fix here).
> 
> I think it's okay for structs to have a constructor. There are many examples in WebKit code base.

Done.

>> Source/WebCore/websockets/WebSocketFrame.h:46
>> +    static const OpCode OpCodePong = 0xA;
> 
> You probably need to have defitions of these variables in .cpp file.
> 
> (According to my vague memory, Visual Studio didn't like to have a constant definition in class declaration, but I may be wrong. Try EWS if you are curious.)

It seems that I need to use enum hack here because g++ complained like below when I defined these variables in .cpp file:

WebKit/Source/WebCore/websockets/WebSocketChannel.cpp:654: error: 'WebCore::WebSocketFrame::OpCodeContinuation' cannot appear in a constant-expression

[Kenichi Ishibashi]

Hi Kent-san, Alexey,

Could you review the patch?

[Kent Tamura]

Comment on attachment 127749 [details]
Patch

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

> Source/WebCore/websockets/WebSocketChannel.cpp:975
> +    // Mask the frame.

The comment is not so helpful.
How about making a function to make a frame data if you'd like to improve the readability of the code?

> Source/WebCore/websockets/WebSocketChannel.cpp:1005
> -    // Mask the frame.
> -    size_t maskingKeyStart = frame.size();
> -    frame.grow(frame.size() + maskingKeyWidthInBytes); // Add placeholder for masking key. Will be overwritten.
> -    size_t payloadStart = frame.size();
> -    frame.append(data, dataLength);
> +    size_t maskingKeyStart = frameData.size();
> +    frameData.grow(frameData.size() + maskingKeyWidthInBytes); // Add placeholder for masking key. Will be overwritten.
> +    size_t payloadStart = frameData.size();
> +    frameData.append(frame.payload, frame.payloadLength);
>  
> -    cryptographicallyRandomValues(frame.data() + maskingKeyStart, maskingKeyWidthInBytes);
> -    for (size_t i = 0; i < dataLength; ++i)
> -        frame[payloadStart + i] ^= frame[maskingKeyStart + i % maskingKeyWidthInBytes];
> +    cryptographicallyRandomValues(frameData.data() + maskingKeyStart, maskingKeyWidthInBytes);
> +    for (size_t i = 0; i < frame.payloadLength; ++i)
> +        frameData[payloadStart + i] ^= frameData[maskingKeyStart + i % maskingKeyWidthInBytes];

Also, making a function to mask the frame data would improve readability.

[Kenichi Ishibashi]

Created attachment 127800 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 127749 [details]
Patch

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

Kent-san,

Thank you for review. I added two functions. I have though about making WebSocketFrame to be a class and put these function as its member functions, but I just added them as static functions for now.

>> Source/WebCore/websockets/WebSocketChannel.cpp:975
>> +    // Mask the frame.
> 
> The comment is not so helpful.
> How about making a function to make a frame data if you'd like to improve the readability of the code?

I put the comment on wrong place.. Anyway, added makeFrameData() function.

>> Source/WebCore/websockets/WebSocketChannel.cpp:1005
>> +        frameData[payloadStart + i] ^= frameData[maskingKeyStart + i % maskingKeyWidthInBytes];
> 
> Also, making a function to mask the frame data would improve readability.

Added a function as appendMaskedFramePayload().

[WebKit Review Bot]

Comment on attachment 127800 [details]
Patch

Clearing flags on attachment: 127800

Committed r108240: <http://trac.webkit.org/changeset/108240>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.