123177 - RFE: Add function stack trace for Error() objects

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Alex Vincent [:WeirdAl]]

Myself, bz, and shaver have been discussing an idea I had, to add a JavaScript
function trace to JavaScript errors.  I'm thinking this would be very useful
information for people attempting to track down bugs in their JavaScript code,
which may not be caused strictly by the function where the error came from.

I wrote up a sample script and posted it to JSENG earlier.  For reference, I'm
reposting it here.

function a() {
   return b()
   }

function b() {
   return c()
   }

function c() {
   return d()
   }

function d() {
   try {
     throw new Error("DA MOW MOW!!!")
     }
   catch (e) {
     return f()
     }
   return true
   }

function f() {
  var k = "arguments.callee"
  var response = {}
  var arr = []
  try {
    while (eval(k + "['caller']")) {
      arr[arr.length] = eval(k + ".caller")
      k += ".caller"
      if (eval("arguments in " + k)) {
        k+=".arguments.callee"
        }
      }
    }
  catch(e) {} // we will have an exception, this is expected.
  response.functionStack = arr
  return response
  }

function test() {
  alert(a().functionStack.join("\n\n"))
  }

test()


This script is a proof-of-concept only, and is highly inefficient.  (It took me
some time to get rid of the strict warnings.)  A native-code solution would be
better by far.

bz has raised a concern about untrusted JavaScript accessing chrome functions,
but believes following the caller chain would be adequate protection against that.

I suggest Error.prototype.functionTrace would be an array of objects, each with
a function, its arguments, and the line number last executed in that function as
properties.  This way we can pinpoint precisely where in each function we moved
to the next function, or to the exception.

Opinions?

Comment 1

[Phil Schwartau]

Reassigning to Kenton; cc'ing Brendan, rginda 

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed extension

The property is named 'stack', and its value is a string that you can split on
';' to get each frame's description, which can be split on ':' to get function
name (or 'anonymous', or 'script') followed by filename followed by line
number.

Comments please.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: better proposed extension

Format is "fun(args):file:line;..."

If no function, fun is empty string; if no args, empty string between (). 
Readers can split on ; to get per-frame substring, and split on : to get
fun(args) followed by file followed by line.

/be

Comment 4

[Robert Ginda]

I like the idea, few comments...

+    for (fp = cx->fp->down; fp; fp = fp->down) {
+        v = (fp->fun && fp->argv) ? fp->argv[-2] : JSVAL_NULL;
+        if (!JSVAL_IS_PRIMITIVE(v) &&
+            checkAccess &&
+            !checkAccess(cx, fp->fun->object, callerid, JSACC_READ, &v)) {
+            JS_free(cx, stackbp);
+            return JS_FALSE;
+        }

Is |checkAccess| a security thing?  If so, should we fail to create the
Exception, or just omit the |stack| property?

+            argstr = JS_ValueToString(cx, fp->argv[i]);

That string may have ";" or ":" in it, making the result unparsable.  Maybe we
should String.prototype.quote() or escape() it.

+        stackbp = JS_sprintf_append(stackbp, "):%s:%u:%s",
+                                    (fp->script && fp->script->filename)
+                                    ? fp->script->filename
+                                    : "stdin",

Perhaps "<none>", or "<null>" is a better default for filename.  There are a
number of script objects in Mozilla that have a NULL filename and didn't come
from stdin.

Comment 5

[Brendan Eich [:brendan]]

Attachment: best yet, can it be better still?

The split-ability goal seems bogus now, in light of all the possible chars in
filenames, never mind in arguments -- it was something I hoped would be easy to
satisfy only before shaver pointed out that function actual arguments would be
helpful in the stack trace (and only if one avoids : in filenames).

Now, readability is king, without wasting too much space on words like "called
from file ... at line ..." (which also require localization).  A \n terminates
each stack frame description (contrast to ; as separator).  A : separates file
and line, as is traditional (ambiguous on Mac as a split char, though -- but
not ambiguous to readers on Macs, I trust).  The @ sign is used to separate
fun(args) from file:line, and args are generated using js_ValueToSource for
best quoting.

While figuring out how to deal with errors and exceptions incurred during stack
formatting (to fix the problem Rob pointed out where checkAccess failure would
stop the script creating a new Error or Exception object, rather than simply
result in a truncated stack trace string), I found some less-than-ideal wording
in jsapi.h, and over-punctuated or otherwise overlong (or just plain wrong --
what was "Mountain View" doing there?!) patterns in jscntxt.c.	Those two files
contain cosmetic fixes only.

Hoping for r= and sr= now, or soon.

/be

Comment 6

[Alex Vincent [:WeirdAl]]

Um, I think a : character would be legal in a query-string. 
http://www.jslab.org/test?name=value1:value2:value3

So might almost any other character following a URI.  Spaces are a notable
exception.  Spaces have to be encoded.

If we're not able to safely determine characters for separators, would it be
harder to return an array of objects, as I originally suggested?

Comment 7

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

Even with colons in the filename, as you are pretty much guaranteed to have with
URLs, you're fine: everything after the last colon is lineno, and the rest is
the filename (excluding that final colon).  Regular expressions,
String.prototype.lastIndexOf, strrchr(3): take your pick.

(I thought literal newlines were legal in javascript: and perhaps data: URLs, if
perhaps not as attribute values in HTML.)

I think brendan is remiss in not demonstrating his work, so I'll do that for him.

This is "script:file.js":

function f(g, a) { 
    g("75", a);
}

function e(prop) {
    f(error, Math[prop]);
}

function error(a1, a2) {
    var x = a1 + a2;
    throw Error();
}

try {
    e("PI");
} catch (ex if ex instanceof Error) {
    print("stack: ", ex.stack);
}

: tip; Linux_All_DBG.OBJ/js -f script:file.js 
stack:  error("75",3.141592653589793)@script:file.js:11
f(function error(a1, a2) {var x = a1 + a2;throw
Error();},3.141592653589793)@script:file.js:2
e("PI")@script:file.js:6
()@script:file.js:15

The line numbers match up, and everything.  No locals, but really, this isn't
meant to supplant debuggers.

Comment 8

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

Comment on attachment 67643 [details] [diff] [review]
best yet, can it be better still?

Move the declarations of v and argsrc down to the beginning of their respective
for-blocks?

Locality is next to godliness, or something, though I guess prevailing style is
sort of
otherwise.

sr=shaver.

Comment 9

[Alex Vincent [:WeirdAl]]

For documentation and reuse purposes, I'll write a script to read and parse this
string once I have a build with the patch enabled, and I'll attach it to this
bug.  We'll need that anyway for other utilities to use this (such as JS Console)

Comment 10

[Robert Ginda]

Comment on attachment 67643 [details] [diff] [review]
best yet, can it be better still?

+ * so callers must stop using the pointer returned from Save after calling the
+ * appropriate Release or Drop API.

appropriate Restore or Drop API?

r=rginda

Comment 11

[Phil Schwartau]

I second Alex's request that the value of this property be an array,
not a delimited string. The latter may be appropriate to a low-level 
language where storage concerns are paramount, but the former seems
more in keeping with a high-level language like JavaScript.


1. Example: the return value of a regexp with backreferences, say          
   /(a)(b)(c)/("abc"), is an array, not a delimited string. 


2. How would it look in a textbook on JavaScript?

  "You capture the call stack like this:"

  var str = err.stack;
  var arr = str.split(some_delimiter);

  What delimiter? Oh, you have to memorize that. And every time
  you use this property you'll have to look this up.

OR:

  var arr = err.stack;


3. The developer will most likely have need of the frames of the
   call stack, not the concatenation of all the frames together.
  

4. Finally, the array approach is more in keeping with what many
   JavaScript programmers currently do do capture call stacks:

   function getCallStack(f)
   {
     var arr = [];   
     while (f)
     {
       arr.push(f);
       f = f.caller;
     }
     return arr;
  }

Comment 12

[Brendan Eich [:brendan]]

A JS backtrace function can make an array easily, but should the engine allocate
an object and a bunch of strings from code that is creating an exception object,
possibly under duress?  Especially in embeddings that run into low-memory
conditions normally?

/be

Comment 13

[Phil Schwartau]

Oops, I didn't think of that!

Comment 14

[Alex Vincent [:WeirdAl]]

As I said, I'll write a function to parse the string appropriately once I have 
a build with that property.  Unfortunately, my Internet time has suddenly 
become very very short, and it will be a little while before I'm able to 
download the build.

How about this as an idea, however:  a parseStack() method of Error.prototype, 
to return an array as I suggested?  (It would need its own toString(), I 
suspect, though.)

I note there are only a couple minor suggestions from r= and sr=, and that the 
tree is open.  Might we have this checked in right soon?

Comment 15

[Brendan Eich [:brendan]]

I declare victory.  I'm not going to bloat compiled code footprint (or source
code brainprint for that matter) with a gratuitous native method to parse the
stack string, when it's easy enough to write in JS.  If parsing could be made
easier by a better format, please open a new bug on that.  I still think the
first goal is concise readability, but admit that I err on the side of concise
in general.  But I've checked in the fix for this bug, so at least we have a
stack property of exception objects now.  Thanks for the prodding to do this, Alex!

/be

Comment 16

[Brendan Eich [:brendan]]

Argh, mccabe didn't make the internal js_ErrorToException code construct the
Exception object, so try { foo.bar } catch (e) { print(e.stack) } where foo is
not defined will not get a predefined stack property.  Only explicitly new'ed
Exception, Error, etc. classes.  Patch to construct coming up.

/be

Comment 17

[Brendan Eich [:brendan]]

I'll fix.

/be

Comment 18

[Aleksey Nogin]

This checkin added 2 "may be used uninitialized" warnings:

js/src/jsexn.c:329
 `older' might be used uninitialized in this function

js/src/jsexn.c:330
 `state' might be used uninitialized in this function

Even though this is the case of compiler not being smart enough, it would still
be nice not to have those warnings so that real bugs are not missed in the pile
of warnings.

Comment 19

[Brendan Eich [:brendan]]

Attachment: proposed followup fix

All exception objects are constructed now, so they get the stack property.  I
also finally extended the API to provide JS_ConstructObjectWithArguments, which
lots of people have requested, and which is a trivial layer on top of the
needed js_ConstructObject extension (to take argc/argv) used by jsexn.c.

Looking for review for 0.9.9.

/be

Comment 20

[Robert Ginda]

Comment on attachment 68739 [details] [diff] [review]
proposed followup fix

r=rginda

Comment 21

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

Comment on attachment 68739 [details] [diff] [review]
proposed followup fix

I like the __GNUC__ bit, pushing the cost of the unnecessary assignments onto
the loserly GNU compilers.  sr=shaver.

Comment 22

[Brendan Eich [:brendan]]

I'll check in today.

Phil, there's a test in the suite that wrongly expects 4 to be the value of
Error.length (e.g., SyntaxError.length, etc. too), because the stack property is
enumerable.  I think that test should just stipulate the number of arguments it
that expects length to reflect, instead of trying to count with a for..in loop.

/be

Comment 23

[Brendan Eich [:brendan]]

Fix is in.

/be

Comment 24

[Alex Vincent [:WeirdAl]]

FYI:  I've filed bug 125612 for adding the function stack trace to the JS 
Console.

Comment 25

[Phil Schwartau]

Attachment: Sample testcase. All errors generated within parent file.

Comment 26

[Phil Schwartau]

The testcase is for the browser, but may be run in the JS shell by
simply excising the <SCRIPT> tags; no further changes are necessary.

The test generates a SyntaxError, a ReferenceError, and a TypeError.
Note the first produces a stack with one more frame than the other two.
This is because I used eval() to generate the error, making one more
function call for the stack to record -

Comment 27

[Phil Schwartau]

Verified FIXED using the latest WinNT Mozilla binary and JS shell.

I have tested that the stack property is accurate for the above testcase.
I have also tested a variation producing errors from an external JS file;
and again the stacks were accurate. I also tested in the JS shell - 

I really like the use of '\n' as the delimiter; I find the display
quite clear and intuitive -

Comment 28

[Phil Schwartau]

The testcase Brendan mentioned in Comment #22 has now been repaired:

        mozilla/js/tests/js1_5/Exceptions/regress-123002.js

Comment 29

[Alex Vincent [:WeirdAl]]

FYI, I've filed bug 229824 for adding a similar stack trace to DOM exceptions.  Anyone 
willing to translate Brendan's C code into C++ to get the same effect?  (I'm guessing 
DOMException.cpp is the right place.) 

Comment 30

[Brendan Eich [:brendan]]

See bug 229824 comment 1.

/be