674716 - Update WebSocket API to latest draft - close codes and reasons

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Patrick McManus [:mcmanus]]

our closeEvent is missing the code and reason attributes, additionally the close method (for sending closes) is missing the same optional stuff.

The protocol defines a number of standard reason codes.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch v1

includes tests.. :sicking r? for dom, I'll add a r? biesi for the network side

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: patch v2

try server showed me that v1 didn't build on windows due to an extra const. That's the only change.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 549008 [details] [diff] [review]
patch v2

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

::: content/base/src/nsWebSocket.cpp
@@ +835,5 @@
>  
>    rv = conn->Init(this);
>    mConnection = conn;
>    if (NS_FAILED(rv)) {
> +    Close(0, NS_LITERAL_STRING(""), 0);

EmptyString()

::: netwerk/protocol/websocket/WebSocketChannelParent.cpp
@@ +87,5 @@
>  
>    rv = mChannel->SetNotificationCallbacks(this);
>    if (NS_FAILED(rv)) {
> +    mChannel->Close(nsIWebSocketChannel::CLOSE_ABNORMAL,
> +                    NS_LITERAL_CSTRING(""));

EmptyString() here and on the other lines

Comment 4

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

Comment on attachment 549008 [details] [diff] [review]
patch v2

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

r=me with that fixed.

::: content/base/src/nsWebSocket.cpp
@@ +1306,5 @@
> +    if (NS_IS_HIGH_SURROGATE(aData[i])) {
> +      if (i + 1 == length || !NS_IS_LOW_SURROGATE(aData[i + 1])) {
> +        return PR_TRUE;
> +      }
> +      ++i;

Nit: Slightly faster as:

++i;
if (i == length || !NS_IS_LOW_SURROGATE(aData[i])) {
  ...

@@ +1352,5 @@
> +  }
> +
> +  if (argc >= 2) {
> +    if (ContainsUnpairedSurrogates(reason))
> +      return NS_ERROR_DOM_SYNTAX_ERR;

Hmm.. If we bail here end up setting mClientReasonCode and possibly reusing it on a later call to close() which seems wrong.

@@ +1358,5 @@
> +    CopyUTF16toUTF8(reason, mClientReason);
> +
> +    // The API requires the UTF-8 string to be 123 or less bytes
> +    if (mClientReason.Length() > 123)
> +      return NS_ERROR_DOM_SYNTAX_ERR;

Same here but with both mClientReason and mClientReasonCode set. We should probably set any of them if all checks pass.

Also please add a test that catches this.

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: patch 3

review comments incorporated .. seeking sr due to API change

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 550393 [details] [diff] [review]
patch 3

that new test I added at jonas's request isn't quite right upon further reflection.. I'll update and re sr?

Comment 7

[Patrick McManus [:mcmanus]]

Attachment: v4

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 550397 [details] [diff] [review]
v4

sr=me

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/e054dec99f3e

Comment 10

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/WebSockets/WebSockets_reference/CloseEvent
https://developer.mozilla.org/en/WebSockets/WebSockets_reference/WebSocket

New pages added; these were not previously documented.

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIWebSocketChannelhttps://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIWebSocketListener

Firefox 8 for developers has been updated as well.