Bug 86958 - [WebSocket] WebSocket object should fire a simple event named error when it is required to fail the websocket connection.
Summary: [WebSocket] WebSocket object should fire a simple event named error when it i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-20 01:10 PDT by Li Yin
Modified: 2012-05-22 03:51 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.97 KB, patch)
2012-05-20 06:56 PDT, Li Yin
no flags Details | Formatted Diff | Diff
Patch (5.97 KB, patch)
2012-05-22 00:21 PDT, Li Yin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Li Yin 2012-05-20 01:10:04 PDT
Spec:http://dev.w3.org/html5/websockets/#feedback-from-the-protocol
If the user agent was required to fail the websocket connection or the WebSocket connection is closed with prejudice, fire a simple event named error at the WebSocket object.
Comment 1 Li Yin 2012-05-20 06:56:05 PDT
Created attachment 142903 [details]
Patch
Comment 2 Takashi Toyoshima 2012-05-21 07:28:44 PDT
Hi Li,
Thank you for sending a patch.
Actually, this is a bug and should be fixed.

A filed bug in chromium side issue tracker is here.
http://code.google.com/p/chromium/issues/detail?id=128057

I think original code just missed to call WebSocketChannelClient::didReceiveMessageError() in WebSocketChannel::fail().

Yuta,
I think you write this part. Could you check this?
Comment 3 Yuta Kitamura 2012-05-21 22:37:00 PDT
Seems like the spec changed at http://html5.org/tools/web-apps-tracker?from=6376&to=6377
(see also https://www.w3.org/Bugs/Public/show_bug.cgi?id=13294)

As Takashi said, I think you just need to call didReceiveMessageError() in fail(). You don't have to add a new function to WebSocketChannelClient.

You probably need to have a line like "RefPtr<WebSocketChannel> protect;" before calling didReceiveMessageError(), otherwise this function may cause the channel to get removed (because users may do "ws.close()" in onerror handler).

Also, you might need to change the definition of didReceiveMessageError() to let it fire the event when the state is not OPEN or CLOSING. This is old code based on the old API spec, and I guess this restriction is leftover from the old age.
Comment 4 Yuta Kitamura 2012-05-21 22:44:22 PDT
Comment on attachment 142903 [details]
Patch

r- because adding a new callback fireErrorEvent() is clearly unacceptable. fireErrorEvent() doesn't work properly in Web Workers, as the definition for workers isn't provided.
Comment 5 Li Yin 2012-05-22 00:19:25 PDT
(In reply to comment #3)
> Seems like the spec changed at http://html5.org/tools/web-apps-tracker?from=6376&to=6377
> (see also https://www.w3.org/Bugs/Public/show_bug.cgi?id=13294)
> 
> As Takashi said, I think you just need to call didReceiveMessageError() in fail(). You don't have to add a new function to WebSocketChannelClient.
> 
> You probably need to have a line like "RefPtr<WebSocketChannel> protect;" before calling didReceiveMessageError(), otherwise this function may cause the channel to get removed (because users may do "ws.close()" in onerror handler).
> 
> Also, you might need to change the definition of didReceiveMessageError() to let it fire the event when the state is not OPEN or CLOSING. This is old code based on the old API spec, and I guess this restriction is leftover from the old age.

Thanks for your review and good solution, I have updated the patch, please review it again.
Comment 6 Li Yin 2012-05-22 00:21:23 PDT
Created attachment 143210 [details]
Patch
Comment 7 Yuta Kitamura 2012-05-22 00:41:45 PDT
Comment on attachment 143210 [details]
Patch

Looks fine. Please wait for comments from a reviewer.
Comment 8 WebKit Review Bot 2012-05-22 03:51:26 PDT
Comment on attachment 143210 [details]
Patch

Clearing flags on attachment: 143210

Committed r117944: <http://trac.webkit.org/changeset/117944>
Comment 9 WebKit Review Bot 2012-05-22 03:51:30 PDT
All reviewed patches have been landed.  Closing bug.