493414 - nsIScriptError could support nsIStackFrame and things could populate it

Status: RESOLVED WORKSFORME

(Core :: XPConnect, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1 prototype patch that implements nsIScriptErrorEx and gets data to it

The error console and other listeners have no means of getting at the call stacks of exceptions that show up on its doorstop in the form of nsIScriptError.  The attached patch attempts to provide a prototype implementation that addresses this.

I have writ an extension that exposes the information via dump():
http://hg.mozilla.org/users/bugmail_asutherland.org/exmmad/

I have writ a blog post about the patch, which can be found here:
http://www.visophyte.org/blog/?p=307

I also paste the relevant discussion of the prototype patch from the blog herein:

Currently (pre-patch), there are basically 3 ways scripting errors can show up in the platform:

   1. nsIScriptError instances.  These are what show up on the error console.  These have information equivalent to a single stack frame.
   2. nsIException instances.  These can provide a stack in the form of an nsIStackFrame chain (the same thing Components.stack gives you).  These get converted into nsIScriptError instances when it comes time to report them to the error console.  From a stack perspective, only XPConnect produces nsIException instances with stack traces, although you can make your own via Components.Exception.  A fundamental limitation of these stack traces is that they are only constructed from live JS call stacks, so if a JS exception has unwrapped its way to the top-level you are out of luck.
   3. JavaScript Error instances.  These have a private super-rich (it even knows arguments!) call-stack that can only be exposed as a string via the non-standard stack attribute.  XPConnect understands JS error reports (the ‘flat’ mechanism by which SpiderMonkey reports errors/exceptions to C++ code), but it has no clue about exceptions and their Error form of existence.  The exceptions in their error report guise are converted into nsIScriptError instances.

What the patch does is:

    * Introduce an nsIScriptErrorEx interface that extends nsIScriptError to provide a ‘location’ attribute like nsIException which is an nsIStackFrame.
    * Modify nsScriptError to implement the extended nsIScriptErrorEx.  Alternatively, I could have made XPConnect’s nsXPCException class implement nsIScriptError or nsScriptError also implement nsIException or something like that and not introduced nsIScriptErrorEx at all.
    * Modify all nsIScriptError-creation sites that I care about (I’m not looking at you, DOM workers) to try and provide or propagate existing nsIStackFrame information.
    * If a JS stack frame is not available, but an exception is in the form of a JS error, suck the call stack out of it.  Theoretically, this should not be a fallback but rather the default case, but it depends on some JS/XPConnect implementation details I am trying to avoid finding out about for now.
    * Modify the JS API to provide call stack sucking functionality.
    * Does various sketchy things to expose XPCJSStack::CreateStack from XPConnect to the error reporters in other modules.  If you thought the choice of creating nsIScriptErrorEx was sketchy, welcome to the Downtown East Side of dubious patches.  I expect there is no chance of it working on Windows because of this, and you may be out of luck on OS X.

Comment 1

[Nochum Sossonko [:Natch]]

Cc'ing some folks that may be interested in this.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v2 prototype patch that implements nsIScriptErrorEx and gets data to it

I had missed XPCConvert::JSErrorToXPCException which was creating an nsScriptError outright.  I modified it to take a location, and modified XPCConvert::JSValToXPCException to call a newly-exposed XPCJSStack::CreateStackFromError, since it has access to the JS exception object.

Then I got into a situation where globalOverlay.js:goUpdateCommand triggered by a menu item (so XUL-ish event) called a C++ XPCOM controller in turn called JS code that threw an exception...

When the inner JS code (the one called by XPConnect would throw an exception), nsXPCWrappedJSClass::CheckForException would defer to nsJSEnvironment.cpp's NS_ScriptErrorReporter because cx->errorReporter != xpcWrappedJSErrorReporter.  This would disable nsXPCWrappedJSClass::CheckForException's logging because of the "reportable = !JS_ReportPendingException(cx)" branch and the fact that JS_ReportPendingException only fails if it can't generate a report; it doesn't care what NS_ScriptErrorReporter does, which in that case, is nothing because it finds a non-native frame.

Then XPConnect ends up throwing an nsIXPCException/nsIException.  This gets propagated up through the JS, and we end up in js_ReportUncaughtException.  That dude was only setting the exception on the context so that the error reporter could see it if the exception was of js_ErrorClass.  So I changed it to set the exception via JS_SetPendingException in both cases.

With that change, NS_ScriptErrorReporter could see the nsIException.  I modified the XPCJSStack::CreateStack logic I had added in the prior patch to also steal the stack (via location) via nsIException in addition to the JS Error object.  Arguably it should steal more/support some concept of inner exception.  What you get now is a very complex string description that is basically gibberish and the stack-trace that you would have gotten if XPConnect had logged its error instead of giving up.

Oh, but that does assume that you modify globalOverlay.js to not have a try/catch that basically swallows my new exciting errors.

Comment 3

[timeless]

Comment on attachment 377900 [details] [diff] [review]
v2 prototype patch that implements nsIScriptErrorEx and gets data to it

1. We typically use nsIFoo2 instead of nsIFooEx
2. We'd be more likely to do nsIFoo2.initWithStack instead of nsIFoo2.initEx

>-  // Note: we must do this before running any more code on cx (if cx is the
>-  // dynamic script context).
>-  ::JS_ClearPendingException(cx);

This move scares me, I need to see more context (i'll look when I stop traveling)

>+  // Note: we must do this before running any more code on cx (if cx is the
>+  // dynamic script context).
>+  ::JS_ClearPendingException(cx);

>+    for (endElem = elem + priv->stackDepth; elem != endElem; elem++) {

I think js prefers ++elem, but I'm not sure.

>+extern JS_EXPORT_API(void *)
>+js_WalkErrorStack(JSContext *cx, JSObject *obj, JSStackFrameProcessor procFunc,
>+                  void *alwaysData, void *initialData);

JS_EXPORT_API should really only be available on JS_ public functions, at least in general.

js_ is for private stuff.

>diff --git a/js/src/xpconnect/public/xpcjsstack.h b/js/src/xpconnect/public/xpcjsstack.h
>new file mode 100644
>--- /dev/null
>+++ b/js/src/xpconnect/public/xpcjsstack.h
>@@ -0,0 +1,77 @@
>+ * The Original Code is Mozilla Communicator client code, released
>+ * March 31, 1998.

This seems odd. 

>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 1998
>+ * the Initial Developer. All Rights Reserved.

This seems odd.

>+ *
>+ * Contributor(s):
>+ *   John Bandhauer <[e-mail]> (original author)
>+ *   Mike Shaver <[e-mail]>
>+ *   Mark Hammond <[e-mail]>

>+ * This file exists to expose XPCJSStack to error reporters so they can build
>+ * stacks directly from JSContexts.

>+//#pragma GCC visibility push(default)

^ this seems odd


>+nsScriptError::GetLocation(nsIStackFrame * *aLocation)

>+    if(!aLocation)
>+        return NS_ERROR_NULL_POINTER;

NS_ENSURE_ARG(aLocation);


>     JSStackFrame *fp = NULL;
>+    // if the stack itself lacks a frame, see if there's an exception
>+    // that might have an exciting stack for us.
>     if (!JS_FrameIterator(cx, &fp))
>+    {


>+        if (cx->exception && JSVAL_IS_OBJECT(cx->exception))
>+            return XPCJSStack::CreateStackFromError(
>+                       cx, JSVAL_TO_OBJECT(cx->exception), stack);

no need for |else|, and given the simplicity, it's best to reorder the logic.
>+        else
>+            return NS_ERROR_FAILURE;

if (!cx->exception || !JSVAL_IS_OBJECT(cx->exception))
  return NS_ERROR_FAILURE;

return XPCJSStack::CreateStackFromError(cx, JSVAL_TO_OBJECT(cx->exception), stack);

>+    }
>+    return XPCJSStackFrame::CreateStack(cx, fp, (XPCJSStackFrame**) stack);
>+}

While you're at it, since the if !JS_FrameIterator case is now the complicated case, please rewrite the outer if as:

if (JS_FrameIterator(cx, &fp))
  return XPCJSStackFrame::CreateStack(cx, fp, (XPCJSStackFrame**) stack);


>diff --git a/js/src/xpconnect/idl/nsIScriptError.idl b/js/src/xpconnect/idl/nsIScriptError.idl
>--- a/js/src/xpconnect/idl/nsIScriptError.idl
>+++ b/js/src/xpconnect/idl/nsIScriptError.idl
>@@ -43,6 +43,8 @@
> #include "nsISupports.idl"
> #include "nsIConsoleMessage.idl"
> 
>+interface nsIStackFrame;
>+
> [scriptable, uuid(b0196fc7-1913-441a-882a-453c0d8b89b8)]
> interface nsIScriptError : nsIConsoleMessage
> {
>@@ -92,6 +94,21 @@
>     AUTF8String toString();
> };
> 
>+[scriptable, uuid(2e8a7f20-af9a-4420-881d-3e87465e0264)]
>+interface nsIScriptErrorEx : nsIScriptError
>+{
>+    readonly attribute nsIStackFrame location;
>+
>+    void initEx(in wstring message,
>+                in wstring sourceName,
>+                in wstring sourceLine,
>+                in PRUint32 lineNumber,
>+                in PRUint32 columnNumber,
>+                in PRUint32 flags,
>+                in string category,
>+                in nsIStackFrame location);
>+};

This should be its own file. (nsIScriptError2.idl containing interface nsIScriptError2 with initWithStack)

What happens if the exception is infinite recursion?

And do you promise that the nsIStackFrame that's passed is kept around forever?

Because most things in gecko aren't threadsafe, but the object that implements nsIScriptError is, it's likely that someone might create an nsIStackFrame try to give it to nsIScriptError2 and be surprised if it's called from the wrong thread.

Also, someone might decide to pass Components.stack as nsIStackFrame, i.e. they /probably/ expect you to eagerly collect the stack information and retain your own copy instead of holding their pointer.

Oh, of course js patches should really be distinct from xpconnect patches, because the people who own the two things are distinct, they have different components, etc.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Andrew, I will assign this bug to you since you are working on this patch.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

timeless, thank you for the review pass.

Does anyone have any high level input they would like to make?  For example, just pass around nsIException instead of nsIScriptError?  I probably need to perform a case-based analysis of the situations and general error flow, but I would be surprised if others more involved with this code haven't already thought about this...

Comment 6

[Benjamin Smedberg]

I definitely think we ought to be storing (copying out) stack frame *data*, not live exception objects with references to actual JS stack frames.

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Adding Rob C for firebug perspective; I think he'll probably like it!

Comment 8

[John J. Barton]

Perhaps this is out of scope, but nsIStackFrame is much less useful and jsdIStackFrame. The info in nsIStackFrame is incomplete and incorrect. The info in jsdIStackFrame is complete and incorrect, but Firebug knows how to correct it using the complete info. Specifically Firebug stores a map of the jsdIScripts that appear in the jsdIStackFrame.

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

XPCJSStack only knows how to copy out stack frame data from JS frames; it does not hold onto the JS stack frame and is an immutable data structure (apart from the requisite XPCOM reference counting).

I'm not aware of any other nsIStackFrame implementation in the tree, so I believe we're already safe from Components.stack or other lifetime issues.


jsdIStackFrame, on the other hand, appears to reflect the synchronous nature of the debugging API and I presume is not intended to out-live the debug hook's invocation.

John J. Barton, did you have specific input on how we can improve the use case of good error reporting when a debugger is not active?  Right now we are throwing away or losing very useful stack information, which is my primary concern.  jsdIStackFrame is a treasure trove, but arguably there is a point where people should just fire up firebug/chromebug/venkman, so I think there is still a case for nsIStackFrame (or something like it).

Comment 10

[John J. Barton]

(In reply to comment #9)
... 
> 
> jsdIStackFrame, on the other hand, appears to reflect the synchronous nature of
> the debugging API and I presume is not intended to out-live the debug hook's
> invocation.

But the data structure does contain the arguments, vital information for one of the most common error problems in Gecko: invalid or unexpected JS values passed to C++ then reported via erroneous or cryptic error codes. 

(Really there is no point in pursuing jsdIStackFrame if the use case is no debugger: turning on jsd is not cost effective in that use case).

> 
> John J. Barton, did you have specific input on how we can improve the use case
> of good error reporting when a debugger is not active?  Right now we are
> throwing away or losing very useful stack information, which is my primary
> concern. 

I 100% support your work and wish you luck in pushing it through. I am not as familiar with the problem of error reporting with out a debugger active. 

The number one way to improve error reporting debugger or not is to correctly identify the calling window. The call stack is an indirect way of helping, but simply giving the window would be simpler.

The number two way is to correct the file and line number information. This would help all error messages and all call stacks. Its only number two because its actually many sites and issues.

But these are all birds-in-the-bush ideas: go for it.

Comment 11

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

+1 for this, thanks for the CC, shaver.

Based on discussions with John B. in the past, an easy way to identify the source window would be a big help for us (and any other consumers of nsIScriptError).

Comment 12

[Steve Roussey (:sroussey)]

What is the current status? I'm working on some stuff trying to get the right errors to display in the right browser tab where firebug is embedded.

Comment 13

[John J. Barton]

I had one other idea on the number one issue for error reporting (no window): add a few events to mark start/stop-parse-html, start/stop-parse-css, start/stop-parse-js and pass the window in the event. Firebug can hold the current value and route errors accordingly. That would be much easier than bug 228205.

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #12)
> What is the current status? I'm working on some stuff trying to get the right
> errors to display in the right browser tab where firebug is embedded.

It was probably slightly misleading to have this assigned to me; I haven't been working this.  This was more of a hail mary where I hoped the platform team might take this and run with it; Thunderbird engineering doesn't really have the resources to address fundamental platform limitations in a way that will be accepted into the tree.

Comment 15

[Alexandre Poirot [:ochameau]]

Andrew, I think I more or less did that via bug 814497 and bug 1184172,
so I imagine we can close this bug now?

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Alexandre Poirot [:ochameau] from comment #15)
> Andrew, I think I more or less did that via bug 814497 and bug 1184172,
> so I imagine we can close this bug now?

Woooooooo!