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

Highlighted line is off by one line on non existing property #3239

Closed
5 of 11 tasks
skinsapp opened this issue Aug 12, 2023 · 9 comments · Fixed by #3283
Closed
5 of 11 tasks

Highlighted line is off by one line on non existing property #3239

skinsapp opened this issue Aug 12, 2023 · 9 comments · Fixed by #3283

Comments

@skinsapp
Copy link

skinsapp commented Aug 12, 2023

OS:

  • Windows
  • MacOS
  • Linux

Platform:

  • iOS
  • Android

SDK:

  • @sentry/react-native (>= 1.0.0)
  • react-native-sentry (<= 0.43.2)

SDK version: 0.0.0

react-native version: 0.0.0

Are you using Expo?

  • Yes
  • No

Are you using sentry.io or on-premise?

  • sentry.io (SaaS)
  • on-premise

If you are using sentry.io, please post a link to your issue so we can take a look:

[Link to issue]

Configuration:

(@sentry/react-native)

const bundleId = Platform.OS === 'ios' ? config.bundleIdIos : config.bundleIdAndroid

const { appVersion, label } = await codePush.getUpdateMetadata()
const release = `${bundleId}@${appVersion}+codepush:${label}` // eg: [email protected]+codepush:v68

Sentry.init({
    dsn: "https://518d6f8ce5f44c12a464be5ff5e614be@o4505131309203456.ingest.sentry.io/4505131310841856",
    tracesSampleRate: .1,
    release,
    dist: label
});

I have following issue:

The sourcemap is one line off. In the following screenshot, line 43 should be highlighted, not line 42:

image

The android version of my app does not have this issue.

Steps to reproduce:

I'm using these scripts to generate the source maps:

{
 "ios:deploy": "npm run appcenter:login && npm run ios:codepush:release && npm run ios:sentry",
 "appcenter:login": "appcenter login --token foobar",
 
 "ios:getNativeVersion": "sed -n '/MARKETING_VERSION/{s/MARKETING_VERSION = //;s/;//;s/^[[:space:]]*//;p;q;}' ./ios/skins.xcodeproj/project.pbxproj",
 "ios:getCodePushVersion": "appcenter codepush deployment list --app foo/bar | sed -n -e 's/^\\(.*\\)\\(Label: \\)\\(v[0-9]*\\) \\(.*\\)$/\\3/p'",

 "ios:codepush:release": "rm -rf sentry && appcenter codepush release-react --extra-bundler-option=--reset-cache --extra-bundler-option=--minify --extra-bundler-option false -m -a foo/bar -d Production --sourcemap-output './sentry/CodePush/main.jsbundle.map' --output-dir './sentry'",

 "ios:sentry": "export SENTRY_PROPERTIES=./ios/sentry.properties; v=$(npm run --silent ios:getNativeVersion) && c=$(npm run --silent ios:getCodePushVersion) && sentry-cli react-native appcenter foo/bar ios './sentry/CodePush' --bundle-id 'com.foo.bar' --version-name $v --dist $c --deployment Production",
}

Actual result:

highlighted line is 42

Expected result:

highlighted line is 43

@kahest
Copy link
Member

kahest commented Aug 16, 2023

Hey @the-skins-app, thanks for reporting and the details - we'll investigate. Please let us know if you see something similar in other parts of your code.

@getsantry getsantry bot removed the status in GitHub Issues with 👀 Aug 16, 2023
@kahest kahest moved this from Needs Discussion to Needs Investigation in Mobile & Cross Platform SDK Aug 16, 2023
@krystofwoldrich
Copy link
Member

Hi,
thank you for the message and the details.

It looks like there is a difference between Sentry symbolication and metro-symbolicator which points to the correct line number 43.

Does this happen only with CodePush updates or also with full app releases?

Does this still happen, when you send a new error?

@krystofwoldrich krystofwoldrich moved this to Waiting for: Community in GitHub Issues with 👀 Aug 31, 2023
@krystofwoldrich
Copy link
Member

krystofwoldrich commented Sep 5, 2023

I've reproduced the error with our sample app.

https://sentry-sdks.sentry.io/issues/4453310672/events/62307475e6814589bfbc249c5f6e91c0/

Minified stack trace

ReferenceError: Property 'thisVariableDoesNotExist' doesn't exist
  at onPress(app:///index.android.bundle:1:985288)

Sentry

ReferenceError: Property 'thisVariableDoesNotExist' doesn't exist
  at Button.props.onPress(src/Screens/HomeScreen.tsx:81:24)

Metro Symbolicate

 ❯ npx metro-symbolicate index.map < stacktrace.txt
ReferenceError: Property 'thisVariableDoesNotExist' doesn't exist
  at onPress(src/Screens/HomeScreen.tsx:82:Button.props.onPress)

stacktrace.txt

ReferenceError: Property 'thisVariableDoesNotExist' doesn't exist
  at onPress(app:///index.android.bundle:1:985288)

index.map

source-map

const fs = require('fs');
const sourceMap = require('source-map');
var smc = new sourceMap.SourceMapConsumer(fs.readFileSync('./sample-new-architecture/index.map', 'utf8'));
console.log(smc.originalPositionFor({ line: 1, column: 985288 }));
{
  source: 'src/Screens/HomeScreen.tsx',
  line: 82,
  column: 12,
  name: 'thisVariableDoesNotExist'
}

@getsantry getsantry bot removed the status in GitHub Issues with 👀 Sep 5, 2023
@krystofwoldrich krystofwoldrich moved this from Needs Investigation to Backlog in Mobile & Cross Platform SDK Sep 5, 2023
@krystofwoldrich krystofwoldrich moved this from Backlog to Needs Discussion in Mobile & Cross Platform SDK Sep 5, 2023
@krystofwoldrich krystofwoldrich changed the title Highlighted line is off by one line (appcenter codepush release-react on iOS -- Android is correct) Highlighted line is off by one line on non existing property Sep 5, 2023
@Swatinem
Copy link
Member

Swatinem commented Sep 5, 2023

The off-by-one seems to be here somewhere:

<Token src/Screens/HomeScreen.tsx:80:23 (0:985283)>
<Token src/Screens/HomeScreen.tsx:81:12 name=thisVariableDoesNotExist (0:985288)>

The first token is the one we find, while the second token is the right one.

We are doing a -1 on both line and column, as the stack traces report those as 1-indexed, whereas the sourcemap tokens themselves contain those as 0-indexed.

So we are internally actually looking for 985287 and find the previous token. I’m just wondering why the other tools are doing the right thing here.

@Swatinem
Copy link
Member

Swatinem commented Sep 5, 2023

Some digging in the past later:

Seems like we have been adjusting columns ever since getsentry/sentry@bc8fa24 / getsentry/sentry#5922
(And adding +1 when the token comes back: getsentry/sentry@6867a45#diff-5649208c3eac2367f8d3130a299d7c3cd3020aa4c1a21f1d4ea57b80de38ef34R600)

When porting that logic to symbolicator, we have cargo-culted that behavior as well:

getsentry/symbolicator@70451dc#diff-d8382530b3ce9204a7a4c01b6ff3bde446870d29db23b45b8236281c5a5eea7cR183-R184
interestingly, backwards adjusting was added later:
getsentry/symbolicator@eb6b197#diff-d8382530b3ce9204a7a4c01b6ff3bde446870d29db23b45b8236281c5a5eea7cR166-R168

Even though other tools agree that columns are 0-based, both in stack traces as well as in the sourcemaps.

https://github.com/mozilla/source-map/blob/master/lib/source-map-consumer.js#L472-L475
https://github.com/tc39/source-map-spec/blob/1ac019b79757d9cea70401fa530a78b6c062feb8/source-map-rev3.md?plain=1#L110

I’m questioning my sanity a bit by now.

@Swatinem
Copy link
Member

Swatinem commented Sep 5, 2023

Okay, @mitsuhiko cleared up the confusion. While the JS/SourceMap ecosystem may agree that columns are 0-based, Sentry itself uses 1-based column in its event payloads and the UI.

So possibly the SDK fails to adjust the column before it sends it over to Sentry?

@skinsapp
Copy link
Author

@krystofwoldrich it only occurs with CodePush, not with Native builds. Yes, new errors are still one line off, but I did notice one that happened to be correct a while ago. I'm not sure why.

@Swatinem thank you for looking into this. You seem to be on the scent.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 Sep 14, 2023
@krystofwoldrich
Copy link
Member

@the-skins-app Thank you for the update. We reproduce it even with a native build. It should not make a difference. Do you have the same error where one if from Code Push one line off and one is from Native build and correct?

@krystofwoldrich
Copy link
Member

Differs based on the engine JSC/Hermes.

@krystofwoldrich krystofwoldrich moved this from Needs Discussion to In Progress in Mobile & Cross Platform SDK Sep 15, 2023
@krystofwoldrich krystofwoldrich moved this from In Progress to Needs Investigation in Mobile & Cross Platform SDK Sep 15, 2023
@github-project-automation github-project-automation bot moved this from Needs Investigation to Done in Mobile & Cross Platform SDK Sep 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants