595934 - Some xpconnect and Chrome errors should be displayed in the web console

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[David Dahl :ddahl]

<ddahl|afk> web consoles ignore xpconnect and chrome errors
<smaug_> we have been using error console for messages which tell web developers that they've been using a deprecated feature
<smaug_> and we're planning to use that more
<ddahl|afk> smaug_: sounds like a new bug for devtools
<ddahl|afk> in this case we should not ignore all xpconnect/chrome errors
<smaug_> blocker bug
<ddahl|afk> smaug_: will you file? do you know where those messages are created?
<smaug_> in DOM
<smaug_> web console seems to not show anything useful for me
<ddahl|afk> smaug_: i will it
<smaug_> I mean any useful error messages
<ddahl|afk> file it

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Error console has been used successfully for example in DOM Events to inform
web developers that they are using deprecated features.

We must not lose the only way to indicate about deprecated features.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

And IMO, this should block the next beta, especially *if* we're going to
warn about WebSocket usage (about using non-stable protocol).

Comment 3

[David Dahl :ddahl]

We need to identify the best way to filter these messages as something to surface in the web console. Right now we see messages via nsIConsoleService that have a category property

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/HUDService.jsm#5094

Are these messages annotated similarly? or all in the same manner?

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Smaug, do you think Beta 8 will be OK for this? Kind of reluctant to add it to the b7 list which we're trying to ratchet down on.

Comment 5

[David Dahl :ddahl]

(In reply to comment #3) 
> Are these messages annotated similarly? or all in the same manner?

smaug: do you know what might be common about these messages, and how we can identify them, or do we need to keep an array of messages to identify them with? (or something like that). 

Can you paste in an mxr link for any messages you know about?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

B8 might be ok.

It is possible that there is nothing common in those messages.
They are probably usually sent using nsContentUtils::ReportToConsole, but
it is possible that some cases use consoleservice (not via nsContentUtils::ReportToConsole).

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

...apparently nsIConsoleService is used in cases when we want to show
the error to web developer.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Btw, it is not at all clear to me what kind of errors are shown in the web console. (Web console doesn't seem to work at all in this build I'm using atm.)

Comment 9

[David Dahl :ddahl]

(In reply to comment #8)
> Btw, it is not at all clear to me what kind of errors are shown in the web
> console. (Web console doesn't seem to work at all in this build I'm using atm.)

I am using today's nightly and it works for me. control-shift-K. The messages are pulled from several observers including the nsIConsoleService

Comment 10

[David Dahl :ddahl]

It sounds like we need to make these messages parse-able so they can be identified as they are available in the console observer. Perhaps with a prefix? If I file another bug, what component would it go under?

Comment 11

[David Dahl :ddahl]

We need to add "DOM Events" to our nsIConsoleService observer as a category to surface in the web console.

Comment 12

[David Dahl :ddahl]

Here is where we switch through the nsIConsoleMessage.category:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/console/hudservice/HUDService.jsm?force=1#5221

We need to figure out what other categories we are missing in that routine.

Comment 13

[David Dahl :ddahl]

Another category: "DOM:HTML"

Comment 14

[David Dahl :ddahl]

We need a test case for each nsIConsoleMessage category...

<smaug_> ddahl: so, message sent to "DOM Events" category isn't shown in the web console
<smaug_> ddahl: testcase: javascript: document.createEvent("Event").preventBubble();

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Also if you run invalid javascript in the location bar, the
error isn't shown in the web console, but is in the error console.
Testcase:
javascript: 123 + foobar;

And note, there can very well be more categories which should be reported.

Comment 16

[David Dahl :ddahl]

(In reply to comment #15)
> And note, there can very well be more categories which should be reported.

Do you know who else to cc on this bug that might know of other categories?

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

I doubt anyone really knows them all. You need to go through all of them
and check whether the message would be useful to web developers.

I don't quite understand the hud service.
I'd thought default: case would report messages to web console, but
apparently that isn't happening.
What is the difference between reportConsoleServiceContentScriptError and
reportConsoleServiceMessage?

Comment 18

[David Dahl :ddahl]

(In reply to comment #17)
> I doubt anyone really knows them all. You need to go through all of them
> and check whether the message would be useful to web developers.
> 
> I don't quite understand the hud service.
> I'd thought default: case would report messages to web console, but
> apparently that isn't happening.
> What is the difference between reportConsoleServiceContentScriptError and
> reportConsoleServiceMessage?

If I remember correctly, there is a potential for an nsIConsoleMessage to appear in the observer, which will have different properties than an nsIScriptError, so we handle them differently.

Comment 19

[David Dahl :ddahl]

(In reply to comment #17)
> I doubt anyone really knows them all.

correct, I was asking for more than one person to cc, as this will take some group knowledge.

> You need to go through all of them and check whether the message would be useful to web developers.

I'm not sure how that is possible without making the messages parse-able via a prefix or making sure all known categories are checked and surfaced to the web console. 

> 
> I don't quite understand the hud service.
> I'd thought default: case would report messages to web console, but
> apparently that isn't happening.

The default case is to hide messages that have no bearing to the developer of the current tab's contentWindow. The noise levels in the JS Error console are just too high.

We do want to surface all relevant messages, this is great information, sounds like we just need to identify all categories that exist.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #19)
> (In reply to comment #17)
> > I doubt anyone really knows them all.
> 
> correct, I was asking for more than one person to cc, as this will take some
> group knowledge.

Jonas and jst are already CC'd.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It sounds to me like what we mostly need is a way to associate a message with a particular window. I guess we could do that using some sort of parsable message, but it seems much better to have an API that takes an nsIWindow as one of its parameters.

Is there such an API we can move the existing code to?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

s/nsIWindow/nsIDOMWindow/ or something ;)

Comment 23

[David Dahl :ddahl]

(In reply to comment #21)
> It sounds to me like what we mostly need is a way to associate a message with a
> particular window. 

Indeed, I filed a bug for this when we started working on this console: bug 567165

Which was re-purposed as a devtools bug and is being fixed as a work around until the nsIConsoleService can include perhaps the outerWindow's ID as a property of the nsIConsoleMessage

Comment 24

[Kevin Dangoor]

We just had a group meeting and Mihai will take up the challenge of spelunking through the code to identify these kinds of messages. ddahl reports that the fix is straightforward once the messages are tracked down.

Comment 25

[Mihai Sucan [:msucan]]

Attachment: proposed patch

Proposed patch.

Notes:

- This patch only adds the new categories we have to track (or not track). Tests are included for the new categories, where it was possible to write non-failing tests.

- I have included in comments references to class names and methods (or file names) where each category is used in Gecko code.

- for the categories that fail to have their messages displayed, I have included a TODO and a bug report.

- I did not only track those messages coming through nsContentUtils:ReportToConsole(). I did also find the messages that are sent directly through the nsIConsoleService.

- The specific case of:

document.createEvent("Event").preventBubble();

... still fails to show because the nsIScriptError.sourceName cannot be used to properly determine in which Web Console tab to show the message. See bug 603706.

Nonetheless, typical events that are dispatched to a specific element from a specific document have a sourceName URI that can and is used to pick the tab where to show the warning message. (See the included test for the DOM Events category.)

- If you run invalid JavaScript in the location bar, it still fails to show. Same bug as above.

Suggestions for improvements are welcome. Thanks!

(For reference I have filed the following reports: 603706, 603711, 603714, 603720, 603723, 603727, 603730, 603732 and 603750.)

Comment 26

[Mihai Sucan [:msucan]]

Attachment: patch update 2

Updated patch: a new test for the SVG category.

Robert Longson has pointed out in bug 603732 that I missed where nsSVGUtils::ReportToConsole() is called. Uh oh, I don't know how I missed it in the MXR search results. Anyway, fixed!

Comment 27

[David Dahl :ddahl]

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

AWESOME. great documentation in that patch!

Comment 28

[Mihai Sucan [:msucan]]

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

Thanks David for the feedback+! Asking for review now.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

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

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm

>     var hud = this.getHeadsUpDisplay(aMessage.hudId);
>     switch (aMessage.origin) {
>       case "network":
>       case "HUDConsole":
>+      case "WebConsole":

What is this for?

>diff --git a/toolkit/components/console/hudservice/tests/browser/Makefile.in b/toolkit/components/console/hudservice/tests/browser/Makefile.in

>+	browser_webconsole_bug_595934_dom_events.js \
>+	browser_webconsole_bug_595934_css_loader.js \
>+	browser_webconsole_bug_595934_dom_html.js \
>+	browser_webconsole_bug_595934_imagemap.js \
>+	browser_webconsole_bug_595934_html.js \
>+	browser_webconsole_bug_595934_malformed_xml.js \
>+	browser_webconsole_bug_595934_svg.js \

Isn't it possible for these to share the same test infrastructure? They look mostly the same. Seems like this could be a single test that iterates through an array of URLs->expected message mappings.

Comment 30

[Mihai Sucan [:msucan]]

(In reply to comment #29)
> Comment on attachment 482827 [details] [diff] [review]
> patch update 2
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> 
> >     var hud = this.getHeadsUpDisplay(aMessage.hudId);
> >     switch (aMessage.origin) {
> >       case "network":
> >       case "HUDConsole":
> >+      case "WebConsole":
> 
> What is this for?

The patch started from David. He added WebConsole ... not sure if it's really needed. I can remove it.

> >diff --git a/toolkit/components/console/hudservice/tests/browser/Makefile.in b/toolkit/components/console/hudservice/tests/browser/Makefile.in
> 
> >+	browser_webconsole_bug_595934_dom_events.js \
> >+	browser_webconsole_bug_595934_css_loader.js \
> >+	browser_webconsole_bug_595934_dom_html.js \
> >+	browser_webconsole_bug_595934_imagemap.js \
> >+	browser_webconsole_bug_595934_html.js \
> >+	browser_webconsole_bug_595934_malformed_xml.js \
> >+	browser_webconsole_bug_595934_svg.js \
> 
> Isn't it possible for these to share the same test infrastructure? They look
> mostly the same. Seems like this could be a single test that iterates through
> an array of URLs->expected message mappings.

I can merge all tests into one file, but we may obtain once again a big file that when it fails ... will be harder to fix (see network tests). I specifically chose to keep them separate.

If you want me to merge the tests, please let me know. Thanks!

Comment 31

[Mihai Sucan [:msucan]]

Attachment: patch update 3

Updated patch based on Gavin's comment above. Merged all JS tests into one. Please let me know if I have to make other changes as well.

Thanks!

Comment 32

[:Gavin Sharp [email: [e-mail]]]

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

>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_595934_message_categories.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_595934_message_categories.js

>+let TestObserver = {

>+  observe: function test_observe(aSubject)
>+  {
>+    if (aSubject instanceof Ci.nsIScriptError &&
>+        aSubject.category == TESTS[pos].category) {
>+      executeSoon(performTest);
>+    }

Maybe you should print out aSubject.category using info() if it doesn't match what was expected, to help debugging in the failure case? Or is that too spammy?

Comment 33

[Mihai Sucan [:msucan]]

Attachment: [checked-in] patch update 4

Updated the patch as suggested. The test will now properly fail when the category is wrong.

Thanks for your review+ Gavin!

Comment 34

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

Comment on attachment 484686 [details] [diff] [review]
[checked-in] patch update 4

http://hg.mozilla.org/mozilla-central/rev/76cd9b4d3034

Comment 35

[Nochum Sossonko [:Natch]]

I'm tagging this with the dev-doc-needed keyword so that all the script error categories can be documented instead of the cheap documentation available in the idl here: http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/idl/nsIScriptError.idl#76

Comment 36

[Eric Shepherd [:sheppy]]

How much detail do we need to go into about what the categories are? For now, there's just a dump of them now here:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIScriptError#Categories

Comment 37

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

I wouldn't write a page-worth for each category, certainly. Probably a couple of lines will be sufficient for each of them.

Just having them on the page is a great start. Thanks!

Comment 38

[Nochum Sossonko [:Natch]]

I'd say noting the source of the categories (either by actually naming the functions that use them or just by naming the part of code, eg css, xpconnect, etc.) would be awesome.

Comment 39

[Eric Shepherd [:sheppy]]

Added bug 625142 to cover the need to document the categories.