Last Comment Bug 674716 - Update WebSocket API to latest draft - close codes and reasons
: Update WebSocket API to latest draft - close codes and reasons
Status: RESOLVED FIXED
[inbound]
: dev-doc-complete
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Patrick McManus [:mcmanus]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks: 666349
  Show dependency treegraph
 
Reported: 2011-07-27 14:44 PDT by Patrick McManus [:mcmanus]
Modified: 2011-10-17 13:04 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (44.65 KB, patch)
2011-07-27 14:48 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
patch v2 (44.64 KB, patch)
2011-07-27 20:05 PDT, Patrick McManus [:mcmanus]
jonas: review+
cbiesinger: review+
Details | Diff | Splinter Review
patch 3 (44.82 KB, patch)
2011-08-03 08:08 PDT, Patrick McManus [:mcmanus]
mcmanus: review+
Details | Diff | Splinter Review
v4 (45.53 KB, patch)
2011-08-03 08:27 PDT, Patrick McManus [:mcmanus]
mcmanus: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2011-07-27 14:44:12 PDT
our closeEvent is missing the code and reason attributes, additionally the close method (for sending closes) is missing the same optional stuff.

The protocol defines a number of standard reason codes.
Comment 1 Patrick McManus [:mcmanus] 2011-07-27 14:48:12 PDT
Created attachment 548926 [details] [diff] [review]
patch v1

includes tests.. :sicking r? for dom, I'll add a r? biesi for the network side
Comment 2 Patrick McManus [:mcmanus] 2011-07-27 20:05:38 PDT
Created attachment 549008 [details] [diff] [review]
patch v2

try server showed me that v1 didn't build on windows due to an extra const. That's the only change.
Comment 3 Christian :Biesinger (don't email me, ping me on IRC) 2011-07-29 13:17:23 PDT
Comment on attachment 549008 [details] [diff] [review]
patch v2

Review of attachment 549008 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsWebSocket.cpp
@@ +835,5 @@
>  
>    rv = conn->Init(this);
>    mConnection = conn;
>    if (NS_FAILED(rv)) {
> +    Close(0, NS_LITERAL_STRING(""), 0);

EmptyString()

::: netwerk/protocol/websocket/WebSocketChannelParent.cpp
@@ +87,5 @@
>  
>    rv = mChannel->SetNotificationCallbacks(this);
>    if (NS_FAILED(rv)) {
> +    mChannel->Close(nsIWebSocketChannel::CLOSE_ABNORMAL,
> +                    NS_LITERAL_CSTRING(""));

EmptyString() here and on the other lines
Comment 4 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-08-02 20:09:42 PDT
Comment on attachment 549008 [details] [diff] [review]
patch v2

Review of attachment 549008 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed.

::: content/base/src/nsWebSocket.cpp
@@ +1306,5 @@
> +    if (NS_IS_HIGH_SURROGATE(aData[i])) {
> +      if (i + 1 == length || !NS_IS_LOW_SURROGATE(aData[i + 1])) {
> +        return PR_TRUE;
> +      }
> +      ++i;

Nit: Slightly faster as:

++i;
if (i == length || !NS_IS_LOW_SURROGATE(aData[i])) {
  ...

@@ +1352,5 @@
> +  }
> +
> +  if (argc >= 2) {
> +    if (ContainsUnpairedSurrogates(reason))
> +      return NS_ERROR_DOM_SYNTAX_ERR;

Hmm.. If we bail here end up setting mClientReasonCode and possibly reusing it on a later call to close() which seems wrong.

@@ +1358,5 @@
> +    CopyUTF16toUTF8(reason, mClientReason);
> +
> +    // The API requires the UTF-8 string to be 123 or less bytes
> +    if (mClientReason.Length() > 123)
> +      return NS_ERROR_DOM_SYNTAX_ERR;

Same here but with both mClientReason and mClientReasonCode set. We should probably set any of them if all checks pass.

Also please add a test that catches this.
Comment 5 Patrick McManus [:mcmanus] 2011-08-03 08:08:45 PDT
Created attachment 550393 [details] [diff] [review]
patch 3

review comments incorporated .. seeking sr due to API change
Comment 6 Patrick McManus [:mcmanus] 2011-08-03 08:14:16 PDT
Comment on attachment 550393 [details] [diff] [review]
patch 3

that new test I added at jonas's request isn't quite right upon further reflection.. I'll update and re sr?
Comment 7 Patrick McManus [:mcmanus] 2011-08-03 08:27:35 PDT
Created attachment 550397 [details] [diff] [review]
v4
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2011-08-03 10:18:06 PDT
Comment on attachment 550397 [details] [diff] [review]
v4

sr=me
Comment 9 Marco Bonardo [::mak] 2011-08-04 03:11:44 PDT
http://hg.mozilla.org/mozilla-central/rev/e054dec99f3e

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