Last Comment Bug 663871 - Websockets Protocol IETF 08
: Websockets Protocol IETF 08
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla7
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on:
Blocks: 783133
  Show dependency treegraph
 
Reported: 2011-06-13 10:12 PDT by Patrick McManus [:mcmanus]
Modified: 2013-12-27 14:20 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
utf8 replacement char v1 (7.92 KB, patch)
2011-06-13 10:16 PDT, Patrick McManus [:mcmanus]
benjamin: review-
Details | Diff | Review
unknown opcode v1 (1.13 KB, patch)
2011-06-13 10:18 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Review
rsvd bits v1 (2.19 KB, patch)
2011-06-13 10:20 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Review
close codes v1 (6.32 KB, patch)
2011-06-13 10:22 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Review
delay tcp close v1 (13.54 KB, patch)
2011-06-13 10:45 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Review
update version number v1 (3.04 KB, patch)
2011-06-13 10:47 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Review
utf8 replacement char v2 (17.20 KB, patch)
2011-06-14 14:42 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Review
enforce utf8 recvrs 1 (4.13 KB, patch)
2011-06-16 11:06 PDT, Patrick McManus [:mcmanus]
cbiesinger: review+
Details | Diff | Review

Description Patrick McManus [:mcmanus] 2011-06-13 10:12:35 PDT
As expected, last week IETF published a websockets protocol -08 with minor changes to -07. IMO this is the one we want to ship.

There are 5 changes (plus the version number bump). In general these are just mandated behaviors for previously undefined errors, a couple new codes for closes conditions (which will be consumed by the eventual API changes if the editor's draft is any indication), and a SHOULD level change to TCP termination.
Comment 1 Patrick McManus [:mcmanus] 2011-06-13 10:16:36 PDT
Created attachment 538942 [details] [diff] [review]
utf8 replacement char v1

websockets -08 sec 8.1 requires UTF8 REPLACEMENT_CHAR substitution invalid portions of received strings that are supposed to be UTF-8 already. I couldn't find code that did this all in UTF-8, so I modeled this on the existing IsUTF8().

Benjamin, I'm taking a guess at a reviewer - feel free to reroute if that's appropos.
Comment 2 Patrick McManus [:mcmanus] 2011-06-13 10:18:38 PDT
Created attachment 538945 [details] [diff] [review]
unknown opcode v1

websockets -08 sec 4.2 now requires frames with unknown opcodes to be ignored .. we were previously throwing errors.
Comment 3 Patrick McManus [:mcmanus] 2011-06-13 10:20:49 PDT
Created attachment 538946 [details] [diff] [review]
rsvd bits v1

websockets -08 sec 4.2 requires frames using rsvd bits to be ignored, previous we were throwing errors.
Comment 4 Patrick McManus [:mcmanus] 2011-06-13 10:22:14 PDT
Created attachment 538947 [details] [diff] [review]
close codes v1

websockets -08 sec 7.4.1 defines two new close codes
Comment 5 Benjamin Smedberg [:bsmedberg] 2011-06-13 10:38:13 PDT
Comment on attachment 538942 [details] [diff] [review]
utf8 replacement char v1

No tests, no review ;-) Obviously a necko peer will need to review the websocket bits.
Comment 6 Patrick McManus [:mcmanus] 2011-06-13 10:43:56 PDT
(In reply to comment #5)
> Comment on attachment 538942 [details] [diff] [review] [review]
> utf8 replacement char v1
> 
> No tests, no review ;-)

you don't need to r-anything it yet, but could you comment on whether or not placing such a function in that spot is conceptually acceptable?
Comment 7 Patrick McManus [:mcmanus] 2011-06-13 10:45:27 PDT
Created attachment 538952 [details] [diff] [review]
delay tcp close v1

This is the most annoying of the new requirements:

"The underlying TCP connection, in most normal cases, SHOULD be closed
first by the server, so that it holds the TIME_WAIT state and not the
client"

It is specified this way because the TIME_WAIT does not prevent the client from reusing recycled port numbers within 2 MSL. So if we are racing through port numbers and servers are sending the FIN quickly, this helps scalability.

OTOH, if we are racing through sessions but servers are slow in sending their FINs, then this means we are keeping the websockethandler state around for a long time (and a lot of them).

This patch limits the wait to a max of 1 full second which accomodates network delay. Of course if the FIN is recvd before that cleanup is done immediately. The delay does not impact the time at which the JS application receives the onclose() event. Furthermore, if we have more than 50 websocket handlers concurrently active we don't honor the SHOULD at all in order to focus on resource reduction immediately.
Comment 8 Patrick McManus [:mcmanus] 2011-06-13 10:47:45 PDT
Created attachment 538953 [details] [diff] [review]
update version number v1

update our version string and the version string in the server to -08.
Comment 9 Christian :Biesinger (don't email me, ping me on IRC) 2011-06-13 13:08:36 PDT
Comment on attachment 538953 [details] [diff] [review]
update version number v1

your server changes should also get an entry in the README
Comment 10 Patrick McManus [:mcmanus] 2011-06-14 14:42:19 PDT
Created attachment 539329 [details] [diff] [review]
utf8 replacement char v2

updated patch now with horrifically ugly tests due to internal symbol limitation in tests.. but tests nonetheless.
Comment 11 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-06-14 17:46:51 PDT
As was mentioned in the security review, the silent discarding of unrecognized data (unknown opcode v1, rsvd bits v1, utf8 replacement char v2) seems totally bogus. I think we should speak up about getting these changes undone in the spec, and avoid shipping these changes. We agreed in the secteam meeting that it is OK to ship a stricter-than-than-spec implementation (i.e. without those three patches) of v8 in FF6, assuming no other blocking issues.
Comment 12 Patrick McManus [:mcmanus] 2011-06-16 08:37:45 PDT
I agree - we should fail on protocol violations of opcode, rsvd bits and utf-8.
Comment 13 Patrick McManus [:mcmanus] 2011-06-16 08:40:01 PDT
Comment on attachment 539329 [details] [diff] [review]
utf8 replacement char v2

For now at least we will fail the bad recvd utf-8 case rather than silently rewriting it.
Comment 14 Patrick McManus [:mcmanus] 2011-06-16 11:06:50 PDT
Created attachment 539832 [details] [diff] [review]
enforce utf8 recvrs 1

sec-review decided that the right way to handle receipt of a non-conformant utf-8 message is to fail the connection.
Comment 15 Christian :Biesinger (don't email me, ping me on IRC) 2011-06-21 06:32:51 PDT
Comment on attachment 539832 [details] [diff] [review]
enforce utf8 recvrs 1

seems like we really should get the spec changed instead of violating it, but r=biesi
Comment 16 Patrick McManus [:mcmanus] 2011-06-21 06:35:04 PDT
(In reply to comment #15)
> Comment on attachment 539832 [details] [diff] [review] [review]
> enforce utf8 recvrs 1
> 
> seems like we really should get the spec changed instead of violating it,

we've sent comments - it might be adjusted going forward.
Comment 17 :Ehsan Akhgari (busy, don't ask for review please) 2011-06-21 15:05:35 PDT
http://hg.mozilla.org/mozilla-central/rev/680da2e79681
Comment 19 Asa Dotzler [:asa] 2011-06-23 14:31:43 PDT
not going to track this and I think it's probably too late for 6 but we can have that discussion if someone nominates the patch.
Comment 20 Patrick McManus [:mcmanus] 2011-06-23 14:34:42 PDT
(In reply to comment #19)
> not going to track this and I think it's probably too late for 6 but we can
> have that discussion if someone nominates the patch.

given that the JS is prefixed, I'm ok leaving it out of FF6 and having a different protocol version in FF7.
Comment 21 Eric Shepherd [:sheppy] 2011-08-05 12:42:39 PDT
The protocol details aren't documented yet (we're not going to document the protocol itself until things settle down more, maybe once we support IETF-09 or later). However, the main landing page now lists our support for IETF-08 in Firefox 7:

https://developer.mozilla.org/en/WebSockets

This is also mentioned on Firefox 7 for developers.
Comment 22 Patrick McManus [:mcmanus] 2011-08-05 12:59:22 PDT
(In reply to Eric Shepherd [:sheppy] from comment #21)
> The protocol details aren't documented yet (we're not going to document the
> protocol itself until things settle down more, maybe once we support IETF-09
> or later). However, the main landing page now lists our support for IETF-08
> in Firefox 7:
> 
> https://developer.mozilla.org/en/WebSockets
> 
> This is also mentioned on Firefox 7 for developers.

Oh man, I'm going to reply with a pedantic semi-correction. I'm sorry in advance.

Firefox 7 (and 8 and hopefully >8 for that matter) support Version 8 of the IETF websocket protocol as defined by ietf draft 10. (which is to say that the protocol version that draft-10 defines is version 8. That is the same protocol version defined by drafts 08 and 09 because the changes in those drafts did not make the wire protocol incompatible.). Most people are calling this 'draft-10 support' and I wouldn't want them to think we didn't support draft-10, because we do! There will surely be a draft-11 to make more changes in standardese but there is a high likelihood it will still work with protocol version 8... but now I'm trying to predict the future in a bugzilla commment - so I'll stop.

Firefox 6 did indeed implement protocol version 7 (which also corresponded to only draft-07.).
Comment 23 Eric Shepherd [:sheppy] 2011-08-05 13:04:49 PDT
Wow, I hate the IETF. :D

I will make the needed corrections. Thanks!
Comment 24 Eric Shepherd [:sheppy] 2011-08-05 13:10:27 PDT
Updated.

Note You need to log in before you can comment on or make changes to this bug.