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

Change access modifier to private for stack trace mode option setter #1806

Merged
merged 8 commits into from
Oct 7, 2024

Conversation

tustanivsky
Copy link
Collaborator

This PR should prevent users from accidentally enabling Enhanced stack trace mode via the SentryUnityOptions parent class property. Enhanced mode is not supported in IL2CPP builds (see #1033 for more details) and thus could cause crashes like in #1783.

Closes #1783

@tustanivsky tustanivsky changed the title fix: change access modifier for stack trace mode option Change access modifier to private for stack trace mode option setter Sep 18, 2024
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Looks like the whole property was private though (getter and setter). We're making the getter public but the setter private. Is this helping somehow?

CHANGELOG.md Outdated Show resolved Hide resolved
@bitsandfoxes
Copy link
Contributor

What we wanted to achieve was take away the footgun for users to switch mode and break their game by hiding the property. But it's still accessible via the base class. With this we're giving the "no-footgun!" attempt a second chance.

@bruno-garcia
Copy link
Member

bruno-garcia commented Oct 6, 2024

What we wanted to achieve was take away the footgun for users to switch mode and break their game by hiding the property. But it's still accessible via the base class. With this we're giving the "no-footgun!" attempt a second chance.

As per my comment, isnt' the whole thing private and now we're making half private? If not, I might misundersatnd the code.

tustanivsky and others added 2 commits October 7, 2024 10:30
Co-authored-by: Bruno Garcia <bruno@brunogarcia.com>
@bitsandfoxes
Copy link
Contributor

As per my comment, isnt' the whole thing private and now we're making half private? If not, I might misundersatnd the code.

Making the property private on the child only hides it on the child. Accessing the property i.e. in the configure callback falls back directly to the SentryOptions.

Screenshot 2024-10-07 at 16 23 37

Keeping the property public but overwriting the accessor on the setter allows us to put a hurdle in front of the footgun.

Screenshot 2024-10-07 at 16 22 48

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

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

Thanks for finding this!

@bitsandfoxes bitsandfoxes dismissed bruno-garcia’s stale review October 7, 2024 14:39

Added some additional context as to why the change.

@bitsandfoxes bitsandfoxes merged commit 37a8820 into main Oct 7, 2024
15 checks passed
@bitsandfoxes bitsandfoxes deleted the fix/trace-mode-access branch October 7, 2024 14:39
@bruno-garcia
Copy link
Member

Thank you for clarifying that

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.

WebGL exception during IL2CPP processing
3 participants