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 deserialization of custom stackframe fields in cached error reports #576

Merged
merged 21 commits into from Sep 3, 2019

Conversation

fractalwrench
Copy link
Contributor

Goal

This changeset fixes the deserialization of custom fields in a stackframe by ErrorReader, when an error report has been cached on disk. This caused several issues:

  • Prevented delivery of JS exceptions in React Native where the method was null for some stackframes
  • Missing columnNumber in React Native JS exceptions, reducing the available information for source mapping
  • Missing loadAddress, frameAddress, and symbolAddress fields for NDK errors
  • Stacktrace length was not trimmed for JS/NDK exceptions at the JVM deserialisation layer, meaning this could cause recursive stacktraces to be dropped by our API

Changeset

If no beforeSend callback has been specified, skip deserialization of the error report and stream the file directly, to reduce unnecessary work and mitigate the impact of any other deserialisation bugs.

Generally speaking, the serialization has been changed to allow an arbitrary List<Map<String, Object>>, which contains the full stacktrace. Broken down across classes, this has had the following effects:

CachedThread:

  • Instantiates a Stacktrace object eagerly in the constructor, rather than in the stream method
  • Adds a secondary constructor which takes an arbitrary list of stackframes.

ErrorReader:

  • Deserializes stackframes as a List<Map> rather than mapping data into a StackTraceElement.

Exceptions:

  • The toStream method now delegates to the BugsnagException class. The pre-existing implementation has been split across BugsnagException and Stacktrace respectively.

Stacktrace:

  • Trim stacktrace length in the constructor rather than in the toStream method
  • Add secondary constructor which provides a List<Map> for arbitrary stackframes
  • Convert StackTraceElement to List<Map> by adapting the existing serialization code

BugsnagException:

  • Implement Streamable. If the wrapped exception is of type Streamable, delegate the streaming to it
  • Add a constructor which accepts List<Map> for serializing stackframes
  • If a List<Map> has been supplied, construct the Stacktrace using that, rather than the throwable's stacktrace.

Tests

  • Added mazerunner scenarios with custom streamable exceptions
  • Added and updated existing unit test coverage

Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

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

I've gotten to the start of Stacktrace.java so far but need to break up my review a bit. This is what I have so far.

@fractalwrench fractalwrench dismissed kattrali’s stale review September 2, 2019 15:47

review comments resolved

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.

None yet

2 participants