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

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: jduell, Assigned: jduell)

Tracking

unspecified
mozilla14
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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

Comment 1

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

Comment 2

5 years ago
Created attachment 622111 [details] [diff] [review]
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.
Attachment #622111 - Flags: review?(mcmanus)
(Assignee)

Updated

5 years ago
Blocks: 753081
(Assignee)

Updated

5 years ago
Duplicate of this bug: 753081
(Assignee)

Comment 4

5 years ago
Created attachment 623793 [details] [diff] [review]
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).
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+
(Assignee)

Comment 6

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

Comment 7

5 years ago
> 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
(Assignee)

Comment 8

5 years ago
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?
(Assignee)

Comment 9

5 years ago
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
Last Resolved: 5 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+
(Assignee)

Comment 12

5 years ago
This relies on pywebsocket changes, else aurora mochitest fails.  Requesting aurora in that bug
Depends on: 752776
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/dc87dfeeeb88
(Assignee)

Updated

5 years ago
Target Milestone: mozilla15 → mozilla14
You need to log in before you can comment on or make changes to this bug.