603750 - nsWebSocket connection failure messages do not show in the Web Console

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Mihai Sucan [:msucan]]

The nsWebSocket.cpp file holds the nsWebSocketEstablishedConnection::PrintErrorOnConsole() method which is used to output connection error messages to the console service. Currently, the implementation uses logStringMessage() which has no indication of the message origin. The Web Console is currently unable to show such messages, because it cannot associate them to a specific tab.

Comment 1

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch. This makes Web Socket connection errors show up in the Web Console.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Why is the document URI the right thing?  What if the websocket stuff is being done from some external script attached to that document?

Comment 3

[Mihai Sucan [:msucan]]

Attachment: patch update 2

Updated patch. Thanks Boris!

Made changes based on your comment and based on an IRC #jsapi discussion. When the WebSocket is initialized I store the window ID, source script file and line number. This is indeed better than just providing the document URI.

Comment 4

[Mihai Sucan [:msucan]]

Asking for blocking2.0+: the new HTML5 Web Sockets API is just introduced in Firefox 4, this means many web devs will try it when Firefox final is released. The old Error console is disabled by default, and the new Web Console does not show any connection errors. This patch makes Web Socket connection errors show up in the new Web Console, improving the experience for any adventurous web dev.

Thanks!

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 490351 [details] [diff] [review]
patch update 2

>+++ b/content/base/src/nsWebSocket.cpp
>+#include "jscntxt.h"

That's a private jseng header; you shouldn't be using it here.  More on this below.

>+  rv = console->LogMessage(
>+    (nsCOMPtr<nsIScriptError>)(do_QueryInterface(errorObject)));

I'd really prefer to have the nsCOMPtr+QI on a separate line before the LogMessage call.

You need to initialize your newly-added integer members in the constructor.

>@@ -3508,19 +3523,41 @@ nsWebSocket::Init(nsIPrincipal* aPrincip
>   if (aOwnerWindow) {
>     mOwner = aOwnerWindow->IsOuterWindow() ?
>       aOwnerWindow->GetCurrentInnerWindow() : aOwnerWindow;
>+
>+    mWindowID = aOwnerWindow->GetOuterWindow()->WindowID();

Is that the behavior you want?

In particular, for a cross-window websocket creation like so:

  |var sock = new otherWin.WebSocket("something")|

which window do you want here for the window id?  The window the script is running in, or otherWin?  aOwnerWindow is otherWin, I believe.  Test?

>+    nsCOMPtr<nsIDocument> doc =
>+      nsContentUtils::GetDocumentFromScriptContext(aScriptContext);

Likewise, aScriptContext here is the context for otherWin above, not necessarily where the script is running.  Again, test?

>+    nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow();

Why does this need to be a strong ref?

>+  JSContext* cx = (JSContext *) aScriptContext->GetNativeContext();

1)  aScriptContext is allowed to be null.
2)  Nothing says it's a JS context.  You should check GetScriptTypeID() instead
    of assuming it's JS (see nsIProgrammingLanguage).

>+  for (JSStackFrame *fp = js_GetTopStackFrame(cx); fp; fp = fp->prev()) {
>+      if (fp->pc(cx)) {
>+          mScriptFile = fp->script()->filename;
>+          mScriptLine = js_FramePCToLineNumber(cx, fp);
>+          break;
>+      }

That's using jseng-internal APIs.  There is no guarantee that this will link; in fact in a number of our configurations it won't.

You want something more like JS_GetScriptedCaller (with null fp arg) to find the relevant stack frame, JS_GetFrameScript to get the script, JS_GetFramePC to get the pc, JS_PCToLineNumber to get the line number, JS_GetScriptFilename to get the filename.  These all live in jsdbgapi.h

>+++ b/content/base/src/nsWebSocket.h

>+  PRUint64 WindowID() { return mWindowID; }
>+  nsCAutoString GetScriptFile() { return mScriptFile; }

The return value here should probably be |const nsCString&|.

>+  PRUint32 GetScriptLine() { return mScriptLine; }

All three of these should be const methods.

>+  nsCAutoString mScriptFile;

nsCString.  And document that it's UTF-8 encoded?

Comment 6

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Updated patch. Thank you Boris for your review! I hope this patch properly addresses your comments.

I made a test now which checks what you asked for, and indeed, you are correct, the previous patch failed with the new test. I didn't know I was using JS API internal APIs (I just searched these things through MXR and used whatever worked at that moment).

Comment 7

[Kevin Dangoor]

mass change: filter on PRIORITYSETTING

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 492298 [details] [diff] [review]
patch update 3

>+  const PRUint64 WindowID() { return mWindowID; }

I was thinking more like:

  const PRUint64 WindowID() const { return mWindowID; }

>+  const nsCString& GetScriptFile() { return mScriptFile; }
>+  const PRUint32 GetScriptLine() { return mScriptLine; }

and similar here.

There was talk of having a helper that takes nsIScriptGlobalObject and hands back window id; we should use that here.

r=me with those nits.

Comment 9

[Mihai Sucan [:msucan]]

Attachment: patch update 4

Updated the patch accordingly. Thanks for the review+!

This patch now depends on bug 606498.

Comment 10

[Johnny Stenback (:jst)]

Not blocking 2.0 on this, but feel free to land the approved patch.

Comment 11

[Mihai Sucan [:msucan]]

Thanks Johnny for the approval flag!

Updating the whiteboard to warn people who might want to checkin this patch: this patch has a *hard* dependency on bug 606498. We need to wait for that, otherwise build fails.

Comment 12

[Johnny Stenback (:jst)]

Ok, thanks for that information! In that case I'm removing checkin-needed to lower the risk of someone landing this too soon. Once bug 606498 is landed, please add back the keyword...

Comment 13

[Mihai Sucan [:msucan]]

Attachment: rebased patch

Rebased the patch on top of the latest changes in bug 606498 and mozilla-central trunk. Ready for checkin.

Comment 14

[Rob Campbell [:rc] (:robcee)]

Attachment: mochitest-browser.chrome.log

ran this with patches from bug 606498 and bug 603706 applied and this reported 72 errors. See attached log for details.

Comment 15

[Mihai Sucan [:msucan]]

Attachment: [checked-in] test bug fix

Fixed the test failure. Switched away from using ws://0.0.0.0/ and ws://0.0.0.1/ to ws://0.0.0.0:81/ and ws://0.0.0.0:82/. Sorry for the trouble!

Comment 16

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 498814 [details] [diff] [review]
[checked-in] test bug fix

http://hg.mozilla.org/mozilla-central/rev/a599d358d01a