1272100 - [FlyWeb] Review WebSocket changes for landing mozilla-central

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Kannan Vijayan [:djvj]]

Attachment: flyweb-websocket.patch

Sub-bug for reviewing WebSocket changes for landing in m-c.

Comment 1

[u408661]

Comment on attachment 8751440 [details] [diff] [review]
flyweb-websocket.patch

I know we also want Michal to review changes to netwerk/websocket stuff

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8751440 [details] [diff] [review]
flyweb-websocket.patch

Review of attachment 8751440 [details] [diff] [review]:
-----------------------------------------------------------------

r+ for the dom/base/WebSocket.* changes.

Comment 3

[Michal Novotny [:michal]]

Comment on attachment 8751440 [details] [diff] [review]
flyweb-websocket.patch

Review of attachment 8751440 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +1580,5 @@
>      if (maskBit) {
> +      if (!mIsServerSide) {
> +        // This is unexpected - the server does not generally send masked
> +        // frames to the client, but it is allowed
> +        LOG(("WebSocketChannel:: Client RECEIVING masked frame."));

This comment is wrong. The spec in section 5.1 clearly says that client to server communication must be masked and server to client must not.

@@ +2025,5 @@
>  
>    mCurrentOutSent = 0;
>    mHdrOut = mOutHeader;
>  
> +  uint8_t maskMask = mIsServerSide ? 0 : kMaskBit;

Wouldn't maskBit be a better name?

@@ +2208,5 @@
>    }
>  
>    // Mask the real message payloads
>  
>    ApplyMask(mask, mCurrentOut->BeginWriting(), mCurrentOut->Length());

ApplyMask should not be called if mask == 0.

@@ +2684,5 @@
> +}
> +
> +void
> +ProcessServerWebSocketExtensions(const nsACString& aExtensions,
> +                                 nsACString& aNegotatedExtensions)

negotated -> negotiated

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> ::: netwerk/protocol/websocket/WebSocketChannel.cpp
> @@ +1580,5 @@
> >      if (maskBit) {
> > +      if (!mIsServerSide) {
> > +        // This is unexpected - the server does not generally send masked
> > +        // frames to the client, but it is allowed
> > +        LOG(("WebSocketChannel:: Client RECEIVING masked frame."));
> 
> This comment is wrong. The spec in section 5.1 clearly says that client to
> server communication must be masked and server to client must not.

Agreed. But this is existing code. I'd rather not change the behavior of client-side websockets in this bug since it might affect existing websites.

However we should probably do what the added comment says and abort the connection if maskBit is 0 and mIsServerSide?

Comment 5

[Michal Novotny [:michal]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> Agreed. But this is existing code. I'd rather not change the behavior of
> client-side websockets in this bug since it might affect existing websites.
> 
> However we should probably do what the added comment says and abort the
> connection if maskBit is 0 and mIsServerSide?

This sounds good to me.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2

Missed adding the new file.

This patch fixes the review comments. No other changes.

Comment 8

[Michal Novotny [:michal]]

Comment on attachment 8758629 [details] [diff] [review]
Patch v2

Review of attachment 8758629 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +1581,5 @@
> +           "from client\n"));
> +      return NS_ERROR_ILLEGAL_VALUE;
> +    }
> +
> +    // XXX Should we close connection if maskBit is 0 and mIsServerSide is true?

Remove this comment.

@@ +1598,4 @@
>        ApplyMask(mask, payload, payloadLength);
> +    } else if (mIsServerSide) {
> +      LOG(("WebSocketChannel::ProcessInput: unmasked frame received "
> +           "from client\n"));

Please make the logged text different from the above. E.g. this could be "masked frame with mask 0 received from client".

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53a8889c2af3
Websocket changes in preparation of FlyWeb landing. Make websocket code support acting as the server side of a websocket connection. r=michal

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/53a8889c2af3