Closed Bug 748580 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Networking: WebSockets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)

References

Details

Attachments

(2 files)

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.
See also https://www.w3.org/Bugs/Public/show_bug.cgi?id=16703#c9 and Ian's response.
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.
Attachment #622111 - Flags: review?(mcmanus)
Blocks: 753081
Duplicate of this bug: 753081
Attached patch mochi testSplinter Review
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).
Attachment #623793 - Flags: review?(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.
Attachment #622111 - Flags: review?(mcmanus) → review+
Attachment #623793 - Flags: review?(mcmanus) → review+
> 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 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
Attachment #622111 - Flags: approval-mozilla-beta?
only nomming /for beta/...
(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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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.
Attachment #622111 - Flags: approval-mozilla-beta?
Attachment #622111 - Flags: approval-mozilla-beta-
Attachment #622111 - Flags: approval-mozilla-aurora+
This relies on pywebsocket changes, else aurora mochitest fails.  Requesting aurora in that bug
Depends on: 752776
Target Milestone: mozilla15 → mozilla14
You need to log in before you can comment on or make changes to this bug.