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: Crash with screenshot is reported twice #2134

Merged
merged 4 commits into from
Sep 9, 2022

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Sep 8, 2022

📜 Description

Fix a bug where crashes with screenshots were reported twice.

💡 Motivation and Context

closes #2130

💚 How did you test it?

Unit Test

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

@github-actions
Copy link

github-actions bot commented Sep 8, 2022

Performance metrics 🚀

Plain With Sentry Diff
Startup time (ms) 1254.10 1277.63 23.53
Size (bytes) 21157 334116 312959

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

A quick glance from a native dev, if this is the proper way for checking if a file is a dir would be great. Thanks for the quick fix.

@@ -107,7 +107,7 @@ getReportCount()
}
struct dirent *ent;
while ((ent = readdir(dir)) != NULL) {
if (getReportIDFromFilename(ent->d_name) > 0) {
if (ent->d_type != 4 && getReportIDFromFilename(ent->d_name) > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

m: Please create a constant for 4. I have no idea what it means.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do that ;)

Copy link
Contributor Author

@brustolin brustolin Sep 8, 2022

Choose a reason for hiding this comment

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

Or even better, I'll use the default constant meant for this.

@brustolin
Copy link
Contributor Author

A quick glance from a native dev, if this is the proper way for checking if a file is a dir would be great.

Well, this was straight from the documentation
https://opensource.apple.com/source/xnu/xnu-6153.41.3/bsd/sys/dirent.h

@Swatinem any comments on this?

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks

@brustolin brustolin merged commit 45ed684 into master Sep 9, 2022
@brustolin brustolin deleted the fix/two-events-with-screenshot branch September 9, 2022 07:52
@github-actions
Copy link

github-actions bot commented Sep 9, 2022

Fails
🚫 Please consider adding a changelog entry for the next release.
`Instructions and example for changelog`$

Please add an entry to CHANGELOG.md` to the "Unreleased" section under the following heading:

To the changelog entry, please add a link to this PR (consider a more descriptive message):`

- Crash with screenshot is reported twice(#2134)

If none of the above apply, you can opt out by adding _#skip-changelog_ to the PR description.

Generated by 🚫 dangerJS against 843c7db

kevinrenskers added a commit that referenced this pull request Sep 15, 2022
* master: (73 commits)
  ref: Fix typo in measurement (#2154)
  Fix typos in changelog (#2153)
  docs: Add a note on flaky tests to Contributing (#2161)
  docs: fix PR reference in CHANGELOG (#2157)
  test: generate graphs for benchmarks (#2158)
  release: 7.25.1
  fix: Prewarmed app start detection (#2151)
  ci: temporarily disable scheduled CI runs uploading profiles/symbols (#2152)
  Run tests on iOS 16 using Xcode 14.0 (#2147)
  ref: Fix Xcode 14 compile issues (#2145)
  Provide fallbacks for assertions in production (#2141)
  Fix compile errors in Xcode 14 (#2146)
  release: 7.25.0
  fix: setting SDK name through options[sdk][name] shouldn't clear version (#2139)
  fix: SentryTracer instances should be called "tracer", not "transaction" (#2137)
  ref: functions to convert to/from enum cases (#2108)
  fix: Crash with screenshot is reported twice (#2134)
  meta: Clarify PR rules in Contributing (#2133)
  meta: Fix Changelog (#2136)
  feat: Add support for dynamic library (#1726)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate issues when enabling screenshots
2 participants