Last Comment Bug 712191 - Clean up websocket close codes and abort logic
: Clean up websocket close codes and abort logic
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Jason Duell [:jduell] (needinfo me)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-19 17:31 PST by Jason Duell [:jduell] (needinfo me)
Modified: 2012-05-30 07:19 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Set mStopped in a few places where we'd ABORT otherwise. (1.99 KB, patch)
2012-04-09 18:09 PDT, Jason Duell [:jduell] (needinfo me)
no flags Details | Diff | Splinter Review
Avoid having AbortSession called twice when ProcessInput called (10.04 KB, patch)
2012-04-09 18:11 PDT, Jason Duell [:jduell] (needinfo me)
mcmanus: review+
Details | Diff | Splinter Review
Fix some close code cases. (7.74 KB, patch)
2012-04-09 18:12 PDT, Jason Duell [:jduell] (needinfo me)
mcmanus: review+
Details | Diff | Splinter Review
Remove requirement to set mStopped before calling StopSession. (2.03 KB, patch)
2012-04-11 13:20 PDT, Jason Duell [:jduell] (needinfo me)
mcmanus: review+
Details | Diff | Splinter Review

Description Jason Duell [:jduell] (needinfo me) 2011-12-19 17:31:52 PST
There are a lot of places where we currently return 1001 (PROTOCOL_ERROR) when we hit an internal error of some sort.  There's no equivalent of the HTTP 5xx series in the WS protocol (IMO an oversight), but Patrick suggests we send GOING_AWAY (1002) in that case.

We also need to make sure that we don't throw exceptions in such cases to the DOM unless the W3C spec specifies we should--I'll audit that at the same time.
Comment 1 Jason Duell [:jduell] (needinfo me) 2012-04-09 18:08:42 PDT
Turning this into a grab-bag about a few minor close_close/abort related issues.
Comment 2 Jason Duell [:jduell] (needinfo me) 2012-04-09 18:09:41 PDT
Created attachment 613459 [details] [diff] [review]
Set mStopped in a few places where we'd ABORT otherwise.

The alternative is to remove the ABORT check for mStopped=1 in StopSession.
Actually, that's probably cleaner.  Let me know what you think.
Comment 3 Jason Duell [:jduell] (needinfo me) 2012-04-09 18:11:29 PDT
Created attachment 613460 [details] [diff] [review]
Avoid having AbortSession called twice when ProcessInput called

OnInputStreamReady was already checking return value and calling AbortSession:
this patch changes other call site to do same, and removes AbortSession calls
from ProcessInput itself.

Code also now returns error from OnDataAvailable() if ProcessInput fails--AFAICT
that's the logical thing to do, though I don't think it makes any practical
difference (if we instead have the loop and/or further OnDataAvail calls
continue and call ProcessInput again, it's set up to check mStopped and drop any
additional content, so same result.  But this is easier to follow.)
Comment 4 Jason Duell [:jduell] (needinfo me) 2012-04-09 18:12:33 PDT
Created attachment 613461 [details] [diff] [review]
Fix some close code cases.

Never send CLOSE_ABNORMAL (1006): spec specifically forbids sending it.  We
already have logic in place to deliver 1006 to client when needed (i.e. when no
Fin sent/received).

Send CLOSE_GOING_AWAY for NS_ERROR_UNEXPECTED.

Make error code explicit instead of using whatever rv might be, where possible.

Use CLOSE_INVALID_PAYLOAD (1007) for bad UTF-8, per spec.
Comment 5 Patrick McManus [:mcmanus] 2012-04-11 08:00:20 PDT
Comment on attachment 613459 [details] [diff] [review]
Set mStopped in a few places where we'd ABORT otherwise.

I think you can just remove the ABORT in StopSession and replace it with (a mostly redundant) mStopped = 1.. I think that's an impt property to be ale to rely on.

sound good?
Comment 6 Patrick McManus [:mcmanus] 2012-04-11 11:43:32 PDT
Comment on attachment 613461 [details] [diff] [review]
Fix some close code cases.

definitely better. thanks.
Comment 7 Jason Duell [:jduell] (needinfo me) 2012-04-11 13:20:03 PDT
Created attachment 614138 [details] [diff] [review]
Remove requirement to set mStopped before calling StopSession.

Didn't remove mStopped = 1 in AbortSession because it seems we want that set right away--no need to delay until we actually send FIN.
Comment 8 Jason Duell [:jduell] (needinfo me) 2012-04-11 15:49:57 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/f585e56c03ed
Comment 9 Jason Duell [:jduell] (needinfo me) 2012-04-11 16:17:25 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ffcf18c1253

(hg link in previous comment is wrong: managed to land empty changeset by accident :)

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