Closed Bug 1133191 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: djc, Assigned: arai)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
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.
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:
  assertThrowsInstanceOf@shell.js: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)
Attachment #8592877 - Flags: review?(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.
Attachment #8592877 - Flags: review?(luke) → review?(jorendorff)
Providing API to return an array of each stack frame's string representation may be better, than calling strtok after calling BuildStackString.
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*?
Assignee: nobody → arai.unmht
Attachment #8592877 - Attachment is obsolete: true
Attachment #8592877 - Flags: review?(jorendorff)
Attachment #8666443 - Flags: review?(jdemooij)
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
Attachment #8666445 - Flags: review?(jorendorff)
Attachment #8666445 - Flags: review?(jdemooij)
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.
Attachment #8666443 - Flags: review?(jdemooij)
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.
Attachment #8666445 - Attachment is obsolete: true
Attachment #8666445 - Flags: review?(jorendorff)
Attachment #8666445 - Flags: review?(jdemooij)
Added a variant of BuildStackString which receives indent and indentLength arguments.
Attachment #8666443 - Attachment is obsolete: true
Attachment #8666731 - Flags: review?(jdemooij)
Changed to call BuildStackString with indent arguments.
Attachment #8666733 - Flags: review?(jorendorff)
Attachment #8666733 - Flags: review?(jdemooij)
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.
Attachment #8666731 - Flags: feedback+
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.
Attachment #8666731 - Flags: review?(jdemooij) → review+
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);
Attachment #8666733 - Flags: review?(jdemooij) → review+
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.
Flags: needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij)
changed to |size_t indent = 0|, and appendN
Attachment #8666731 - Attachment is obsolete: true
Attachment #8667244 - Flags: review+
Changed PrintStackTrace to return true when thrown value is not ErrorObject.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73ba50f51ad0
Attachment #8666733 - Attachment is obsolete: true
Attachment #8666733 - Flags: review?(jorendorff)
Attachment #8667248 - Flags: review?(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.
Attachment #8667248 - Flags: review?(jorendorff) → review+
Depends on: 1219057
See Also: → 481008
You need to log in before you can comment on or make changes to this bug.