[Webkit-unassigned] [Bug 87336] [WebSocket] WebSocket object should fire error event when WebSocket server can't be connected.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=87336





--- Comment #4 from Li Yin <li.yin at intel.com>  2012-05-24 05:44:11 PST ---
(In reply to comment #3)
> (From update of attachment 143762 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=143762&action=review
> 
> I recommend adding another test that calls ws.close() in onclose handler to make sure the browser won't crash in these scenarios.
> 
Okay, I have done some tests which covers calling ws.close() in onclose handler and calling ws.close in onerror handler,
both of them willn't crash, because I check the m_failed value.
And I will add these tests in the next patch.

> > Source/WebCore/ChangeLog:17
> > +        From http://tools.ietf.org/html/rfc6455#section-4.1
> > +        If the connection could not be opened, either because a direct
> > +        connection failed or because any proxy used returned an error,
> > +        then the client MUST _Fail the WebSocket Connection_ and abort
> > +        the connection attempt.
> > +
> > +        From 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.
> 
> I'm not quite sure if these spec requirements have anything to do with the change itself. Could you add more explanations in your words?
If the WebSocket Server can't be connected and the underlying transport layer is unexpectedly lost,
I think they should trigger the fail the WebSocket connection algorithm, fire the error event, not only send close event.
In these scenarios, Firefox will fire error event at first, and then fire the close event.

> 
> > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:232
> > +            RefPtr<WebSocketChannel> protect(this); // The client can close the channel, potentially removing the last reference.
> 
> This use of "protect" is wrong, because the channel may get deleted in the end of the block and the code after the block may access to "this" which is already deleted.
> 
> If the end of a block containing a protector is reached, the function must immediately exit.
> 
> > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:308
> > +                RefPtr<WebSocketChannel> protect(this); // The client can close the channel, potentially removing the last reference.
> 
> This protector is not necessary because the channel is guaranteed to exist until at least the end of this function (see deref() in the end of this function).
> 
> The deref() in the end of this function is paired with ref() in connect(), and it is guaranteed that the channel is alive between these two.

Thanks for your good reminding, I will have a double check.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.