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

• Component: WebCore Misc.
• Status: NEW

[Li Yin]

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.

[Li Yin]

Currently, WebSocket object only fired close event in the following cases:
1. WebSocket server can't be connected.
2. The underlying transport layer connection is unexpectedly lost.

[Li Yin]

Created attachment 143762 [details]
Patch

[Yuta Kitamura]

Comment on attachment 143762 [details]
Patch

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.

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

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

[Li Yin]

(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.

[Yuta Kitamura]

Comment on attachment 143762 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=143762&action=review

>>> Source/WebCore/ChangeLog:17
>>> +        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.

Okay, so could you please put some sentences explaining the change to the ChangeLog? ChangeLog is the best place to put the rationale and brief overview of a patch. IMHO copy & pasting part of a specification in ChangeLog is not so friendly to readers.

[Li Yin]

Created attachment 143835 [details]
Patch

[Li Yin]

(In reply to comment #5)
> Okay, so could you please put some sentences explaining the change to the ChangeLog? ChangeLog is the best place to put the rationale and brief overview of a patch. IMHO copy & pasting part of a specification in ChangeLog is not so friendly to readers.

I have updated the log, added the some explantation, please have a check again.
Thanks in advance.

[Takashi Toyoshima]

Thank you for your continuous contributions to improve WebSocket implementation.

Behaviors looks OK.
But, I feel this handling should be in SocketStreamHandleBase class.

Currently,
 - WebSocket implements WebSocket API related features.
 - WebSocketChannel implements WebSocket protocol stack handlings.
 - SocketStreamHandle implements abstracted bidirectional communication port.

SocketStreamHandle has a client interface to notify network errors as
SocketStreamHandleClient::didFailSocketStream.
So this kind of error should be handled in SocketStreamHandle
and reported to SocketStreamHandleClient(== WebSocketchannel) via this interface.

[Li Yin]

Currently, the didFailSocketStream can't be invoked on chromium, because SocketStreamDispatcherHost didn't implement the vitual function "OnError".
So, both of close message and error message are handled by OnClose function in SocketStreamDispatcherHost.
WebSocketChannel::didCloseSocketStream handled two things, one for normal closing, another for abnomal aborting.

If we want to invaoke the WebSocketChannel::didFailSocketStream function, we must send the IPC message from SocketStreamDispatcherHost when the socket is closed abnormally.

I will have a try to do that.

[Li Yin]

Created attachment 145080 [details]
Patch

[Li Yin]

Currently this patch aims to fix the issue only for chromium platform.

There is a related bug in chromium to track this issue.
http://code.google.com/p/chromium/issues/detail?id=128057
And I have already uploaded the chromium-side patch.

Some main changes are listed as followed:
Chromium side:
1. Add OnError function in SocketStreamDispatcherHost Class, send the Error Message to SocketStreamDispatcher.
2. Add OnFailed function in SocketStreamDispatcher Class, handle the Error Message from SocketStreamDispatcherHost.
3. Add OnError function in WebSocketStreamHandleBridgeImpl, let DumpRenderTree can work like Browser.

WebKit side:
1. Change the behavior of didFail, call didFailSocketStream instead of didCloseSocketStream.

[WebKit Review Bot]

Comment on attachment 145080 [details]
Patch

Attachment 145080 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12867288

New failing tests:
http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html

[WebKit Review Bot]

Created attachment 145130 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Li Yin]

The test http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html depends on related chromium patch.
https://chromiumcodereview.appspot.com/10458057/

[Li Yin]

Hi Yuta, Takashi 

   Could you give some comments about the chromium side patch and webkit side patch?
   Thanks in advance.

[Takashi Toyoshima]

Comment on attachment 145080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145080&action=review

I looked over this patch and left some comments.
I should understand details, so I'll take another look tomorrow.

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

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

> Source/WebKit/chromium/src/SocketStreamHandle.cpp:-156
> -        m_socket.clear();

This part of change looks right.

> LayoutTests/ChangeLog:15
> +

It'd be better to have worker version of this test.
As you may already know, worker thread handling in WebSocketChannel is a little complicated.

[Li Yin]

(In reply to comment #16)
> (From update of attachment 145080 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145080&action=review
> 
> I looked over this patch and left some comments.
> I should understand details, so I'll take another look tomorrow.
> 
> > 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.

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

> 
> > Source/WebKit/chromium/src/SocketStreamHandle.cpp:-156
> > -        m_socket.clear();
> 
> This part of change looks right.
> 
> > LayoutTests/ChangeLog:15
> > +
> 
> It'd be better to have worker version of this test.
> As you may already know, worker thread handling in WebSocketChannel is a little complicated.
Okay, I will try to add tests for worker.

[Takashi Toyoshima]

Comment on attachment 145080 [details]
Patch

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.

[Li Yin]

Comment on attachment 145080 [details]
Patch

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.

[Li Yin]

WebSocket in WorkerContext should implement didReceiveMessageError to fire error event.
Set it depends on bug 88744

[Li Yin]

Created attachment 147511 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 147511 [details]
Patch

Attachment 147511 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12951561

[Li Yin]

Currently, this patch can work in GTK+ port.

For chromium platform, the new test case depends on chromium-side patch. So setting skipped the new tests, and I will remove it after the chromium-side patch landed.

For Mac port and BlackBerry, the implement isn't completed.
I will continue to do that.

[Li Yin]

Created attachment 147574 [details]
Patch

[Li Yin]

Fix Mac port building error, and Mac port can pass the new tests.

[Li Yin]

This patch just completed the main steps.

Hi Takashi, Yuta,
   Could you give some comments? If the main behavior is Okay, I will do next steps make it look perfect for all the platforms.
   Thanks in advance.

[Yuta Kitamura]

Comment on attachment 147574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147574&action=review

> Source/WebCore/ChangeLog:10
> +        Change the behavior of WebSocketChannel::didFailSocketStream, let it fire error event only,
> +        don't terminate the connection, we need call disconnect function after calling didFailSocketStream
> +        in SocketStreamHandle class if we want to disconnect connection.

Why can't didFailSocketStream() terminate the connection? Please explain the motivation behind this change in the ChangeLog.

> Source/WebCore/ChangeLog:22
> +        * platform/network/soup/SocketStreamHandleSoup.cpp:

SocketStreamHandleCFNet is missing?

> Source/Platform/chromium/public/WebSocketStreamError.h:42
> +    explicit WebSocketStreamError(int errorCode, const char* errorMsg)

1. I think it's better to use WebString here because: (a) the liveness of const char* is ambiguous (how long is it safe to touch errorMsg?), and (b) errorMsg is a user-facing string. WebString is a text type which is used for bridging WebKit's String (WTF::String) and Chromium's string16.

2. We don't usually use abbreviation like "Msg".

> Source/Platform/chromium/public/WebSocketStreamError.h:44
> +    : m_errorCode(errorCode)
> +    , m_errorMsg(errorMsg)

Indentation

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:-345
> -    ASSERT(handle == m_handle || !m_handle);

I think it's better to keep this ASSERT as a sanity check.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:363
> +    if (m_client) {
> +        if (!m_closing && !m_failed) {

These two ifs can be merged into one.

> LayoutTests/platform/chromium/TestExpectations:3792
> +BUGCR128057 SKIP : http/tests/websocket/tests/hybi/connect-error-by-no-websocket-server.html = FAIL

FAIL is now banned so please use more specific labels (TEXT, IMAGE or IMAGE+TEXT).

[Li Yin]

Comment on attachment 147574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147574&action=review

>> Source/WebCore/ChangeLog:10
>> +        in SocketStreamHandle class if we want to disconnect connection.
> 
> Why can't didFailSocketStream() terminate the connection? Please explain the motivation behind this change in the ChangeLog.

didFailSocketStream will fire error event, close can be invoked in onerror event handle, so the WebSocketChannel object maybe released already.
In that case, if we call disconnect function, it results in crash.

>> Source/WebCore/ChangeLog:22
>> +        * platform/network/soup/SocketStreamHandleSoup.cpp:
> 
> SocketStreamHandleCFNet is missing?

It is missed, I will add it. Thanks your reminding.

>> Source/Platform/chromium/public/WebSocketStreamError.h:42
>> +    explicit WebSocketStreamError(int errorCode, const char* errorMsg)
> 
> 1. I think it's better to use WebString here because: (a) the liveness of const char* is ambiguous (how long is it safe to touch errorMsg?), and (b) errorMsg is a user-facing string. WebString is a text type which is used for bridging WebKit's String (WTF::String) and Chromium's string16.
> 
> 2. We don't usually use abbreviation like "Msg".

Okay, I will change it.

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:-345
>> -    ASSERT(handle == m_handle || !m_handle);
> 
> I think it's better to keep this ASSERT as a sanity check.

handle isn't used except for ASSERT(handle == m_handle || !m_handle);
If keep the handle, it will result in building error in Release mode of Mac port.

[Li Yin]

Comment on attachment 147574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147574&action=review

>> Source/WebCore/ChangeLog:10
>> +        in SocketStreamHandle class if we want to disconnect connection.
> 
> Why can't didFailSocketStream() terminate the connection? Please explain the motivation behind this change in the ChangeLog.

didFailSocketStream will fire error event, close can be invoked in onerror event handle, so the WebSocketChannel object maybe released already.
In that case, if we call disconnect function, it results in crash.

>> Source/WebCore/ChangeLog:22
>> +        * platform/network/soup/SocketStreamHandleSoup.cpp:
> 
> SocketStreamHandleCFNet is missing?

It is missed, I will add it. Thanks your reminding.

>> Source/Platform/chromium/public/WebSocketStreamError.h:42
>> +    explicit WebSocketStreamError(int errorCode, const char* errorMsg)
> 
> 1. I think it's better to use WebString here because: (a) the liveness of const char* is ambiguous (how long is it safe to touch errorMsg?), and (b) errorMsg is a user-facing string. WebString is a text type which is used for bridging WebKit's String (WTF::String) and Chromium's string16.
> 
> 2. We don't usually use abbreviation like "Msg".

Okay, I will change it.

>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:-345
>> -    ASSERT(handle == m_handle || !m_handle);
> 
> I think it's better to keep this ASSERT as a sanity check.

handle isn't used except for ASSERT(handle == m_handle || !m_handle);
If keep the handle, it will result in building error in Release mode of Mac port.

[Yuta Kitamura]

Comment on attachment 147574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147574&action=review

>>>> Source/WebCore/ChangeLog:10
>>>> +        in SocketStreamHandle class if we want to disconnect connection.
>>> 
>>> Why can't didFailSocketStream() terminate the connection? Please explain the motivation behind this change in the ChangeLog.
>> 
>> didFailSocketStream will fire error event, close can be invoked in onerror event handle, so the WebSocketChannel object maybe released already.
>> In that case, if we call disconnect function, it results in crash.
> 
> didFailSocketStream will fire error event, close can be invoked in onerror event handle, so the WebSocketChannel object maybe released already.
> In that case, if we call disconnect function, it results in crash.

I'm confused. If you use guard objects appropriately, you can avoid (or delay) the destruction of WebSocketChannel. What's the scenario that causes the crash?

My gut feeling is that you don't have to change SocketStreamHandle* at all.

>>>> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:-345
>>>> -    ASSERT(handle == m_handle || !m_handle);
>>> 
>>> I think it's better to keep this ASSERT as a sanity check.
>> 
>> handle isn't used except for ASSERT(handle == m_handle || !m_handle);
>> If keep the handle, it will result in building error in Release mode of Mac port.
> 
> handle isn't used except for ASSERT(handle == m_handle || !m_handle);
> If keep the handle, it will result in building error in Release mode of Mac port.

You can use ASSERT_UNUSED in such cases.

[Li Yin]

Created attachment 148003 [details]
Patch

[Li Yin]

> > didFailSocketStream will fire error event, close can be invoked in onerror event handle, so the WebSocketChannel object maybe released already.
> > In that case, if we call disconnect function, it results in crash.
> 
> I'm confused. If you use guard objects appropriately, you can avoid (or delay) the destruction of WebSocketChannel. What's the scenario that causes the crash?
> 
> My gut feeling is that you don't have to change SocketStreamHandle* at all.

Okay, I updated the patch, didn't change SocketStreamHandle*, just added didReceiveMessageError in didFailSocketStream.
Thanks your comments.

> You can use ASSERT_UNUSED in such cases.
Done.

[Yuta Kitamura]

Comment on attachment 148003 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148003&action=review

> Source/Platform/chromium/public/WebSocketStreamError.h:54
> +    WebSocketStreamError() { }
> +
> +    explicit WebSocketStreamError(int errorCode, const WebString& errorMessage)
> +        : m_errorCode(errorCode)
> +        , m_errorMessage(errorMessage)
> +    {
> +    }
> +
> +    int errorCode() const { return m_errorCode; }
> +    const WebString errorMessage() const { return m_errorMessage; }
> +
> +private:
> +    int m_errorCode;
> +    WebString m_errorMessage;

* In general, WebXXX classes should correspond to WebCore's classes named XXX, and WebXXX should have compatible interface. In this case, SocketStreamError takes three arguments (errorCode, failingURL and localizedDescription) so WebSocketStreamError is expected to take the similar arguments.

* Also, you need to provide a conversion function from WebCore::SocketStreamError type inside WEBKIT_IMPLEMENTATION macro guards (I guess these getters aren't necessary at all, though).

* If you want to expose public member functions to Chromium code, you need to put WEBKIT_EXPORT macro to the function declaration.

* I think it's better to hold "WebPrivateError<WebCore::SocketStreamError> m_private" instead of holding actual error code and error message. See other Chromium API code to learn this style.

* For more about Chromium's WebKit API, read <http://trac.webkit.org/wiki/ChromiumWebKitAPI>. Take a look at WebURLError (corresponding to ResourceError) and WebGeolocationError (corresponding to GeolocationError) to learn more.

[Li Yin]

(In reply to comment #34)
> (From update of attachment 148003 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148003&action=review
> 
> > Source/Platform/chromium/public/WebSocketStreamError.h:54
> > +    WebSocketStreamError() { }
> > +
> > +    explicit WebSocketStreamError(int errorCode, const WebString& errorMessage)
> > +        : m_errorCode(errorCode)
> > +        , m_errorMessage(errorMessage)
> > +    {
> > +    }
> > +
> > +    int errorCode() const { return m_errorCode; }
> > +    const WebString errorMessage() const { return m_errorMessage; }
> > +
> > +private:
> > +    int m_errorCode;
> > +    WebString m_errorMessage;
> 
> * In general, WebXXX classes should correspond to WebCore's classes named XXX, and WebXXX should have compatible interface. In this case, SocketStreamError takes three arguments (errorCode, failingURL and localizedDescription) so WebSocketStreamError is expected to take the similar arguments.
> 
> * Also, you need to provide a conversion function from WebCore::SocketStreamError type inside WEBKIT_IMPLEMENTATION macro guards (I guess these getters aren't necessary at all, though).
> 
> * If you want to expose public member functions to Chromium code, you need to put WEBKIT_EXPORT macro to the function declaration.
> 
> * I think it's better to hold "WebPrivateError<WebCore::SocketStreamError> m_private" instead of holding actual error code and error message. See other Chromium API code to learn this style.
> 
> * For more about Chromium's WebKit API, read <http://trac.webkit.org/wiki/ChromiumWebKitAPI>. Take a look at WebURLError (corresponding to ResourceError) and WebGeolocationError (corresponding to GeolocationError) to learn more.

Thanks for your good comments, I will improve it.

[Li Yin]

Created attachment 149227 [details]
Patch

[Yuta Kitamura]

Comment on attachment 149227 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149227&action=review

Looks legit overall.

> Source/WebCore/Modules/websockets/WebSocketChannel.cpp:357
> +        message = "WebSocket network error: " + error.localizedDescription();

It might be better to provide error code in addition to description.

> Source/WebCore/platform/network/SocketStreamErrorBase.h:43
> +#if !PLATFORM(CHROMIUM)

Why do we need this #if guard?

[Li Yin]

Comment on attachment 149227 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149227&action=review

>> Source/WebCore/platform/network/SocketStreamErrorBase.h:43
>> +#if !PLATFORM(CHROMIUM)
> 
> Why do we need this #if guard?

On chromium platform, SocketStreamError inherits RefCounted<SocketStreamError>, it is different with other ports.
SocketStreamErrorBase::copy function will cause some errors because of returned value.
And I can't find places use SocketStreamErrorBase::copy or ocketStreamErrorBase::compare, maybe we can delete the implementation of SocketStreamErrorBase.

[Li Yin]

Created attachment 154548 [details]
Patch

[Li Yin]

(In reply to comment #37)
> (From update of attachment 149227 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149227&action=review
> 
> Looks legit overall.
> 
> > Source/WebCore/Modules/websockets/WebSocketChannel.cpp:357
> > +        message = "WebSocket network error: " + error.localizedDescription();
> 
> It might be better to provide error code in addition to description.
> 

Done.

And rebase to the newest trunk code.

[Anders Carlsson]

Comment on attachment 154548 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.