712191 - Clean up websocket close codes and abort logic

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Jason Duell]

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]

Turning this into a grab-bag about a few minor close_close/abort related issues.

Comment 2

[Jason Duell]

Attachment: 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]

Attachment: 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]

Attachment: 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]]

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]]

Comment on attachment 613461 [details] [diff] [review]
Fix some close code cases.

definitely better. thanks.

Comment 7

[Jason Duell]

Attachment: 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]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f585e56c03ed

Comment 9

[Jason Duell]

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

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

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/f585e56c03ed
https://hg.mozilla.org/mozilla-central/rev/2ffcf18c1253