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] IllegalArgumentException: in ReaderPostRenderer #20997

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

zwarm
Copy link
Contributor

@zwarm zwarm commented Jun 17, 2024

Fixes #20996

This PR is an attempt to fix IllegalArgumentException: jsObjectName wvHandler was already added.

The exception we are encountering indicates that the wvHandler JavaScript object name is being added multiple times to the WebView, which is not allowed. This can happen if setWebViewMessageHandler is called more than once for the same WebView instance without removing or properly handling the previously added JavaScript object.

To avoid this issue, this PR sets a tag on the webview and checks for the instance of that tag before the jsObject is added again.

Note: I was unable to recreate the issue, so please feel free to block this fix.

To Test:

As the issue can't be recreated, please test that the existing functionality works as expected. These test instructions were taken from #20895, as this is when the code was added.

  • Open Jetpack
  • Make sure data collection is enabled in App Settings -> Privacy
  • Go to Reader
  • Go to any post
  • Select some text
  • Verify the reader_article_text_highlighted event is tracked (use logcat)
  • Tap on Copy in the context menu
  • Verify the reader_article_text_copied event is tracked

Regression Notes

  1. Potential unintended areas of impact
    The crash still happens

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual testing

  3. What automated tests I added (or what prevented me from doing so)
    N/A


PR Submission Checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing Checklist (strike-out the not-applying and unnecessary ones): N/A

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ This PR is assigned to the milestone 25.1 ❄️. The due date for this milestone has already passed.
Please assign it to a milestone with a later deadline or check whether the release for this milestone has already been finished.

Generated by 🚫 Danger

Copy link

sonarcloud bot commented Jun 17, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@wpmobilebot
Copy link
Contributor

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr20997-cfd8b51
Commitcfd8b51
Direct Downloadjetpack-prototype-build-pr20997-cfd8b51.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr20997-cfd8b51
Commitcfd8b51
Direct Downloadwordpress-prototype-build-pr20997-cfd8b51.apk
Note: Google Login is not supported on these builds.

Copy link

codecov bot commented Jun 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 40.98%. Comparing base (9e26ec5) to head (cfd8b51).

Files Patch % Lines
...ordpress/android/ui/reader/ReaderPostRenderer.java 0.00% 3 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           release/25.1   #20997      +/-   ##
================================================
- Coverage         40.99%   40.98%   -0.01%     
================================================
  Files              1522     1522              
  Lines             69622    69625       +3     
  Branches          11489    11490       +1     
================================================
  Hits              28539    28539              
- Misses            38494    38497       +3     
  Partials           2589     2589              

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

Copy link
Member

@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.

I also couldn't reproduce the crash but your solution looks reasonable to me. LGTM! 👍🏻

@irfano irfano merged commit 42fee84 into release/25.1 Jun 18, 2024
21 of 24 checks passed
@irfano irfano deleted the issue/20996-iae-jsobjectname branch June 18, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants