228205 - Redesign nsIConsoleService and related APIs

Status: NEW

(DevTools :: Console, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

The goal here is to solve a bunch of bugs and make the console service easier to
use in the process by making callers not have to deal with intl directly.  The
proposed changes are as follows:

Allow callers to just pass to the console sevice the following information in a
single function call:

     1)  [wstring] Module logging the message
     2)  [wstring] Message ID 
     3)  [array_of(wstring)] Array of message-specific info strings
     4)  [long] Length of said array    
     5)  [wstring] URL of thing triggering the error (eg script)
     6)  [wstring] Actual source text that led to the error
     7)  [unsigned long] Line number
     8)  [unsigned long] Column number
     9)  flags (though maybe we can eliminate this?)
     10) Document object associated with the error (possibly null).

The plan is that the console service will use items 1-4 to construct the actual
error text (item 1 determines the stringbundle to use, item 2 determines the
string in that bundle, then make intl do some work).  This means callers don't
need to depend on intl.

Item 10 will be used in two ways:
  A) Get the document's docshell (if any) and walk up the same-type
     docshell tree till it gets to its root (the content area), then
     will get the url of the document loaded there and associate it with the
     error (this makes reloading and framesets happy while allowing easy
     location of the errors that apply to the "window from which you opened JS
     console").
  B) If item #5 matches the URI of the document in item #10, the document's
     docshell, if it exists, will be QIed to nsIWebPageDescriptor
     and the current descriptor will be associated with the error (make view
     source happy for POST pages).

What the console service will do with all that info is to create an
nsIConsoleMessage (which will look a hell of a lot like what nsIScriptError
looks like right now).  In addition to what nsIScriptError has now, the console
message, at creation time, will remember the timestamp.  This nsIConsoleMessage
will be dispatched to console service listeners, who all currently handle
nsIScriptErrors with missing info anyway, so should be easily changeable to
handle this.

nsIScriptError will probably be eliminated.

In addition to the above, there will be a method on the console service that
takes a nsIConsoleMessage param (created somehow somewhere).  This can be used
by people who somehow want/need to subclass nsIConsoleMessage.

There will no longer be a logStringError message.  Callers will log an error as
above with 0 or null for most of the fields (but non-null module and message id).

A "clear()" method will be added to the console service that clears its current
set of stored messages.  Then we can remove the evil JS console hack.

The only really problematic thing here is item 10 in the argument list. 
Currently the console service is in XPCOM, and it can't depend on things like
nsIDOMDocument (for scriptability), nsIDocument, nsIDocShell, etc.  The proposal
is to leave the IDL where it is (so XPConnect can still use it), make that param
an nsISupports documented to be a document object, and move the implementation
out into embedding/components where it can depend on all this garbage.

brendan, what do you think?

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

>     5)  [wstring] URL of thing triggering the error (eg script)

maybe nsIURI? a |wstring| looses the charset information. oh... this is xpcom...

Comment 2

[Alex Vincent [:WeirdAl]]

bz: Would it be reasonable to add the error's stack property to that list?  See 
bug 125612.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Maybe.... This is about errors in general, not just JS errors.  So most of them
will not have a stack property.

For that matter, is the stack on Error() objects a JS-only thing?  Or can it be
reflected into C++ somehow?  (I suspect the former.)

Comment 4

[Alex Vincent [:WeirdAl]]

:) See bug 229824.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That's still talking about JS-only objects.

Once there is a reasonable API that lets me pass these stack traces around in
C++, we can add them to nsIConsoleService.

Comment 6

[Benjamin Smedberg]

Is there a reason why this interface has to live in XPCOM? It doesn't look like
XPCOM itself uses the interface for anything (though I can imagine the component
manager/loader probably should). If not, can't we push the entire interface+impl
into embedding/components, which is tier_9 and solves all the dependencies.

We're gonna need something like this so that the ffox extension manager can
report meaningful errors in a non-synchronous way... unless somebody else wants
to step up to the plate, I'm gonna hack together a not-quite-perfect solution
very soon.

I also question the need for 6-7-8: can't we just make those part of 2?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Can xpconnect usefully depend on embedding/components?

Comment 8

[Boris Zbarsky [:bzbarsky]]

What do you mean by making 6-7-8 part of 2?  2 is an identifier for the type of
error (data that can be used to look up the error message to report or
whatever).  6-7-8 are information about the circumstances in which the error
occured (for display by the JS console, for example).  I see no relationship...

Comment 9

[Brendan Eich [:brendan]]

> Is there a reason why this interface has to live in XPCOM?

So XPConnect can depend on it, as bz said in comment 0.  No, I don't think we
want XPConnect to depend on later tiers.  We've had bugs already where we've had
to ifdef it bogusly.

Still, XPCOM is only one place we could put it that satisfies all dependencies.
 We could also put it in XPConnect, I suppose, but it ought to be independent of
JS some day (soon, if Python integration for XUL proceeds well).  Doug should
comment on putting an abstracted nsIConsoleService.idl, but no impl, in XPCOM.

There may be another equally low tier we can put the .idl file under -- thoughts?

/be

Comment 10

[cls]

(In reply to comment #9)
> There may be another equally low tier we can put the .idl file under -- thoughts?

'fraid not unless you're going to create a new one.  js & xpcom live in tier_2
and xpconnect is the first thing built after that...in tier_9.

Comment 11

[Brendan Eich [:brendan]]

Ok, how about xpcom/io/nsIConsoleService.idl?  xpcom/io already hosts interfaces
not implemented in XPCOM, but used by XPCOM.  It's legit to have a more
primitive library expose a "callback" or set of callbacks.  Doug, what do you say?

/be

Comment 12

[Doug Turner (:dougt)]

xpcom/io is fine for the nsIConsoleService IDL.  As you mentioned in comment #9,
the implementation should live else where.

Comment 13

[Johnny Stenback (:jst)]

Maybe nsIStackFrame would be useful here? Oh, and for #10, in stead of a
document object, how about just an nsISupports argument just as a general error
context, or what not? All errors may not always be related to a document...

Comment 14

[Boris Zbarsky [:bzbarsky]]

We could make it a general context, but in the end we want to be able to get the
document URI and the docshell from the context thing, whatever it is...

nsIStackFrame would work to encapsulate some of the args if we want that sort of
thing, yes.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #14)
> We could make it a general context, but in the end we want to be able to get the
> document URI and the docshell from the context thing, whatever it is...

then make it an interface requestor and document what people should be prepared
to return?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Getting an interface requestor off a document is tough and a lot of work for
some of the callers.  I'd rather do nsISupports and say what it should be
prepared to QI to.

Comment 17

[Johnny Stenback (:jst)]

Yeah, say that it's either a nsIDOMNode or an nsIInterfaceRequestor, or
something along those lines...

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

Can we also add an nsIStackFrame argument to nsIConsoleService? That way, it
would be possible to show a stack trace for the error leading to the console
message.

Comment 19

[Joe Hewitt]

Fixing this bug would be a huge help to the FireBug extension (https://addons.mozilla.org/extensions/moreinfo.php?id=1843&application=firefox).

The goal of the extension is to show the errors that derive from each window/tab in a separate console that is in a panel at the bottom of the browser window.  Since nsIScriptError does not report the document that the error came from, FireBug's error reporting is not always accurate.  The best I can do is append errors to the console that is currently visible, assuming it is most probable that errors will come from the document you are looking at.

Comment 20

[ark]

I would also vote to fix this bug, I started using the extension mentioned in comment 19, it's very useful, unfortunately this bug causes it to report wrong error count for windows out of focus.

Comment 21

[Boris Zbarsky [:bzbarsky]]

I'm not going to be working on this any time in the near future.  It'd be nice to have for 1.9, but I doubt I'll manage it.  If someone wants to take this and implement per the plan, that would be great.

Comment 22

[Philip Chee]

SeaMonkey::Error Console->Toolkit::Error Console

On trunk we use the toolkit error console.

Comment 23

[John J. Barton]

To re-iterate and amplify Joe's comment 19, a central challenge for dev tools  is the mismatch between how developers think and how the consoleService responds. Almost all development work is related to a window, except perhaps the lowest platform layers that start before the first window. So almost all errors start in some window, get thrown into nsIConsoleService without a window ref, then land in a place where the developer needs to figure out which window the error came from. Firebug works hard to reconstruct the information that the error source already has. In addition to getting it wrong sometimes, this also a significant performance hit since lots of sites are pumping lots of errors via ajax on to pages Firebug users aren't even debugging.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment 22: thanks for taking this out of my bug list.  Was there a reason you did that?

Comment 23: That's what this bug is largely about, yes.

Comment 27

[Steve Roussey (:sroussey)]

Cross referencing bug reports:
http://code.google.com/p/fbug/issues/detail?id=1572

Comment 28

[Sebastian Zartner [:sebo]]

Another cross referencing:
http://code.google.com/p/fbug/issues/detail?id=1213

Comment 29

[Brian Grinstead [:bgrins]]

The Error Console has been removed in favor of the Browser Console (see Bug 1278368), and the component is going to be removed.  If this bug is also relevant in the Browser Console, please reopen and move this into Firefox -> Developer Tools: Console.

Comment 30

[Zack Weinberg (:zwol)]

I am mass-reopening and re-componenting every single one of the Toolkit:Error Console bugs that appear to have been closed without anyone even *glancing* at whether they were relevant to the Browser Console.

If you want to close a whole bunch of old bugs -- FOR ANY REASON -- it is YOUR RESPONSIBILITY to check EVERY SINGLE ONE OF THEM and make sure they are no longer valid.  Do not push that work onto the bug reporters.

(It's okay to close bugs that haven't been touched in years when they don't have enough information for you to figure out whether the problem is still relevant to the current software - the reporter probably isn't coming back to clarify.  But that is the ONLY situation where it is okay.)

(I'm going to have to do this in two steps because of the way the "change several bugs at once" form works.  Apologies for the extra bugspam.)

Comment 31

[Brian Grinstead [:bgrins]]

Setting reminder to ask Boris if this bug should remain opened once he's accepting requests.

Comment 32

[Brian Grinstead [:bgrins]]

Discussed on irc and sounds like this is still a relevant bug, although we might want to revisit the proposed API before making any changes

Comment 33

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 40 votes and 65 CCs.
:nchevobbe, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 34

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.