Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix function names in javascript stacktraces #5666

Closed
wants to merge 2 commits into from

Conversation

HazAT
Copy link
Member

@HazAT HazAT commented Jun 30, 2017

I've noticed weird function names for stracktraces in javascript. So @mitsuhiko and myself investigated the issue.

We found out that we should substract 1 not only from the line number but also from the column. Since lineno and col start at 1 and in libsourcemap we start to count at 0, we have to substract both by one to get the correct function name again.

I know this is a major change, since this means all the groups will change (tested it). Probably we will have to discuss how we gonna tackle this problem.

Here are two screenshots indicating the change:
Current status:
wrong

Fix:
right

@mitsuhiko
Copy link
Member

I want to quickly leave some extra comments here about the story with the function names in particular. One of the issues we currently have is the mentioned incorrect offset. The second however is that our assumption of how to recover the function name is a bit naive and I wonder if we could do better.

Since sourcemaps only tell us the named token closest to the error there is no clear way to get to the contained function. The path that @mattrobenolt chose originally is that we assume that the calling frame will have the approximated function name at frame's location. This means that the token that describes the function will be the name of the function.

This illustrates this:

function onClick(...) { ... }
function onTap(...) { throw new Error('...') }
let func = onClick;
if (isMobile) func = onTap;
func();

If now onTap fails the function name reported in sentry is func in the best case and not onTap. Reason being that the name reported in the function is actually Error, so we go up one frame to find func() where the reported name is func.

I looked at what some other tools are doing. Chrome does not resolve function names at all in stracktraces it seems. stacktrace-js' stacktrace-gps project parses the sourcecode backwards to locate the closest function or closure in an assignment (https://github.com/stacktracejs/stacktrace-gps/blob/64ca313dca7371bc75258cd65da40cb2e39d1535/stacktrace-gps.js#L65-Lundefined)

Now obviously parsing backwards has some problems. One is that it's potentially very slow and buggy, secondly it means that we need to have language specific parsers (eg: what if someone uses coffeescript or "super cool custom language").

I'm raising it though because since fixing this bug in the PR will mean regrouping for some people (where we got the line wrong) so we might get some value in tackling the core of the issue better.

@mitsuhiko mitsuhiko requested a review from mattrobenolt June 30, 2017 12:37
@mitsuhiko
Copy link
Member

FWIW not necessarily all groups will change. The groups will change for cases like the expanded frame where the wrong column changed the source line. In particular this happens with multiline function calls. The grouping will use the source line content, not the function name. So if we in theory never cross over to a different line the change will not change the group.

In other words:

This will not regroup:

foo(bar, baz)

But this will:

foo(
    bar,
    baz
)

@mitsuhiko
Copy link
Member

Closing in favour of #5922

@mitsuhiko mitsuhiko closed this Aug 20, 2017
@mitsuhiko mitsuhiko deleted the bugfix/js-sourcemap-fix branch August 20, 2017 13:44
@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants