WebSocket object should fire error event when WebSocket server can't be connected.

Source/modules/websockets/WebSocketChannel.cpp

Index: Source/modules/websockets/WebSocketChannel.cpp
diff --git a/Source/modules/websockets/WebSocketChannel.cpp b/Source/modules/websockets/WebSocketChannel.cpp
index 344c8b485df322a040890b43553efd2fcdedcba4..776cee48c23aca4104cbb8d963074b723056e2a5 100644
--- a/Source/modules/websockets/WebSocketChannel.cpp
+++ b/Source/modules/websockets/WebSocketChannel.cpp
@@ -75,6 +75,7 @@ WebSocketChannel::WebSocketChannel(Document* document, WebSocketChannelClient* c
     , m_resumeTimer(this, &WebSocketChannel::resumeTimerFired)
     , m_suspended(false)
     , m_closing(false)
+    , m_failed(false)
     , m_receivedClosingHandshake(false)
     , m_closingTimer(this, &WebSocketChannel::closingTimerFired)
     , m_closed(false)
@@ -207,7 +208,10 @@ void WebSocketChannel::fail(const String& reason)
     m_deflateFramer.didFail();
     m_hasContinuousFrame = false;
     m_continuousFrameData.clear();
-    m_client->didReceiveMessageError();
+    if (!m_failed) {
+        m_failed = true;

[tyoshino (SeeGerritForStatus), 2013/04/17 13:50:10]

Let's check m_client here. I know that it's currently called by the registered
client and it'll never be NULL, but I think it's better not to connect these
classes tightly using the knowledge.

+        m_client->didReceiveMessageError();
+    }
 
     if (m_handle && !m_closed)
         m_handle->disconnect(); // Will call didClose().

[tyoshino (SeeGerritForStatus), 2013/04/17 13:50:10]

Could you please s/didClose/didCloseSocketStream/ using this opportunity?

[Li Yin, 2013/04/18 02:50:05]

didClose will been invoked already after m_handle->disconnect().
Because m_handle->disconnect() will trigger underlying SocketStream close, and
then the didClose callback will be invoked when underlying SocketStream was
closed.

[tyoshino (SeeGerritForStatus), 2013/04/18 03:28:16]

Yes. But it's named didCloseSocketStream. Not didClose. Then,
didCloseSocketStream calls m_client->didClose(). We're in WebSocketChannel. Not
to confuse readers, we should clarify what this didClose() means. If it's
m_client->didClose(), we should write so.

We've renamed WebSocketChannel::didClose() to didCloseSocketStream() in this CL.
I think didClose() in this comment meant WebSocketChannel::didClose(). But it
wasn't updated.
https://bugs.webkit.org/show_bug.cgi?id=65374
https://bugs.webkit.org/attachment.cgi?id=102482&action=review

[Li Yin, 2013/04/18 07:03:03]

Okay, thanks for your clarification. I will change the comments "Will call
didClose" into "Will call didCloseSocketStream".

[tyoshino (SeeGerritForStatus), 2013/04/18 08:29:59]

Yes. Thanks

@@ -322,20 +326,27 @@ void WebSocketChannel::didUpdateBufferedAmount(SocketStreamHandle*, size_t buffe
 void WebSocketChannel::didFailSocketStream(SocketStreamHandle* handle, const SocketStreamError& error)
 {
     LOG(Network, "WebSocketChannel %p didFailSocketStream()", this);
-    ASSERT(handle == m_handle || !m_handle);
-    if (m_document) {
-        String message;
-        if (error.isNull())
-            message = "WebSocket network error";
-        else if (error.localizedDescription().isNull())
-            message = "WebSocket network error: error code " + String::number(error.errorCode());
-        else
-            message = "WebSocket network error: " + error.localizedDescription();
-        InspectorInstrumentation::didReceiveWebSocketFrameError(m_document, m_identifier, message);

[tyoshino (SeeGerritForStatus), 2013/04/17 13:50:10]

removed as intended?

-        m_document->addConsoleMessage(NetworkMessageSource, ErrorMessageLevel, message);

[tyoshino (SeeGerritForStatus), 2013/04/17 13:50:10]

ditto

[Li Yin, 2013/04/18 02:50:05]

Currently, the onerror event is still called very sensitively. And most of
websocket tests will trigger this event.
For example:
When websocket server send closed frame, and then immediately abort the
underlying tcp connection, but browser is still needed to send close frame. It
will trigger onerror event. See close-code-and-reason.html

What I want to say is that if we add console message, most of the websocket
layout tests expectation will be modified.
And maybe exposing these internal error information to users is not necessary.

What is your thought?

[tyoshino (SeeGerritForStatus), 2013/04/18 03:28:16]

OK. Please keep them removed. Let's postpone fixing that.

-    }
+    ASSERT_UNUSED(handle, handle == m_handle || !m_handle);
     m_shouldDiscardReceivedData = true;
-    handle->disconnect();
+    String message;
+    if (error.isNull())
+        message = "WebSocket network error";
+    else if (error.localizedDescription().isNull())
+        message = "WebSocket network error: error code " + String::number(error.errorCode());
+    else
+        message = "WebSocket network error: error code " + String::number(error.errorCode()) + ", " + error.localizedDescription();
+    String failingURL = error.failingURL();
+    ASSERT(failingURL.isNull() || m_handshake->url().string() == failingURL);
+    if (failingURL.isNull())
+        failingURL = m_handshake->url().string();
+    LOG(Network, "Error Message: %s, FailURL: %s", message.utf8().data(), failingURL.utf8().data());

[tyoshino (SeeGerritForStatus), 2013/04/17 13:50:10]

Please
- add "WebSocketChannel %p" boilerplate
- quote %s ('%s')

+    RefPtr<WebSocketChannel> protect(this);
+    if (m_client && !m_closing && !m_failed) {

[tyoshino (SeeGerritForStatus), 2013/04/17 13:50:10]

Sorry if i'm misunderstanding, but !m_closing is not necessary here?

[Li Yin, 2013/04/18 07:03:03]

It is not necessary any more, because the issue is fixed on
https://codereview.chromium.org/10668018
I will remove it.

+        m_failed = true;
+        m_client->didReceiveMessageError();
+    }
+    if (m_handle && !m_closed)
+        m_handle->disconnect();
 }
 
 void WebSocketChannel::didReceiveAuthenticationChallenge(SocketStreamHandle*, const AuthenticationChallenge&)