feat: improve error report

[gengjiawen]

fix: bellard/quickjs#93

currently for input

var dog = {};
dog.bark = function() {
  throw new Error('woof woof');
};
function makeStack() {
  try {
    dog.bark();
  } catch (error) {
    print(error.stack)
  }
}
makeStack();

Engine behaviour

v8:
Error: woof woof
    at dog.bark (/tmp/3df242e:3:9)
    at makeStack (/tmp/3df242e:7:9)
    at /tmp/3df242e:12:1

quickjs-ng:
    at <anonymous> (/tmp/59832ce:3:13)
    at makeStack (/tmp/59832ce:7:5)
    at <eval> (/tmp/59832ce:12:1)

this PR:

quickjs-ng:
    at bark (/tmp/59832ce:3:13)
    at makeStack (/tmp/59832ce:7:5)
    at <eval> (/tmp/59832ce:12:1)

[bnoordhuis]

1. I don't think JS_SetPropertyInternal2 is the right place for this because then we're adding a property as a side effect of adding another property.

A better place is when constructing the backtrace, maybe? (But we're already calling get_func_name there so why isn't that working?)

2. Going through JS_ToCString and JS_FreeCString is kind of inefficient. It's better to split up get_func_name into two parts, one that looks up the JSValue, and one that turns it into a C string.

[bnoordhuis]

Minor style issues: alignment + star leans right; applies to the closing brace too

Correct me if I'm wrong but you want to set the name if it's not empty, right? Also, get_func_name can return NULL

Suggested change

	const char* name = get_func_name(ctx, val);
	if (strcmp(name, "") == 0) {
	const char *name = get_func_name(ctx, val);
	if (name && *name) {

[gengjiawen]

I don't think JS_SetPropertyInternal2 is the right place for this because then we're adding a property as a side effect of adding another property.

It's anonymous function by definition function() {throw new Error('woof woof');}; It use the object property name as a fallback for easy debugging.

[gengjiawen]

Correct me if I'm wrong but you want to set the name if it's not empty, right?

Yeap. quickjs now already works when dog.bark value is named function

var dog = {};
dog.bark = function c2() {
  throw new Error('woof woof');
};
function makeStack() {
  try {
    dog.bark();
  } catch (error) {
    print(error.stack)
  }
}
makeStack();

You can see it here http://k2.gengjiawen.com:8001/

[bnoordhuis]

Yes, I understand that. I think in general it should be possible to recover that information from the JSStackFrame when the backtrace is built.

(I worked on something similar recently for constructors, although I haven't finished that yet.)

The problem I see with the approach in this PR is that it produces the wrong result in this example, right?

var a = {foo: function() { throw Error("fail") }}
var b = {bar: a.foo}
b.bar() // "foo" in stack trace

edit: cross-posted

[gengjiawen]

Yes, I understand that. I think in general it should be possible to recover that information from the JSStackFrame when the backtrace is built.

Yeap. Is there enough context when construct backtrace since this request the property info ? From what I see looks like quickjs not saving enough context for now.

The problem I see with the approach in this PR is that it produces the wrong result in this example, right?

Personally I think it's acceptable. the function is foo when defined.
v8 for reference

/tmp/6bfa5ab:1: Error: fail
var a = {foo: function() { throw Error("fail") }}
                           ^
Error: fail
    at Object.foo [as bar] (/tmp/6bfa5ab:1:34)
    at /tmp/6bfa5ab:3:3

[gengjiawen]

Also, get_func_name can return NULL

Is this why the segment fault in ASAN ?

[bnoordhuis]

Is there enough context when construct backtrace

The answer is a qualified "yes". I had it sort of working for constructors but in general no. I'll grant that making JS_CallInternal record that information is probably a lot more involved than your current approach.

That said, I still don't think JS_SetPropertyInternal2 is the proper place for that because that makes the .name property magically appear as an observable side effect of another operation.

After thinking about it some more, I think the right place is in the parser. We already parse something like this correctly:

var f = function() {} // f.name === "f"

And also:

var o = {f: function() {}} // o.f.name === "f"

But not:

var o = {}
o.f = function() {} // o.f.name === ""

[gengjiawen]

@bnoordhuis Do you prefer merge this or still planing your original solution ?

[gengjiawen]

@bnoordhuis second thought ?

[bnoordhuis]

No, not really. Mutating the function object as a side effect of setting a property on another object is observable by JS. Observable is bad.

What could maybe work is setting some kind of private property, one that isn't observable by JS. The next question then is: does the extra code regress performance of JS_SetPropertyInternal2? That function is very performance-sensitive.

(JS_SetPropertyInternal2 could stand to gain from a careful performance review anyway. Breaking it up in separate functions for the hot and cold code paths should help improve performance.)