Re: inspector/console/console-uncaught-exception-in-eval.html. Looks like we have one more frame (builtin probably), which we should not show to the end user. I don't think that's an improvement per se. Can we skip those when reporting stacks to blink?

> Can we skip those when reporting stacks to blink?

No. Whether a built-in function is implemented in Javascript or C++ is an implementation detail. For example, currently Array.forEach is implemented in Javascript, and something like

[1, 2].forEach(function() { throw new Error(); })

correctly contains forEach in the stack trace.

However, we may implement forEach in C++ in the future out of security and performance concerns. That does not mean forEach should be hidden in the stack trace. In fact, we are going extra lengths to make built-ins implemented in C++ appear in the stack trace (see V8  issue 4815 ). So in general I do not want to skip those stack frames.

That being said, do we know where exactly this additional "@program" comes from? https://storage.googleapis.com/chromium-layout-test-archives/v8_linux_blink_rel/2784/layout-test-results/inspector/console/console-uncaught-exception-in-eval-pretty-diff.html

That seems somewhat bogus.

CallFrame in js_protocol.json has not optional url, line and column [1]. I think other DevTools clients can be relied on it and in DevTools frontend we show location for each frame. (program) is appeared when we can't resolve location for some reasons (e.g. script was collected).
Probably we can provide location of previous stack frame for builtin? It looks like hack but then we don't need to change protocol.

[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/v8_inspector/js_protocol.json?rcl=0&l=147

I got an impression that @program is the frame from inside the builtin implementation (forEach code in this case). Isn't that the case?

I think the correct stack for end user would be:
@anonymous:    throw new Error()
@outer scope:  [1, 2].forEach(..)
Having a builtin frame from inside of forEach implementation would not help the user. For example, I see "at Object.sin (native)" in V8  issue 4815  - that's not something we'd like to expose in debugger.

Sorry if I'm confused and this is actually the opposite of what's happening.

During offline discussion with @dgozman, we found something similar in blink implementation:
1. Go to webkit.org and run following code in console:
var request = new XMLHttpRequest();
request.onload = () => {console.log(new Error().stack);}
request.open('GET', '/', false); 
request.send(null);
2. Dumped stack trace doesn't contain send method and it looks good here because it's some internal blink function like Math.asin for V8.
Error
    at XMLHttpRequest.request.onload (<anonymous>:2:37)
    at <anonymous>:4:9

We do make the distinction between V8's internal builtins such as Math.asin, and API callbacks installed by blink. The former should be displayed in the stack trace, which is the goal of v8  issue 4815 . For the latter we currently don't have a solution yet. I'm not sure how important that is.

To clarify: we are porting more and more V8 internal built-ins from JavaScript to C++, and in doing so, they disappear from the stack trace, which is a regression. V8  issue 4815  addresses this.

I think I have a clearer picture now.

Consider following snippet:

function f() {
  throw new Error();
}

try {
  Math.sin({valueOf: f});
} catch (e) {
  console.log(e.stack)
}

The stack that V8 formats is

Error
    at Object.f [as valueOf] (<anonymous>:2:11)
    at Object.sin (native)
    at <anonymous>:6:8

Disregard that Math.sin is displayed as Object.sin. That should be fixed.

If we now replace Math.sin by Math.min, which is implemented in C++, we would get

Error
    at Object.f [as valueOf] (<anonymous>:2:11)
    at <anonymous>:6:8

It's not obvious from a developer's perspective why this is the case, and since Math.sin is already implemented in C++ in newer V8 versions, this would cause a regression. v8:4815 addresses this.

In both cases however, if I leave the exception uncaught, devtools prints

Uncaught Error
f @ Script snippet #1:2
(anonymous function) @ Script snippet #1:6

I would argue that the built-in frame should not be omitted. Because:
- It misrepresents the actual stack by omitting a frame
- It omits potentially useful information

Regardless of whether you want to omit the built-in frame or not, the current implementation of omitting frames seems to be flawed. We don't have any additional logic [0] to skip frames when collecting stack trace in V8 aside from preventing cross-context leaks, but DevTools seems to have one (as shown above with Math.sin as example). So I suggest modifying that logic in DevTools, for example to skip every frame that does not have a source location attached.


[0] https://cs.chromium.org/chromium/src/v8/src/isolate.cc?sq=package:chromium&dr&rcl=1467069544&l=702

We did some more digging, and found out where the filtering of built-in function frames happens. But let me take a step back...

When an Error object is created in V8, we capture a "simple" stack trace. We use that to format the Error.stack string.

If DevTools is opened, we capture a "detailed" stack trace in addition. That stack trace is passed to the message listener callback and is used to render the stack trace in the console.

For the simple stack trace, we include all frames except for functions with native script, but not marked as "native" (yes, naming is confusing...). See [0]. Functions that are visible to user script are marked as native. The reason we do filtering this way is because native functions can call helper functions that are just an implementation detail. For example String.prototype.replace may call ExpandReplacement. When it throws, we do want to include String.prototype.replace on the stack trace, but not ExpandReplacement. Both are compiled from native script, but only the former is marked as "native".

For built-ins implemented in C++, there is no such issue. Prior to Jakob's patch, they never showed up in the stack trace to begin with, which is a bug, documented in v8:4815.

For the detailed stack trace, we have a similar check [1] that is supposed to be in sync with [0]. But it's not. It only checks for the script type, and omits frames that have native scripts. That means that built-ins implemented in JS which are intended to show up on the stack trace actually don't, because of this check being out-of-sync. The result is that the detailed stack trace actually show less than the simple stack trace. The problem gets worse since we have a way to reconstruct the detailed stack trace from the simple stack trace for the case when DevTools is opened late. Being out-of-sync means that the stack trace rendered on the console depends on when DevTools was opened.

After Jakob's patch [2], C++ built-ins now do show up. But since the check in [1] does not affect C++ builtins, we now actually do the right thing here, but that comes in conflict with the current expectations in blink layout tests.

If we collect the detailed stack trace by omitting C++ built-ins to preserve the current behavior, tests pass.

But I would argue that we should fix the discrepancy between the detailed and simple stack trace, and change the layout test expectations. I'll create a new issue for this.

Btw the "@program" string from the stack trace in the test failure mentioned above is a fall back substituted by blink, when V8 reports an empty string for function name. 

[0] https://cs.chromium.org/chromium/src/v8/src/isolate.cc?q=IsVisibleInStackTrace&sq=package:chromium&l=322&dr=CSs
[1] https://cs.chromium.org/chromium/src/v8/src/frames.cc?q=IsValidFrame&sq=package:chromium&l=175&dr=CSs
[2] https://codereview.chromium.org/2090723005/

I filed issue 624285 for this.

I modified my builtin exit frame patch to preserve the old behavior of detailed stack traces. Layout tests pass now, closing as invalid. 

We can continue general discussion about stack trace behavior in issue 624285.