752411 - Console logs every message type as [JavaScript Error] or [JavaScript Warning], e.g., for CSS errors

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: General, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: PoC of QIing to nsIScriptError to avoid the bogus [JS foo] prefixes

A long time ago, when Stuart first released ChimericalConsole, I bugged him about this, and he replied "that's just the way it is" (although Firefox managed to do the right thing in its Error Console, somehow).

I looked into it a long time ago, when I didn't know very much, and made no headway.  While cleaning up some old tabs, I found that investigation again, and figured it out, more-or-less.  Stuart was in fact right that "that's just the way it is"; apparently the default nsIConsoleMessage implementation(?) in Gecko (src/xpconnect/src/nsScriptError.cpp) does that to every message, and if you just use the nsIConsoleMessage you get from nsIConsoleListener's Observe, "that's just the way it is" indeed.

However, what Firefox does is QI to nsIScriptError and parse the resulting script error object back into its original parts, then re-assemble and format the message (this also lets them turn off/hide some types based on UI buttons, etc.).

I doubt we're ever going to fix this, and I don't expect Stuart is going to fix ChimericalConsole, but all that investigation effort is for naught if it just stays in my head, so here's a *very* partial patch (just a PoC of QIing and getting one part back out) that shows how we'd fix this bug, in another world.  Maybe it'll help someone out in the future.

Comment 1

[Stuart Morgan]

IIRC I just swizzle logMessage: (which is why it's a whole method just for a single NSLog... I cheated a bit by providing myself a hook ;) ) so if we fix Camino, CC should mostly do the right thing. I could then do a bit of post-processing to colorize differently or something.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Hair-pulling, time-eating fix

(In reply to Stuart Morgan from comment #1)
> IIRC I just swizzle logMessage: (which is why it's a whole method just for a
> single NSLog... I cheated a bit by providing myself a hook ;) ) so if we fix

I thought maybe that's what CC did, but I wasn't sure.

Anyway, I had a few minutes this evening, so putting together the rest of the attributes was easy, just bit of copying and tweaking.  Except, no, OMG, PAIN!

The types that get returned from Get* methods we need to call don't match the IDL/compiled .h/MDN documentation, at least for the string types.  Moreover, nsScriptError.cpp uses different types from the only apparent C++ caller of Get* I found, nsDOMThreadService.cpp.  I suppose if I understood C++ (or had a browser I could build in clang, perhaps), I would have been able to figure out how to convert between the types that error-senders vs. QI-callers need :P

Once I stumbled onto that, my switch statement suddenly compiled and the non-string Get* methods stopped crashing when called.  Sadly, I really should have given up and done something else far, far before I stumbled onto the type-conversion.

But, I didn't, so here's the finished patch.  I tweaked the output format slightly from the old default, putting only the [MessageVendor Severity] in brackets and dropping the "" around the filename/URL (dunno if that's for the better or not; philippe?).  Firefox doesn't log a MessageVendor for a number of vendors/types (and also just calls Exceptions Errors); I don't know that we want to gloss over that distinction by default.  Note that some error vendors use lower-case vendor-names while others use Initial Caps names; not much we can do about Gecko component inconsistency ;-)

I also implemented this as a new "full-info" method that assembles the error string from all* the available attributes and then passes it to the old logMessage; for one thing, that let me continue using ChimericalConsole while working on the patch ;-)  Plus, that just seems like a better way for non-Camino clients to deal with customizing their display, rather than being forced to (re-)parse our final output into its original components.

*There are a couple of attributes that I fetch but don't do anything with; I'm not sure how common they are (I've seen them in Fx error console, but can't find any testcases right now), but I've included them for completeness for third-party console vendors who want to hook in to the new full-info method.  

(Finally, if ChimericalConsole is ever revised, can it please learn to remember its window size and position, and perhaps gain a "mark" function like Console so that I can find my place when scrolling back through the 100s of errors some pages generate?  Pretty please ;-) )

Comment 3

[philippe (part-time)]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #2)

> … I tweaked the output format
> slightly from the old default, putting only the [MessageVendor Severity] in
> brackets and dropping the "" around the filename/URL (dunno if that's for
> the better or not; philippe?).

That is OK with me, your output format is slightly more friendly on the old eyes. All those "" are more clutter than anything

The patch works fine here, I don't think it misses anything.

> (Finally, if ChimericalConsole is ever revised, can it please learn to
> remember its window size and position, and perhaps gain a "mark" function
> like Console so that I can find my place when scrolling back through the
> 100s of errors some pages generate?  Pretty please ;-) )

Oh, yes, + 10 on window size & position. Additional colours are not really necessary; eventually flagging real (js) errors (as opposed to the endless css warnings) could be a bonus (putting [MessageVendor Severity] in red or something

Comment 4

[Stuart Morgan]

Comment on attachment 622310 [details] [diff] [review]
Hair-pulling, time-eating fix

Review of attachment 622310 [details] [diff] [review]:
-----------------------------------------------------------------

Mostly looks good, just some small tweaks.

When this lands, email me with a reminder about this and the other CC requests and I'll spin up a new version.

::: src/application/JSConsole.h
@@ +20,5 @@
> +          category:(NSString*)category
> +          severity:(NSString*)severity
> +              file:(NSString*)filename
> +        lineNumber:(NSString*)lineNumber
> +      columnNumber:(NSString*)columnNumber

Since you get these two as numbers, pass them as numbers (just use 'unsigned int' rather than bringing Gecko types into this file) and format them inside the log method.

::: src/application/JSConsole.mm
@@ +64,5 @@
> +          errorSeverity = @"Error";
> +          break;
> +        case nsIScriptError::warningFlag:
> +          errorSeverity = @"Warning";
> +          break;      

Fix the trailing whitespace here and on some blank lines below.

@@ +203,5 @@
> +  // just the filename, as long as the filename is not null; some vendors of
> +  // nsIScriptErrors pass null for the filename, which gets conveted to a valid
> +  // empty NSString by stringWith_nsAString.
> +  NSString* fileInfo = nil;
> +  if ([filename length] != 0 && [lineNumber length] != 0)

if ([filename length] != 0) {
  NSString* lineInfo = (lineNumber > 0) ? [NSString stringWithFormat:@"%u", lineNumber] : @"";
  NSString* fileInfo = NSString stringWithFormat:@" {file: %@%@}", filename, lineInfo};
}

@@ +210,5 @@
> +    fileInfo = [NSString stringWithFormat:@" {file: %@}", filename];
> +
> +  NSString* messageString = @"";
> +  if (prefix)
> +    messageString = [messageString stringByAppendingString:prefix];

This method would be simpler if you start by declaring messageString, then appending as you go; it eliminates the need for |prefix| and |fileInfo|, and the checks here.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: v1.1, with sr comments addressed

(In reply to Stuart Morgan from comment #4)
> ::: src/application/JSConsole.h
> @@ +20,5 @@
> > +          category:(NSString*)category
> > +          severity:(NSString*)severity
> > +              file:(NSString*)filename
> > +        lineNumber:(NSString*)lineNumber
> > +      columnNumber:(NSString*)columnNumber
> 
> Since you get these two as numbers, pass them as numbers (just use 'unsigned
> int' rather than bringing Gecko types into this file) and format them inside
> the log method.

Since these are now |unsigned int|, is it still safe to pass |nil| to them from the "QI failed" section in nsConsoleListener::Observe (currently line 113 et seq. of the patched file)?  (It doesn't appear to crash, but is it actually safe?)

> @@ +210,5 @@
> > +    fileInfo = [NSString stringWithFormat:@" {file: %@}", filename];
> > +
> > +  NSString* messageString = @"";
> > +  if (prefix)
> > +    messageString = [messageString stringByAppendingString:prefix];
> 
> This method would be simpler if you start by declaring messageString, then
> appending as you go; it eliminates the need for |prefix| and |fileInfo|, and
> the checks here.

I did it like this originally because you can have an error that's essentially just the "message" part, no category, severity, or file/line/column info (in fact, that's the default state of an nsIConsoleMessage; you have to QI to nsIScriptError to send a more detailed message), and appending to nil is bad.

But a more elegant way to work around that is to do

NSString* messageString = prefix ? [prefix stringByAppendingString:message] : message;

right after setting up |prefix|, right?  (At least, it seems to work in my testing.)

FWIW, some useful test URLs are 
* http://modernizr.com/ (CSS, plus NSS CVE warning)
* http://modernizr.github.com/Modernizr/test/index.html?noglobals=true (CSS, XPCOM, content JS, DOM Worker JS, etc.)
* data:application/xhtml+xml,<html> (XML)
* http://www.javatester.org/enabled.html with Java installed and enabled and aoJavaPolicy[1] installed and gDebugLog set to true (vendor-provided nsIConsoleMessage).

[1] http://hg.mozilla.org/users/alqahira_ardisson.org/misc/raw-file/9eb9bf9d752d/aoJavaPolicy.js

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Whitespace + style follow-up

When I went to clean up my whitespace problems, I found the files had some pre-existing issues (plus one EOL comment), so I fixed the rest of them in this follow-up patch.