Last Comment Bug 718557 - Clean up DOM Websocket close logic.
: Clean up DOM Websocket close logic.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jason Duell [:jduell] (needinfo? me)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-16 21:13 PST by Jason Duell [:jduell] (needinfo? me)
Modified: 2012-05-30 07:17 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (14.03 KB, patch)
2012-01-16 21:13 PST, Jason Duell [:jduell] (needinfo? me)
bugs: review-
Details | Diff | Review
v2 (14.03 KB, patch)
2012-01-27 13:49 PST, Jason Duell [:jduell] (needinfo? me)
no flags Details | Diff | Review
v2 (for realz) (13.35 KB, patch)
2012-01-27 13:52 PST, Jason Duell [:jduell] (needinfo? me)
bugs: review+
Details | Diff | Review

Description Jason Duell [:jduell] (needinfo? me) 2012-01-16 21:13:17 PST
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.
Comment 1 Olli Pettay [:smaug] 2012-01-25 04:03:09 PST
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.
Comment 2 Jason Duell [:jduell] (needinfo? me) 2012-01-27 13:49:50 PST
Created attachment 592259 [details] [diff] [review]
v2

all fixes made.
Comment 3 Jason Duell [:jduell] (needinfo? me) 2012-01-27 13:52:20 PST
Created attachment 592261 [details] [diff] [review]
v2 (for realz)

forgot to refresh mq patch
Comment 4 Jason Duell [:jduell] (needinfo? me) 2012-01-31 20:43:34 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/77991fb798e5
Comment 5 Ed Morley [:emorley] 2012-02-02 08:16:00 PST
https://hg.mozilla.org/mozilla-central/rev/77991fb798e5

Note You need to log in before you can comment on or make changes to this bug.