[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 #18 from Takashi Toyoshima <toyoshim at chromium.org>  2012-06-06 23:50:12 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:234
>>> +        }
>> 
>> Here, you try to call didReceiveMessageError() at most once.
>> But I notice that didReceiveMessageError() is invoked from many places in this source code.
>> This looks another original bug on WebSocketChannel (Or, is this introduced at previous change?)
> 
> There is not a obvious language in Spec to describe whether error event should be fired more than once or not.
> If WebSocket fails the connection when fire an error event, it seems that it is not necessary to fire more error events.

I agree for now.
Maybe we should clarify the behavior and will fix in another patch if needed.

>>> 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)

>>> Source/WebKit/chromium/src/SocketStreamHandle.cpp:-156
>>> -        m_socket.clear();
>> 
>> This part of change looks right.
> 
> Okay, I will try to add tests for worker.

As I commented above, how about keeping original code from line 156 to 159.
I think all you have to do is just change the client function from didCloseSocketStream to didFailSocketStream in line 160.

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