Skip to content

Conversation

@kidinov
Copy link
Contributor

@kidinov kidinov commented May 25, 2023

Closes: #9100

Description

Sentry Issue: WOOCOMMERCE-ANDROID-6VF

To reproduce the crash:

  • Open a product
  • Click on the image
  • Open image
  • Enabled don't keep activities
  • Return back to the app

The crash happens because two fragments use the same ViewModel while inside of the model, it expects specific sets of arguments passed from a Bundle

private val navArgs: ProductImagesFragmentArgs by savedState.navArgs()

In the older version of Hilt, for some unclear reason in this situation, it was a passing bundle from the first fragment in the back stack, which was ProductImagesFragment, and therefore crash didn't happen, although the state could be inconsistent already because state inside of the VM not recreated properly.

With the update hilt, it passes the latest bundle there; that's why it cannot recover required images

As a quick workaround, I just pass all the arguments from the first fragment to the second, but issues with the state still stay here because there is an inconsistency between the stored in the bundle state and the state that is kept inside the VM

Ideally, we should decouple two fragments and use 2 VMs and communicate.

Testing instructions

Images/gif

  • Open a product
  • Click on the image
  • Open image
  • Enabled don't keep activities
  • Return back to the app
  • Notice that it doesn't crash anymore
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

kidinov added 2 commits May 24, 2023 15:56
… request code to nav_graph_image_gallery.xml and use them in ProductImagesFragment.kt to avoid crash on activity death/restart
@kidinov kidinov added the type: crash The worst kind of bug. label May 25, 2023
@kidinov kidinov linked an issue May 25, 2023 that may be closed by this pull request
@kidinov kidinov added this to the 13.8 milestone May 25, 2023
@peril-woocommerce
Copy link

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@kidinov kidinov requested a review from 0nko May 25, 2023 06:29
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented May 25, 2023

You can test the changes on this Pull Request by downloading an installable build, or scanning this QR code:

@kidinov kidinov changed the title [FIX] ProductImagesFragment activity restart [FIX] ProductImagesFragment activity death May 25, 2023
@0nko 0nko self-assigned this May 25, 2023
Copy link
Contributor

@0nko 0nko left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@0nko 0nko enabled auto-merge May 25, 2023 13:51
@0nko 0nko merged commit fa02565 into trunk May 25, 2023
@0nko 0nko deleted the 9100-runtimeexception-javalangreflectinvocationtargetexception branch May 25, 2023 14:22
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.

RuntimeException: java.lang.reflect.InvocationTargetException

4 participants