Clean up websocket close codes and abort logic

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: jduell, Assigned: jduell)

Tracking

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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

Comment 1

5 years ago
Turning this into a grab-bag about a few minor close_close/abort related issues.
Summary: Pass GOING_AWAY (1002) to server if websocket fails for internal reasons → Clean up websocket close codes and abort logic
(Assignee)

Comment 2

5 years ago
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.
Attachment #613459 - Flags: review?(mcmanus)
(Assignee)

Comment 3

5 years ago
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.)
Attachment #613460 - Flags: review?(mcmanus)
(Assignee)

Comment 4

5 years ago
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.
Attachment #613461 - Flags: review?(mcmanus)
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?
Attachment #613459 - Flags: review?(mcmanus)
Attachment #613460 - Flags: review?(mcmanus) → review+
Comment on attachment 613461 [details] [diff] [review]
Fix some close code cases.

definitely better. thanks.
Attachment #613461 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 7

5 years ago
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.
Attachment #613459 - Attachment is obsolete: true
Attachment #614138 - Flags: review?(mcmanus)
Attachment #614138 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f585e56c03ed
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ffcf18c1253

(hg link in previous comment is wrong: managed to land empty changeset by accident :)
https://hg.mozilla.org/mozilla-central/rev/f585e56c03ed
https://hg.mozilla.org/mozilla-central/rev/2ffcf18c1253
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
You need to log in before you can comment on or make changes to this bug.