[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 #19 from Li Yin <li.yin at intel.com>  2012-06-09 19:46:49 PST ---
(From update of attachment 145080)
View in context: https://bugs.webkit.org/attachment.cgi?id=145080&action=review

>>>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:351
>>>> +#if PLATFORM(CHROMIUM)
>>> 
>>> Unfortunately, I have not enough understanding on other ports behaviors.
>>> Why do you need special handling for chromium port?
>> 
>> On chromium, if the network is down, both of OnError and OnClose callback are called.[net/socket_stream.cc] 
>> In webkit side, didFailSocketStream and didCloseSocketStream will be invoked one by one. So didFailSocketStream should not call disconnect function, because didCloseSocketsStream will do that.
>> 
>> But in other platform, like gtk, either didFailSocketStream or didCloseSocketStream is called when the network is down. So it is required to call disconnect function in didFailSocketStream. It is the difference between chromium and gtk, or other platforms.
> 
> Thank you for clarification.
> 
> I think we should not write platform dependent codes in this module as possible as we can.
> So it will be better to handle this difference in SocketStreamHandle implementation.
> 
> How about reseting m_socket and m_handle after calling didFailSocketStream as the original code did in SocketStreamHandleInternal.
> didCloseSocketSrteam is called only when m_socket and m_handle are set.
> (See also the next comment)

Thanks for your good comments.

I agree with you that platform related code should be implemented in SocketStreamHandle class.

I just want to avoid the crash in GTK and other ports by using PLATFORM(chromium), though it is bad behavior.
The crash will be introduces by followed comments.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:-360
> -    m_shouldDiscardReceivedData = true;

m_client->didReceiveMessageError(); // Suppose fire error event

Suppose we do the same operation in GTK port, error event will be filed.
If developers call onclose in error event handle, and then GTK follows closing algorithm, didCloseSocketStream will be invoked synchronously(asynchronously in chromium),
didCloseSocketStream will call deref release itself. But followed handle->disconnect() is still required to execute, which will result in crash.

I think the main gap between chromium and other ports is whether didFailStreamSocket should call handle->disconnect. If we follow the behavior of chromium,
didFailStreamSocket just fire error event, it sounds a good idea, and we can call didCloseStreamSocket after didFailStreamSocket in SocketStreamHandle class of GTK port.

So the behavior of didFailStreamSocket should be discussed.
1. didFailStreamSocket need to disconnect handle to tracker didCloseStreamSocket.
2. didFailStreamSocket just fires error event, you need to call didCloseStreamSocket by yourself in SocketStreamHandle class if you want to close.

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