Clean up DOM Websocket close logic.

RESOLVED FIXED in mozilla13

Status

()

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

People

(Reporter: jduell, Assigned: jduell)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 589100 [details] [diff] [review]
v1

This patch does a bunch of related things:

1) It cleans up nsWebSocket's CloseConnection and FailConnection so that they
   explictly take close code/reason arguments, rather than rely on member
   variables.  It's always clear at call time what error code should be passed,
   so there's no need to use member variables.  Makes code cleaner & safer IMO.

2) Adds a new CLOSE_INTERNAL_ERROR code that we return whenever we close the WS
   connection due to some internal problem (malloc fails, can't get
   charConverter, etc).  The WS spec suprisingly doesn't have any equivalent of
   the HTTP 500 (server internal error) code.  On consultation with Patrick
   McManus we're going to go with 1001 ("endpoint is going away").  Previously
   we were responding with whatever mClientReasonCode was set to (by default it
   was 0 and we sent no close code at all).

3) Changes OnServerClose so that when we receive a server close, we reply with a
   close message that has the same code/reason as the server provide, as the
   spec recommends (previously we were sending whatever mClientReasonCode was
   set to--usually 0, so no close code at all).  

4) Gets rid of m{Client|Server}Reason{Code}, which are no longer needed.
   Centralize naming of mCloseEventXXX variables, and give them names that match
   the spec for code clarity.
Attachment #589100 - Flags: review?(bugs)

Comment 1

6 years ago
Comment on attachment 589100 [details] [diff] [review]
v1

>+nsWebSocket::FailConnection(PRUint16 reasonCode) {
>+    return FailConnection(reasonCode, NS_LITERAL_CSTRING(""));
>+}
>+
>+inline nsresult
>+nsWebSocket::CloseConnection(PRUint16 reasonCode) {
>+    return CloseConnection(reasonCode, NS_LITERAL_CSTRING(""));
>+}


EmptyCString() please. Could you actually have
the methods like
CloseConnection(PRUint16 reasonCode, const nsAString& aReason = EmptyCString())
Not sure if that compiles.
Also, aReasonCode. Parameters start with 'a'

Sorry for the delay.
Attachment #589100 - Flags: review?(bugs) → review-
(Assignee)

Comment 2

6 years ago
Created attachment 592259 [details] [diff] [review]
v2

all fixes made.
Attachment #589100 - Attachment is obsolete: true
Attachment #592259 - Flags: review?(bugs)
(Assignee)

Comment 3

6 years ago
Created attachment 592261 [details] [diff] [review]
v2 (for realz)

forgot to refresh mq patch
Attachment #592259 - Attachment is obsolete: true
Attachment #592259 - Flags: review?(bugs)
Attachment #592261 - Flags: review?(bugs)
(Assignee)

Updated

6 years ago
Attachment #592261 - Attachment description: v2 (for realiz) → v2 (for realz)

Updated

6 years ago
Attachment #592261 - Flags: review?(bugs) → review+
(Assignee)

Comment 4

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/77991fb798e5

Comment 5

6 years ago
https://hg.mozilla.org/mozilla-central/rev/77991fb798e5
Target Milestone: --- → mozilla13
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.