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

Attempt to fix a crash that could occur when formatting dates and times #13393

Merged
merged 3 commits into from
Jan 28, 2025

Conversation

hichamboushaba
Copy link
Member

@hichamboushaba hichamboushaba commented Jan 24, 2025

Closes: #13390

Description

This PR is an attempt to fix the linked crash, the above crash happens when we try to format dates, and looking online lead me to this StackOverflow answer https://stackoverflow.com/a/18383395, which matches our case, as we are using the same SimpleFormatDate instances for the whole lifetime of DateUtils class, and multi-threading is involved (for example with the paging library in OrderListItemDataSource).

I tried to reproduce the crash manually by making multiple concurrent calls, but I failed, but I think this fix by using computed properties should fix the crash, as we'll use each instance just once.

Steps to reproduce

Just a quick check of the top-level screens should be enough.

Testing information

Tested and confirmed no regression.

  • I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

  • The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
  • Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
  • Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

@hichamboushaba hichamboushaba added the type: crash The worst kind of bug. label Jan 24, 2025
SimpleDateFormat is not thread safe, and reusing for multiple calls where there is a change of multi threading can lead to crashes, see https://stackoverflow.com/a/18383395
@hichamboushaba hichamboushaba force-pushed the issue/13390-fix-dateformat-crash branch from 0774b03 to 6167832 Compare January 24, 2025 16:58
@hichamboushaba hichamboushaba marked this pull request as ready for review January 24, 2025 16:59
@hichamboushaba hichamboushaba changed the title Avoid reusing the same SimpleDateFormat for multiple calls Attempt to fix a crash that could occur when formatting dates and times Jan 24, 2025
@hichamboushaba hichamboushaba added this to the 21.6 milestone Jan 24, 2025
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 24, 2025

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
App Name WooCommerce-Wear Android
Platform⌚️ Wear OS
FlavorJalapeno
Build TypeDebug
Commitc98ba62
Direct Downloadwoocommerce-wear-prototype-build-pr13393-c98ba62.apk

@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 24, 2025

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

App Name WooCommerce Android
Platform📱 Mobile
FlavorJalapeno
Build TypeDebug
Commitc98ba62
Direct Downloadwoocommerce-prototype-build-pr13393-c98ba62.apk

@codecov-commenter
Copy link

codecov-commenter commented Jan 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.10%. Comparing base (73135c1) to head (c98ba62).
Report is 4 commits behind head on trunk.

Additional details and impacted files
@@            Coverage Diff            @@
##              trunk   #13393   +/-   ##
=========================================
  Coverage     41.10%   41.10%           
  Complexity     6486     6486           
=========================================
  Files          1325     1325           
  Lines         77572    77572           
  Branches      10694    10694           
=========================================
  Hits          31889    31889           
  Misses        42850    42850           
  Partials       2833     2833           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@irfano irfano self-assigned this Jan 28, 2025
Copy link
Contributor

@irfano irfano left a comment

Choose a reason for hiding this comment

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

Great find, @hichamboushaba! LGTM! 👍🏻

@irfano irfano enabled auto-merge January 28, 2025 10:43
@irfano irfano merged commit efb750f into trunk Jan 28, 2025
15 of 18 checks passed
@irfano irfano deleted the issue/13390-fix-dateformat-crash branch January 28, 2025 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: crash The worst kind of bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ArrayIndexOutOfBoundsException: length=13; index=13
4 participants