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

Fixed an off-by-one error for JavaScript processing #5922

Merged
merged 9 commits into from
Aug 23, 2017

Conversation

mitsuhiko
Copy link
Member

@mitsuhiko mitsuhiko commented Aug 20, 2017

This replaces #5666

This fixes two things:

  • the off by one (affects grouping)
  • correctly restores the "previous token" behavior (does not affect grouping)

Backstory

The way we deal with minified stacktraces in javascript right now is that we attempt to resolve the token of the previous frame in the frame where the error happens. The reason for this is that the token where the error is happening is not the function name but the token closest to the source of the error.

Case in point:

function foo() {
  throw new Error('test');
}

function bar() {
  foo();
}

In this case the token closest to the error location is Error in foo but we are actually interested in the function name (foo). The way @mattrobenolt solved this originally is that the token resolved is the token of the previous frame. So in the case above one frame above the closest token is foo so that one is taken.

The problem however is that this currently uses a off by one so for calls like this.foo() not foo is taken but this.

This is incorrect in all stacktraces unless the token to the left is the same token. This depends on the browser version about where the column is pointing (might point to the end of a token and not the start for instance).

There are two issues with this. One is the off by one this PR is solving, the other one is if the code we wrote is actually correct in how it resolves the function name (eg: uses the calling frame). This is not attempting on solving the latter issue.

@mitsuhiko
Copy link
Member Author

This turns out to be significantly more broken than i thought. The entire path that sets the function from the previous frame is no longer working.

@ghost
Copy link

ghost commented Aug 22, 2017

2 Warnings
⚠️ Changes to build requirements
⚠️ You should update CHANGES due to the size of this PR

Generated by 🚫 danger

@mitsuhiko mitsuhiko requested review from mattrobenolt and removed request for HazAT August 22, 2017 00:43
Copy link
Contributor

@mattrobenolt mattrobenolt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanna exclude this example-project folder from the entire python package? Not sure it's worth bundling all of that up in the dist.

@mitsuhiko
Copy link
Member Author

@mattrobenolt i thought we already skip the tests entirely on bundling?

@mattrobenolt
Copy link
Contributor

Hmm, that might be true. https://github.com/getsentry/sentry/blob/master/MANIFEST.in

I also didn't notice that this was within tests. So should be fine.

@mitsuhiko
Copy link
Member Author

@mattrobenolt good to merge?

I want to later make a separate PR that uses a newer version of rust-sourcemap once I land this: getsentry/rust-sourcemap#4

Copy link
Contributor

@mattrobenolt mattrobenolt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok now.

@mitsuhiko mitsuhiko merged commit e2e8530 into master Aug 23, 2017
@mitsuhiko mitsuhiko deleted the feature/js-offbyone branch August 23, 2017 16:48
@HazAT
Copy link
Member

HazAT commented Aug 23, 2017

🎉

@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants