1133191 - Display JS stack trace on assertions (in test suite/shell)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Dirkjan Ochtman (:djc)]

I noticed that assertions in the JS tests (e.g. in the ecma_6/shell.js test framework) only show the line number for the inner-most stack frame. In case where this is a specific assertion helper, that's a pretty useless way of reporting failure. It would be nice if a stack trace was presented, so that a more useful report of an assertion failure is presented.

Comment 1

[Tooru Fujisawa [:arai]]

how about showing stack trace for all errors in js shell (maybe in my_ErrorReporter), like python does?
so we don't need to modify each assertion function, and we can get stack trace for other exceptions.

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Display JS stack trace on exception in js shell.

Modified my_ErrorReporter to show stack trace if exception is pending (so, that wasn't a warning).
Here is example output (4 lines start with "Stack:"), it indents stack trace with 2 spaces for readability.


## ecma_6/RegExp/flag-accessors.js: rc = 3, run time = 0.151355
1120169: Implement RegExp.prototype.{global, ignoreCase, multiline, sticky, unicode}
shell.js:874:8 Error: Assertion failed: expected exception SyntaxError, got TypeError: global method called on incompatible undefined
Stack:
  [e-mail]:874:9
  testThrowsGeneric@ecma_6/RegExp/flag-accessors.js:43:5
  @ecma_6/RegExp/flag-accessors.js:23:1
TEST-UNEXPECTED-FAIL | ecma_6/RegExp/flag-accessors.js | (args: "")


These days I always apply this patch, it helps a lot while testing new features :)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=157caf7e8f5d (strtok_r wasn't defined in Windows)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38ffa5b94499 (defined macro to replace strtok_r to strtok_s)

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 8592877 [details] [diff] [review]
Display JS stack trace on exception in js shell.

This seems like a generally good idea, but I'm probably not the one to make this decision.

Comment 4

[Tooru Fujisawa [:arai]]

Providing API to return an array of each stack frame's string representation may be better, than calling strtok after calling BuildStackString.

Comment 5

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Add API to return an array of the string representation of each stack frame.

Added BuildStackArray, which stores each stack frame's string representation to AutoStringVector (=AutoVectorRooter<JSString*>), also, splitted out common code from BuildStackString to ForEachStackFrame.

One question, how the number of GetTag in js/src/jspubtd.h decided?  Is -15 appropriate for JSString*?

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Display JS stack trace on exception in js shell.

Changed BuildStackString to BuildStackArray.  Also, fixed the behavior when exception is thrown from different compartment.  Now output looks like following:

Code:
  newGlobal().eval(`
  function f1() {
    f2();
  }
  f1();
  `);

Output:
  /tmp/a.js line 1 > eval:3:3 ReferenceError: f2 is not defined
  Stack:
    f1@/tmp/a.js line 1 > eval:3:3
    @/tmp/a.js line 1 > eval:5:1
    @/tmp/a.js:1:1

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8666443 [details] [diff] [review]
Part 0: Add API to return an array of the string representation of each stack frame.

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

Hm can't we call ComputeStackString to get the stack trace?

::: js/src/jsapi.h
@@ +218,5 @@
>  
>  typedef AutoVectorRooter<Value> AutoValueVector;
>  typedef AutoVectorRooter<jsid> AutoIdVector;
>  typedef AutoVectorRooter<JSObject*> AutoObjectVector;
> +typedef AutoVectorRooter<JSString*> AutoStringVector;

The Auto*Rooter is deprecated I think, the right way to do this now is...

@@ +223,4 @@
>  
>  using ValueVector = js::TraceableVector<JS::Value>;
>  using IdVector = js::TraceableVector<jsid>;
>  using ScriptVector = js::TraceableVector<JSScript*>;

... to add this here:

using StringVector = js::TraceableVector<JSString*>;

And then use Rooted<StringVector> and Handle<StringVector> :)

::: js/src/vm/SavedStacks.cpp
@@ +802,5 @@
> +
> +class StackStringBuilder : public StackBuilder
> +{
> +  public:
> +    StackStringBuilder(JSContext* cx)

This constructor should be marked `explicit`, also once below.

@@ +807,5 @@
> +      : _empty(true), _sb(cx)
> +    {}
> +
> +    bool
> +    append(JSContext* cx, HandleSavedFrame frame, bool skippedAsync) {

Nit: `bool` should go on the same line as append, same for the other methods in this file.

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8666445 [details] [diff] [review]
Part 1: Display JS stack trace on exception in js shell.

Discussed in IRC.
I'll add indentation argument to BuildStackString, this will make it simple.

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Add indentation variant to JS::BuildStackString.

Added a variant of BuildStackString which receives indent and indentLength arguments.

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Display JS stack trace on exception in js shell.

Changed to call BuildStackString with indent arguments.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8666731 [details] [diff] [review]
Part 0: Add indentation variant to JS::BuildStackString.

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

I like this version a *lot* more than the first iteration :)

Do you think anyone would ever want to use anything other than white space as an indent? If not, it might make sense to only have a `size_t indent = 0` parameter and then do

  for (size_t i = 0; i < indent; i++) {
      if (!sb.append(' '))
          return false;
  }

which would simplify the function for callers a little.

::: js/src/jsapi.h
@@ +5389,5 @@
>   */
>  extern JS_PUBLIC_API(bool)
>  BuildStackString(JSContext* cx, HandleObject stack, MutableHandleString stringp);
>  
> +extern JS_PUBLIC_API(bool)

Nit: should probably document this new variant of BuildStackString with a comment.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8666731 [details] [diff] [review]
Part 0: Add indentation variant to JS::BuildStackString.

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

Nice.

::: js/src/vm/SavedStacks.cpp
@@ +770,5 @@
>                  if (!asyncCause && skippedAsync)
>                      asyncCause.set(cx->names().Async);
>  
>                  js::RootedAtom name(cx, frame->getFunctionDisplayName());
> +                if ((indent && !sb.append(indent, indentLength))

As fitzgen said, having a `size_t indent` argument is a bit simpler. You can then use appendN:

if (!sb.appendN(' ', indent)
    ...

(Or keep the `const char*` and use strlen(indent), but size_t arg seems simpler.)

We can make it a default argument instead of adding a separate function (size_t indent = 0), but then it has to go after the stringp outparam. Whichever you prefer, either way is fine with me.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8666733 [details] [diff] [review]
Part 1: Display JS stack trace on exception in js shell.

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

r=me with comments below addressed.

::: js/src/shell/js.cpp
@@ +5156,5 @@
> +    const char indent[] = "  ";
> +    if (!BuildStackString(cx, stackObj, indent, sizeof(indent) - 1, &stackStr))
> +        return false;
> +
> +    char* stack = JS_EncodeStringToUTF8(cx, stackStr);

You can use UniquePtr so you don't need the JS_free below:

UniquePtr<char[], JS::FreePolicy> stack(JS_EncodeStringToUTF8(cx, stackStr));

@@ +5189,3 @@
>      gGotError = PrintError(cx, gErrFile, message, report, reportWarnings);
> +    if (!exn.isUndefined())
> +        (void) PrintStackTrace(cx, exn);

Can you confirm we don't call this function if we're reporting a warning? Is exn undefined then? Because if PrintError doesn't print the warning, we shouldn't print a stack trace :)

Also, I think it makes sense to add AutoSaveExceptionState inside this if, like above, and call .restore() after the PrintStackTrace call.

Finally, how do you feel about printing a message if we can't get the stack trace? If you think it's unnecessary we can skip it.

    if (!PrintStackTrace(cx, exn))
        fputs("(Unable to print stack trace)\n", gOutFile);

Comment 14

[Tooru Fujisawa [:arai]]

Thank you for feedback and reviewing :)

Adding only |size_t indent| would be better.  I'll make it default argument.

(In reply to Jan de Mooij [:jandem] from comment #13)
> Can you confirm we don't call this function if we're reporting a warning? Is
> exn undefined then? Because if PrintError doesn't print the warning, we
> shouldn't print a stack trace :)

|JS_IsExceptionPending(cx)| returns false for warning, even inside |finally| block when exception is thrown in corresponding |try|.

> Finally, how do you feel about printing a message if we can't get the stack
> trace? If you think it's unnecessary we can skip it.
> 
>     if (!PrintStackTrace(cx, exn))
>         fputs("(Unable to print stack trace)\n", gOutFile);

With current implementation, following code hits that.

code:
  throw 1

output:
  uncaught exception: 1
  (Unable to print stack trace)

Not sure if it's good.  Maybe PrintStackTrace should return true if pending exception is not ErrorObject?  Other failure would worth printing error.

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #14)
> Not sure if it's good.  Maybe PrintStackTrace should return true if pending
> exception is not ErrorObject?  Other failure would worth printing error.

Good point, maybe your patch was right and we shouldn't add the error. Or PrintStackTrace should only return false on OOM (JS_EncodeStringToUTF8 or BuildStackString failure). Either way is fine with me.

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 0: Add indentation variant to JS::BuildStackString. r=jandem

changed to |size_t indent = 0|, and appendN

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Display JS stack trace on exception in js shell. r=jandem

Changed PrintStackTrace to return true when thrown value is not ErrorObject.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73ba50f51ad0

Comment 18

[Jason Orendorff [:jorendorff]]

Comment on attachment 8667248 [details] [diff] [review]
Part 1: Display JS stack trace on exception in js shell. r=jandem

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

I love it. Thank you, arai.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/632e3de508dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/706f10f6c21b

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/632e3de508dd
https://hg.mozilla.org/mozilla-central/rev/706f10f6c21b