748580 - websockets: omit close code (rather than code=1000) when no close code passed to close()

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Jason Duell]

Takashi Toyoshima from the Chromium team notes that Chrome is omitting a code in this case.  The spec isn't crystal clear on it AFIACT, but I think omitting is more sensible (code 1005 doesn't really make sense w/o it), so I'm planning to implement it.

Comment 1

[Jason Duell]

See also https://www.w3.org/Bugs/Public/show_bug.cgi?id=16703#c9 and Ian's response.

Comment 2

[Jason Duell]

Attachment: v1. Send empty close frame if no code given.

test will land in a separate bug, because it will depend on the pywebsockets mods I made in bug 752778, and so it may or may not land (and at different time).  I've tested this extensively, FWIW.

Comment 4

[Jason Duell]

Attachment: mochi test

This is just the test from bug 753081 moved here with one minor tweak to the passive close handler (so we don't sent 1005 frames on the wire: a pywebsocket bug at some level--I've alerted them to it).

Comment 5

[Patrick McManus [:mcmanus]]

Comment on attachment 622111 [details] [diff] [review]
v1. Send empty close frame if no code given.

The right thing to do here is ambiguous imo, but consistency across browsers is good and and the new way is just as good as the old way.

Comment 6

[Jason Duell]

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

Comment 7

[Jason Duell]

> The right thing to do here is ambiguous imo

The spec editors thinks it's not ambiguous, FWIW:

  https://www.w3.org/Bugs/Public/show_bug.cgi?id=16703#c10

Comment 8

[Jason Duell]

Comment on attachment 622111 [details] [diff] [review]
v1. Send empty close frame if no code given.

Yet another low-risk Websocket spec compliance bug.  This one I'm only nomming (nom, nom!) because it's not quite as trivial as the others.  But still fairly low-risk IMO.

[Approval Request Comment]
User impact if declined:  we'll reply to servers with "OK" code (1000) when users actually meant to send no code (1005).  In most cases applications won't care.
Testing completed (on m-c, etc.):  lots of testing, including mochitest.
Risk to taking this patch (and alternatives if risky):  very low.
String or UUID changes made by this patch: none

Comment 9

[Jason Duell]

only nomming /for beta/...

Comment 10

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jason Duell (:jduell) from comment #6)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b65953b2b195

https://hg.mozilla.org/mozilla-central/rev/b65953b2b195

Comment 11

[Alex Keybl [:akeybl]]

Comment on attachment 622111 [details] [diff] [review]
v1. Send empty close frame if no code given.

[Triage Comment]
From the nomination, it's unclear what the motivation to fix this on our second to last beta is. WebSockets have been in the product since FF11, and we know of no sites behaving incorrectly because of this issue. Since FF14 appears to be affected, pre-approving for that branch since it still has time to bake this change prior to release.

Comment 12

[Jason Duell]

This relies on pywebsocket changes, else aurora mochitest fails.  Requesting aurora in that bug

Comment 13

[Jason Duell]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dc87dfeeeb88