Last Comment Bug 718557 - Clean up DOM Websocket close logic.
: Clean up DOM Websocket close logic.
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla13
Assigned To: Jason Duell [:jduell] (needinfo me)
: Patrick McManus [:mcmanus]
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Description User image Jason Duell [:jduell] (needinfo me) 2012-01-16 21:13:17 PST
Created attachment 589100 [details] [diff] [review]

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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2012-01-25 04:03:09 PST
Comment on attachment 589100 [details] [diff] [review]

>+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 User image Jason Duell [:jduell] (needinfo me) 2012-01-27 13:49:50 PST
Created attachment 592259 [details] [diff] [review]

all fixes made.
Comment 3 User image 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 User image Jason Duell [:jduell] (needinfo me) 2012-01-31 20:43:34 PST
Comment 5 User image Ed Morley [:emorley] 2012-02-02 08:16:00 PST

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