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

Address NullPointerException crash in AztecHeadingSpan #1071

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

SiobhyB
Copy link

@SiobhyB SiobhyB commented Dec 4, 2023

A potential fix for the crash described at wordpress-mobile/WordPress-Android#18657.

Related PRs

Description

Sentry has been reporting the following crash for both the WordPress and Jetpack Android apps:

NullPointerException
org.wordpress.aztec.spans.AztecHeadingSpan in chooseHeight

The stack trace for each individual event specifically references one of the following lines (the specific line varies for each event), all of which are within the chooseHeight function and include a reference to previousFontMetrics:

As described at wordpress-mobile/WordPress-Android#18657 (comment), it has proven difficult to reproduce this crash. It's suspected that there are specific instances with lower performing devices or devices on poor connections that can lead to previousFontMetrics being set as null elsewhere in the code while chooseHeight is running.

Given the difficulty reproducing, it hasn't been possible to get to the heart of this issue. Instead, with this PR, the previous non-null assertions previously used with previousFontMetrics have been replaced with safe calls. With this approach, the functionality should remain the same, while guarding against NullPointerException crashes.

Test

Verify Aztec demo continues to function as expected

  1. Load the Aztec demo with this PR's changes applied.
  2. Verify that you can add headings with no unexpected side effects.

Verify Gutenberg continues to function as expected

  1. Using the installable build available from Address NullPointerException crash in AztecHeadingSpan WordPress-Android#19724, install the app and navigate to the post editor.
  2. Add some heading blocks and ensure there are no unexpected side effects when performing tasks. Example tasks to test include splitting the block, applying new font sizes and line heights, etc.

@SiobhyB SiobhyB added the Gutenberg This issue is also valid importing Aztec in Gutenberg label Dec 4, 2023
@SiobhyB SiobhyB changed the title refactor: Guard previousFontMetrics against null Address NullPointerException crash in AztecHeadingSpan Dec 4, 2023
@SiobhyB SiobhyB changed the title Address NullPointerException crash in AztecHeadingSpan Address NullPointerException crash in AztecHeadingSpan Dec 4, 2023
@SiobhyB SiobhyB marked this pull request as ready for review December 4, 2023 15:47
@SiobhyB SiobhyB requested review from fluiddot and khaykov December 4, 2023 15:48
@SiobhyB
Copy link
Author

SiobhyB commented Dec 4, 2023

@khaykov, 👋 , I wanted to give you a heads up on this PR as I know Day One uses Aztec for the Android app. Can you take a look to let me know if you spot anything that might cause issues for you? We're hoping to merge this fix in time for a beta in the next day or so.

Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

LGTM 🎊 !

Regarding the changes, I forced setting previousFontMetrics to null in the Aztec demo, and seems there are no visible side effects.

I've tested both the Aztec demo app and Gutenberg, the Headings worked as expected 🎊. The only thing I noticed is some unexpected behaviors in the Aztec demo, although they aren't regressions as they can be reproduced in trunk. I can follow-up by opening a GitHub issue.

aztec-android-heading-bugs.mp4

@SiobhyB
Copy link
Author

SiobhyB commented Dec 4, 2023

@khaykov, I'm going to go ahead to merge this PR, so we can get it ready for an upcoming beta. Please let me know if there's anything that stands out to you as off, though! I think the changes are fairly safe, but thought it was best to make you aware of them regardless.

@SiobhyB SiobhyB merged commit 79eddba into trunk Dec 4, 2023
14 checks passed
@SiobhyB SiobhyB deleted the refactor/guard-previousfontmetrics-against-null branch December 4, 2023 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg This issue is also valid importing Aztec in Gutenberg
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants