The default bug view has changed. See this FAQ.

Update WebSocket API to latest draft - close codes and reasons

RESOLVED FIXED in mozilla8

Status

()

Core
Networking: WebSockets
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

({dev-doc-complete})

Trunk
mozilla8
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 666349
No longer depends on: 666349, 674527
(Assignee)

Comment 1

6 years ago
Created attachment 548926 [details] [diff] [review]
patch v1

includes tests.. :sicking r? for dom, I'll add a r? biesi for the network side
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #548926 - Flags: review?(jonas)
(Assignee)

Updated

6 years ago
Attachment #548926 - Flags: review?(cbiesinger)
(Assignee)

Comment 2

6 years ago
Created attachment 549008 [details] [diff] [review]
patch v2

try server showed me that v1 didn't build on windows due to an extra const. That's the only change.
Attachment #548926 - Attachment is obsolete: true
Attachment #548926 - Flags: review?(jonas)
Attachment #548926 - Flags: review?(cbiesinger)
Attachment #549008 - Flags: review?(jonas)
(Assignee)

Updated

6 years ago
Attachment #549008 - Flags: review?(cbiesinger)
(Assignee)

Updated

6 years ago
Blocks: 674890
(Assignee)

Updated

6 years ago
No longer blocks: 674890
(Assignee)

Updated

6 years ago
Blocks: 674905
(Assignee)

Updated

6 years ago
No longer blocks: 674905
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
Attachment #549008 - Flags: review?(cbiesinger) → review+
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.
Attachment #549008 - Flags: review?(jonas) → review+
(Assignee)

Comment 5

6 years ago
Created attachment 550393 [details] [diff] [review]
patch 3

review comments incorporated .. seeking sr due to API change
Attachment #549008 - Attachment is obsolete: true
Attachment #550393 - Flags: superreview?
Attachment #550393 - Flags: review+
(Assignee)

Comment 6

6 years ago
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?
Attachment #550393 - Flags: superreview?
(Assignee)

Comment 7

6 years ago
Created attachment 550397 [details] [diff] [review]
v4
Attachment #550393 - Attachment is obsolete: true
Attachment #550397 - Flags: superreview?(bzbarsky)
Attachment #550397 - Flags: review+
Comment on attachment 550397 [details] [diff] [review]
v4

sr=me
Attachment #550397 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/e054dec99f3e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.