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

Propagate changes in app.isLaunching to the NDK layer #1180

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Mar 9, 2021

Goal

Currently app.isLaunching is only set on JVM error reports. This changeset adds it to NDK error reports and adds verification that it has been set for both in the form of E2E tests.

Changeset

  • LaunchCrashTracker extends BaseObservable and notifies its observers when isLaunching is set to false
  • The NativeBridge which communicates with the NDK layer invokes a JNI function which updates bsg_event->app.is_launching to false
  • Fixed JVM serialization bug where inForeground rather than isLaunching value was serialized
  • Added is_launching to bsg_app_info struct
  • Added bugsnag_app_get_is_launching/bugsnag_app_set_is_launching functions to allow users to alter the value in an on_error callback.

Struct migration

When a fatal NDK error occurs, the memory contents of bsg_event struct is persisted on disk and then loaded again the next launch. Because the struct can change between releases, old payloads need to be migrated to the new struct. This has been achieved by the following changes:

  • Check whether BUGSNAG_EVENT_VERSION is 4 and load it into bugsnag_report_v4, which is a copy of the previous struct definition
  • Check the BUGSNAG_EVENT_VERSION in the serializer and migrated the bsg_app_info values
  • Set a default value for app.is_launching to false if no information was available in the struct

Testing

  • Added/updated unit test coverage where appropriate, particularly for migrating the struct
  • Verified that app.isLaunching is set appropriately in JVM/NDK E2E tests

@fractalwrench fractalwrench changed the title feat: propagate app.isLaunching to NDK layer Propagate changes in app.isLaunching to the NDK layer Mar 10, 2021
@fractalwrench fractalwrench marked this pull request as ready for review March 10, 2021 09:50
@fractalwrench fractalwrench force-pushed the PLAT-5979/ndk-is-launching branch 2 times, most recently from e843b4d to 6f0afd3 Compare March 11, 2021 15:32
@fractalwrench fractalwrench force-pushed the PLAT-5979/ndk-is-launching branch 2 times, most recently from 9a95120 to 830b3ed Compare March 11, 2021 16:24
Copy link
Contributor

@twometresteve twometresteve left a comment

Choose a reason for hiding this comment

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

LGTM so long as @kstenerud is now happy.

@fractalwrench fractalwrench merged commit 7a4f8e8 into integration/road-1175-identify-crashes-on-launch Mar 12, 2021
@fractalwrench fractalwrench deleted the PLAT-5979/ndk-is-launching branch March 12, 2021 10:15
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.

3 participants