Closed Bug 1272100 Opened 3 years ago Closed 3 years ago

[FlyWeb] Review WebSocket changes for landing mozilla-central

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: djvj, Assigned: sicking)

References

Details

(Whiteboard: [necko-would-take])

Attachments

(1 file, 2 obsolete files)

Attached patch flyweb-websocket.patch (obsolete) — Splinter Review
Sub-bug for reviewing WebSocket changes for landing in m-c.
Attachment #8751440 - Flags: feedback?(amarchesini)
Comment on attachment 8751440 [details] [diff] [review]
flyweb-websocket.patch

I know we also want Michal to review changes to netwerk/websocket stuff
Attachment #8751440 - Flags: review?(michal.novotny)
Whiteboard: [necko-would-take]
Comment on attachment 8751440 [details] [diff] [review]
flyweb-websocket.patch

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

r+ for the dom/base/WebSocket.* changes.
Attachment #8751440 - Flags: feedback?(amarchesini) → feedback+
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
Attachment #8751440 - Flags: review?(michal.novotny) → feedback+
> ::: 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?
(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.
Attached patch Patch v2 (obsolete) — Splinter Review
Assignee: nobody → jonas
Attachment #8751440 - Attachment is obsolete: true
Attachment #8758622 - Flags: review?(michal.novotny)
Attached patch Patch v2Splinter Review
Missed adding the new file.

This patch fixes the review comments. No other changes.
Attachment #8758622 - Attachment is obsolete: true
Attachment #8758622 - Flags: review?(michal.novotny)
Attachment #8758629 - Flags: review?(michal.novotny)
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".
Attachment #8758629 - Flags: review?(michal.novotny) → review+
Pushed by sicking@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/53a8889c2af3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.