Stack traces containing an unparsable line should still return the parsed trace without throwing an error

[fresheneesz]

Its inevitable that browsers will come up with new traceline formats for specific circumstances. The stack parser should be able to gracefully fail in those cases - the StackFrame should simply have undefined for all its parts. Example:

CapturedExceptions.CHROME_40Garbo = {
    message: "some message",
    name: "Error",
    stack: "Error\n"+
    "    at Object.StackTrace$$get [as get] (http://localhost:8100/generatedBuild/stackinfo.umd.js:3788:23)\n"+
    "    at 21.module.exports (http://localhost:8100/generatedBuild/stackinfo.umd.js:3923:38)\n"+
    "    at something-totally-unparsable\n"
}

        it('should parse v8 traces with unparsable lines', function () {
            debugger;
            var stackFrames = unit.parse(CapturedExceptions.CHROME_40Garbo);
            expect(stackFrames).toBeTruthy();
            expect(stackFrames.length).toBe(3);
            expect(stackFrames[0]).toMatchStackFrame(['Object.StackTrace$$get', undefined, 'http://localhost:8100/generatedBuild/stackinfo.umd.js', 3788, 23]);
            expect(stackFrames[1]).toMatchStackFrame(['21.module.exports', undefined, "http://localhost:8100/generatedBuild/stackinfo.umd.js", 3923, 38]);
            expect(stackFrames[2]).toMatchStackFrame([undefined, undefined, undefined, undefined, undefined]);
        });

This would allow applications using stacktrace.js to continue working as well as possible if a situation like this comes up.

[JamesMGreene]

:+1:

[oliversalzburg]

I agree that the example stack trace shouldn't result in an Error being thrown. However, I don't agree that the parsed StackFrame should have all fields undefined.

If the "file name" in a frame points to (native), this will be registered as the filename property of the StackFrame. So if something-totally-unparsable is passed into the parser, this must also be the resulting filename.

If that would be acceptable, a simple falsey check on urlLike in ErrorStackParser$$extractLocation should yield useful results.

[fresheneesz]

I agree that if "(native)" is there, it should be put as the filename. But if the line actually can't be parsed, then putting the whole line in as the filename doesn't make sense. What does make sense is that the raw stacktrace line should be accessible in the StackFrame object: https://github.com/stacktracejs/stackframe/issues/2

[oliversalzburg]

@fresheneesz What I was trying to say is, if the input is already somewhat unexpected (like "(native)"), then the parser will try a "best guess" approach. The error that is currently thrown is a result of ErrorStackParser$$extractLocation receiving undefined as an argument. There also might be other possible error cases that I might not be aware of.

What would actually result from unexpected input is hard to predict (for me at least), without sample data.

It seems beneficial to have the unparsed data in the stackframe available regardless.

[oliversalzburg]

@fresheneesz The parser will now stay with the best-guess approach, but the stack frames will contain the original line from the stack trace. I hope you consider this satisfactory.

[fresheneesz]

@oliversalzburg Does that mean that if stacktrace.js can't figure out the line at all, it'll get undefined for line, column, file, etc, but then also have the original stack trace line to access?

[oliversalzburg]

@fresheneesz What do you mean by "it can't figure it out"? It will never not be able to produce a result. If the result is what you expect is a different topic. I could always produce an input that will break the parser and result in a parsed stack that doesn't represent what actually happened.

The parser only interprets the input given to it. There are no strict rules.

So, no, it is highly unlikely that any input will cause the parser to produce a result where are fields are undefined, unless the input is undefined. The source line will always be retained though.

[fresheneesz]

@oliversalzburg Ah, hm, ok. As long as there is no input that could cause an exception to be thrown, then I'm happy enough with it. I do think it would be best, tho, if stacktrace.js could detect and indicate somehow that it doesn't understand the input format. By always mapping the input format to some guess-output, i think its losing some information that is helpful sometimes.

But in any case, the change sounds like a big improvement. Thanks!

[oliversalzburg]

@fresheneesz The problem is, you can't really create strong rules for what format to expect. Just take a :, for example. It could be the delimiter between file name and line number, or it could be the delimiter between host name and port number, or it could be the delimiter after the drive letter on a Windows system. This makes it really hard to define strong expectations about the input.

Also, you can throw error messages that will contain fake trace lines (like throw new Error("message\n at nonexistent (fakefile.js:123)")), which is impossible to detect. So the parser needs to have some flexibility.

At least, this is what I've taken away from the topic so far. I'm open to learn new things :)

Regardless, I agree 100% that the parser shouldn't throw (unhandled) exceptions. If there was a case where that happens, then it should return an undefined result like you suggested.

[fresheneesz]

Glad we agree : ) . I do think, tho, that you can define strong expectations about the input. In fact, that's exactly what I did when i wrote stackinfo (which I like to think motivated some of the overhaul of stacktrace.js). Also, the error message should be able to be cleanly separated from the actual stack trace, at least in modern browsers, right? I know you can in chrome.