Make the WebSocket fail messages meaningful

Make the WebSocket fail messages meaningful

Currently the source file and the line number of fail messages to be shown is <origin url> and 1 respectively.
It is helpless for developers.
Fixed the behavior by:
 1) If the error is occurred in the JS context, show the source file and the line number of the calling statement.
 2) Otherwise, show the source file and the line number where the Websocket object is created.

Note that we can't test 1) behavior by the LayoutTests because the completion of the source file and the line number is not done in DumpRenderTree.

This CL doesn't fix the problem for WebSocket objects in a WebWorker.

R=tyoshino
BUG=164546
TEST=http/tests/websocket

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149148

[yhirano, 2013-04-19 06:29:10]

Hi, I was sorry for confusing you, but it is ready now.
Can you review the patch?

[tyoshino (SeeGerritForStatus), 2013-04-19 14:18:10]

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:49: #include
"ScriptCallStack.h"
swap order of these two

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:207: // So, the
addConsoleMessage method will show the stack apropriately.
apro -> appro

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:208:
m_document->addConsoleMessage(JSMessageSource, ErrorMessageLevel, message);
Could you please check which of JSMessageSource and NetworkMessageSource is
suitable for this call. AFAIK, this is converted into json and passed to the
inspector frontend javascript, but looks like not used yet.

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:214:
static_cast<ScriptExecutionContext*>(m_document)->addConsoleMessage(JSMessageSource,
ErrorMessageLevel, message, url, lineNumber, 0, 0);
Without this cast, we cannot call the non-virtual addConsoleMessage?

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
File Source/modules/websockets/MainThreadWebSocketChannel.h (right):

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.h:202: ScriptCallFrame
m_callframeAtConnection;
use the same capitalization?
m_callFrame...

[yhirano, 2013-04-22 05:18:29]

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:49: #include
"ScriptCallStack.h"
On 2013/04/19 14:18:10, tyoshino wrote:
> swap order of these two

Done.

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:207: // So, the
addConsoleMessage method will show the stack apropriately.
On 2013/04/19 14:18:10, tyoshino wrote:
> apro -> appro

Done.

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:208:
m_document->addConsoleMessage(JSMessageSource, ErrorMessageLevel, message);
On 2013/04/19 14:18:10, tyoshino wrote:
> Could you please check which of JSMessageSource and NetworkMessageSource is
> suitable for this call. AFAIK, this is converted into json and passed to the
> inspector frontend javascript, but looks like not used yet.
devtools JS (Source/devtools/front_end/ConsoleMessage.js) would change the
appearance of logs according to the message source parameter.
WebSocket.cpp uses JSMessageSource, too.
According to codesearch, no one but MainThreadWebSocketChannel uses
NetworkMessageSource.

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:214:
static_cast<ScriptExecutionContext*>(m_document)->addConsoleMessage(JSMessageSource,
ErrorMessageLevel, message, url, lineNumber, 0, 0);
On 2013/04/19 14:18:10, tyoshino wrote:
> Without this cast, we cannot call the non-virtual addConsoleMessage?
In C++, a member function of a derived class hides same-name  member functions
of its base class.
Maybe the Document class should declare the non-virtual addMessageConsole
function with the "using" keyword.

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
File Source/modules/websockets/MainThreadWebSocketChannel.h (right):

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.h:202: ScriptCallFrame
m_callframeAtConnection;
On 2013/04/19 14:18:10, tyoshino wrote:
> use the same capitalization?
> m_callFrame...

Done.

[tyoshino (SeeGerritForStatus), 2013-04-22 05:47:17]

https://codereview.chromium.org/14320014/diff/5001/Source/modules/websockets/...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/14320014/diff/5001/Source/modules/websockets/...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:214:
static_cast<ScriptExecutionContext*>(m_document)->addConsoleMessage(JSMessageSource,
ErrorMessageLevel, message, url, lineNumber, 0, 0);
How about continue not showing lineno here in case it failed to get call frame
on construction?

[tyoshino (SeeGerritForStatus), 2013-04-22 06:32:34]

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:208:
m_document->addConsoleMessage(JSMessageSource, ErrorMessageLevel, message);
On 2013/04/22 05:18:29, yhirano wrote:
> On 2013/04/19 14:18:10, tyoshino wrote:
> > Could you please check which of JSMessageSource and NetworkMessageSource is
> > suitable for this call. AFAIK, this is converted into json and passed to the
> > inspector frontend javascript, but looks like not used yet.
> devtools JS (Source/devtools/front_end/ConsoleMessage.js) would change the
> appearance of logs according to the message source parameter.
> WebSocket.cpp uses JSMessageSource, too.
> According to codesearch, no one but MainThreadWebSocketChannel uses
> NetworkMessageSource.

XHR is using it too but in InspectorConsoleAgent.

This should be because the only objects that involve network event are XHR,
resource loading and WebSocket.

I think we should use NetworkMessageSource when fail() is invoked from
processBuffer(), processFrame(), didReceiveSocketStreamData() and maybe
didOpenSocketStream(). You can do this in a separate CL.

For call from didFail, I'm not sure which is the best. Let's use JSMessageSource
for now.

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:214:
static_cast<ScriptExecutionContext*>(m_document)->addConsoleMessage(JSMessageSource,
ErrorMessageLevel, message, url, lineNumber, 0, 0);
On 2013/04/22 05:18:29, yhirano wrote:
> On 2013/04/19 14:18:10, tyoshino wrote:
> > Without this cast, we cannot call the non-virtual addConsoleMessage?
> In C++, a member function of a derived class hides same-name  member functions
> of its base class.
> Maybe the Document class should declare the non-virtual addMessageConsole
> function with the "using" keyword.

OK

[yhirano, 2013-04-22 07:33:38]

I uploaded a patch fixing a crash bug in the former patches.
Also I updated a issue description.

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/14320014/diff/1/Source/modules/websockets/Mai...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:208:
m_document->addConsoleMessage(JSMessageSource, ErrorMessageLevel, message);
On 2013/04/22 06:32:34, tyoshino wrote:
> On 2013/04/22 05:18:29, yhirano wrote:
> > On 2013/04/19 14:18:10, tyoshino wrote:
> > > Could you please check which of JSMessageSource and NetworkMessageSource
is
> > > suitable for this call. AFAIK, this is converted into json and passed to
the
> > > inspector frontend javascript, but looks like not used yet.
> > devtools JS (Source/devtools/front_end/ConsoleMessage.js) would change the
> > appearance of logs according to the message source parameter.
> > WebSocket.cpp uses JSMessageSource, too.
> > According to codesearch, no one but MainThreadWebSocketChannel uses
> > NetworkMessageSource.
> 
> XHR is using it too but in InspectorConsoleAgent.
> 
> This should be because the only objects that involve network event are XHR,
> resource loading and WebSocket.
> 
> I think we should use NetworkMessageSource when fail() is invoked from
> processBuffer(), processFrame(), didReceiveSocketStreamData() and maybe
> didOpenSocketStream(). You can do this in a separate CL.
> 
> For call from didFail, I'm not sure which is the best. Let's use
JSMessageSource
> for now.
I see, thank you. I will file a bug.

https://codereview.chromium.org/14320014/diff/5001/Source/modules/websockets/...
File Source/modules/websockets/MainThreadWebSocketChannel.cpp (right):

https://codereview.chromium.org/14320014/diff/5001/Source/modules/websockets/...
Source/modules/websockets/MainThreadWebSocketChannel.cpp:214:
static_cast<ScriptExecutionContext*>(m_document)->addConsoleMessage(JSMessageSource,
ErrorMessageLevel, message, url, lineNumber, 0, 0);
On 2013/04/22 05:47:18, tyoshino wrote:
> How about continue not showing lineno here in case it failed to get call frame
> on construction?
Fixed around the line 113 for this problem.

[commit-bot: I haz the power, 2013-04-22 10:23:49]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[tyoshino (SeeGerritForStatus), 2013-04-25 05:42:11]

LGTM

[commit-bot: I haz the power, 2013-04-25 05:42:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/14320014/10003

[commit-bot: I haz the power, 2013-04-25 05:42:29]

Failed to apply patch for
Source/modules/websockets/MainThreadWebSocketChannel.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/websockets/MainThreadWebSocketChannel.cpp
  Hunk #1 FAILED at 46.
  Hunk #2 succeeded at 87 with fuzz 1 (offset 1 line).
  Hunk #3 succeeded at 109 (offset 1 line).
  Hunk #4 succeeded at 199 (offset 1 line).
  1 out of 4 hunks FAILED -- saving rejects to file
Source/modules/websockets/MainThreadWebSocketChannel.cpp.rej

Patch:       Source/modules/websockets/MainThreadWebSocketChannel.cpp
Index: Source/modules/websockets/MainThreadWebSocketChannel.cpp
diff --git a/Source/modules/websockets/MainThreadWebSocketChannel.cpp
b/Source/modules/websockets/MainThreadWebSocketChannel.cpp
index
b2f9e43e41eb61b36b68cfc865a5153090fa9a1c..c6ef9e9627a724400bc59158bc83db949fa403bd
100644
--- a/Source/modules/websockets/MainThreadWebSocketChannel.cpp
+++ b/Source/modules/websockets/MainThreadWebSocketChannel.cpp
@@ -46,6 +46,7 @@
 #include "Page.h"
 #include "ProgressTracker.h"
 #include "ScriptCallStack.h"
+#include "ScriptCallStackFactory.h"
 #include "ScriptExecutionContext.h"
 #include "Settings.h"
 #include "SocketStreamError.h"
@@ -86,6 +87,7 @@
MainThreadWebSocketChannel::MainThreadWebSocketChannel(Document* document, WebSo
     , m_closeEventCode(CloseEventCodeAbnormalClosure)
     , m_outgoingFrameQueueStatus(OutgoingFrameQueueOpen)
     , m_blobLoaderStatus(BlobLoaderNotStarted)
+    , m_callFrameAtConnection("", "", 0)
 {
     if (Page* page = m_document->page())
         m_identifier = page->progress()->createUniqueIdentifier();
@@ -107,6 +109,8 @@ void MainThreadWebSocketChannel::connect(const KURL& url,
const String& protocol
         InspectorInstrumentation::didCreateWebSocket(m_document, m_identifier,
url, m_document->url(), protocol);
     ref();
     m_handle = SocketStreamHandle::create(m_handshake->url(), this);
+    RefPtr<ScriptCallStack> callstack = createScriptCallStack(1, true);
+    m_callFrameAtConnection = callstack && callstack->size() > 0 ?
callstack->at(0) : ScriptCallFrame("", "", 0);
 }
 
 String MainThreadWebSocketChannel::subprotocol()
@@ -195,7 +199,19 @@ void MainThreadWebSocketChannel::fail(const String& reason)
     ASSERT(!m_suspended);
     if (m_document) {
         InspectorInstrumentation::didReceiveWebSocketFrameError(m_document,
m_identifier, reason);
-        m_document->addConsoleMessage(NetworkMessageSource, ErrorMessageLevel,
"WebSocket connection to '" + m_handshake->url().elidedString() + "' failed: " +
reason);
+        const String message = "WebSocket connection to '" +
m_handshake->url().elidedString() + "' failed: " + reason;
+        RefPtr<ScriptCallStack> callstack = createScriptCallStack(1, true);
+        if (callstack && callstack->size() > 0) {
+            // We are in a JS callstack.
+            // So, the addConsoleMessage method will show the stack
appropriately.
+            m_document->addConsoleMessage(JSMessageSource, ErrorMessageLevel,
message);
+        } else {
+            // We are not in a JS callstack.
+            // Then show the source file and the line number at the connection
initiation.
+            const String& url = m_callFrameAtConnection.sourceURL();
+            unsigned lineNumber = m_callFrameAtConnection.lineNumber();
+           
static_cast<ScriptExecutionContext*>(m_document)->addConsoleMessage(JSMessageSource,
ErrorMessageLevel, message, url, lineNumber, 0, 0);
+        }
     }
 
     // Hybi-10 specification explicitly states we must not continue to handle
incoming data

[commit-bot: I haz the power, 2013-04-25 09:09:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/14320014/20001

[commit-bot: I haz the power, 2013-04-25 09:30:39]

Retried try job too often on mac_layout_rel for step(s) test_shell_tests,
webkit_lint, webkit_tests, webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-04-25 09:36:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/14320014/20001

[commit-bot: I haz the power, 2013-04-25 09:57:51]

Retried try job too often on mac_layout_rel for step(s) test_shell_tests,
webkit_lint, webkit_tests, webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-04-25 10:12:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/14320014/20001

[commit-bot: I haz the power, 2013-04-25 10:15:52]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_layout_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-25 10:24:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/14320014/20001

[commit-bot: I haz the power, 2013-04-25 22:56:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/14320014/23002

[commit-bot: I haz the power, 2013-04-26 00:33:47]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-04-26 00:36:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yhirano@chromium.org/14320014/23002

[commit-bot: I haz the power, 2013-04-26 01:21:01]

Change committed as 149148