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

feat(proguard): Remap exception names in proguard #15795

Merged
merged 2 commits into from
Nov 30, 2019

Conversation

mitsuhiko
Copy link
Member

No description provided.

@mitsuhiko
Copy link
Member Author

@jan-auer not super happy with the completely broken abstraction here but i rather refactor the entire thing at a later point than to clean it up now. Not sure though if this invalidates some assumptions.

Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

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

Agreed that this can be cleaned up later, as the abstraction is already a bit leaky.

def _report_stack(stacktrace, container):
if not stacktrace or not get_path(stacktrace, "frames", filter=True):
def _report_stack(stacktrace, container, is_exception=False):
if not is_exception and (not stacktrace or not get_path(stacktrace, "frames", filter=True)):
Copy link
Member

Choose a reason for hiding this comment

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

This change is indeed confusing as we're now reporting non-existing stack traces just to get to the exception container. I also don't have a better idea, since I suppose you need the stack frame's platform to decide whether you want to touch the container exception.

To be safe, you could default frames to an empty list, but since you only selectively turn this on, I could not find an invalidated invariant. At least the native plugin relies on empty stacktraces being removed, but there you don't toggle, so it's fine.

As an alternative, could we add a find_exceptions_in_data and call it in should_process_for_stacktraces as well as process_stacktraces before processing the stack traces?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would like to change the entire thing to return stacks + related data and treat missing stacks as empty stacks.

for view in self.mapping_views:
original = view.lookup(key)
if original != key:
new_module, new_cls = original.rsplit(".", 1)
Copy link
Member

Choose a reason for hiding this comment

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

Will this work with generics that contain class paths?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea. At least not different to how we already do with methods

@bruno-garcia
Copy link
Member

Is there anything blocking this PR?
Would be great to get this in.

@mitsuhiko mitsuhiko merged commit 1ce310b into master Nov 30, 2019
@mitsuhiko mitsuhiko deleted the feature/remap-exception-names branch November 30, 2019 00:49
bruno-garcia added a commit to getsentry/sentry-android that referenced this pull request Nov 30, 2019
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 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.

None yet

3 participants