Jetpack Section: Scan - fix crash on snackbar display

[ashiagr]

Description

This PR fixes below crash when snackbar is displayed on Scan and Threat Details screen.

java.lang.NullPointerException: scan_state_layout must not be null
        at org.wordpress.android.ui.jetpack.scan.ScanFragment.showSnackbar(ScanFragment.kt:126)
        at org.wordpress.android.ui.jetpack.scan.ScanFragment.access$showSnackbar(ScanFragment.kt:31)
        at org.wordpress.android.ui.jetpack.scan.ScanFragment$setupObservers$2$1.invoke(ScanFragment.kt:89)

Findings

We passed fragment's root constraint layout (using Kotlin synthetics) in the showSnackbar method. We started experiencing this crash after replacing FragmentContainerLayout with Fragment in the corresponding activity layout xml.

As per this SO answer, we instantiated viewModels for these screen inside onViewCreated so the layout referenced in the showSnackbar method should've been present at the time we had to display the snackbar.

I think it is the way Kotlin synthetic properties are cached and referenced in the different ways we add a fragment (directly in the xml or using FragmentContainerView). Or crash might be occurring on random devices as per this comment, however I'm not 100% sure.

Solution

It is sufficient to just pass fragment's view in the showSnackbar method, so I've removed reference to the fragment's constraint layout in the method. This fixes the crash and snackbar is displayed properly.

To Test

Fix or ignore a threat from Threat Details screen (on a Jurassic Ninja site with scan capability and bad calendar plugin installed), see that app does not crash and snackbar is displayed properly.

(Or you can return Failure.RemoteRequestFailure from FixThreatsUseCase so that snackbar is shown for the failed response.)

PR submission checklist:

• I have considered adding unit tests where possible.
• 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.

[peril-wordpress-mobile]

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

[ParaskP7]

👋 @ashiagr !

I have reviewed and tested this PR as per the instructions, everything works as expected, good fix! 🌟

[ParaskP7]

👋 @ashiagr !

I also triggered a re-run of the CI (from failed) because it was stuck as some rake step, not sure. Hopefully it will complete now. Feel free to merge this PR when CI completes.

[ParaskP7]

👋 @ashiagr !

CI failed again, maybe ask Platform9 for support on that.

[peril-wordpress-mobile]

You can test the changes on this Pull Request by downloading the APK here.

[AliSoftware]

CI fixed!

[malinajirka]

@JavonDavis I introduced this crash in beta-2 😢. This PR fixes the issue and is targeting the frozen branch. Could you please include it in the next beta version? Thanks!
P.S. Thank you @ashiagr and @ParaskP7!

[fluiddot]

👋 I see this PR doesn't included the last commit from #14082, unfortunately this is causing a conflict with a PR I'd like to include in the release/16.7 branch.

@AliSoftware how should I solve the conflict with or without the last commit? Thanks for the help!

[AliSoftware]

The last commit of #14082 should have been included too indeed – it was added late in the investigation of this issue which is why I forgot to cherry-pick it here later, but it is part of the integrity of the whole fix and should indeed be added on top.

Note tho that @jkmassel is currently applying the fix from #14082 to the base of release/16.7 so that the CI is properly fixed on the release branch for all release PRs. So maybe we should wait for Jeremy to push the fix on release/ branch first 😉

[fluiddot]

Note tho that @jkmassel is currently applying the fix from #14082 to the base of release/16.7 so that the CI is properly fixed on the release branch for all release PRs. So maybe we should wait for Jeremy to push the fix on release/ branch first 😉

Ok, thank you very much @AliSoftware for the info, we'll wait for the fix then.