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 decodeFile Null Crash #556

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Fix decodeFile Null Crash #556

wants to merge 21 commits into from

Conversation

tobitech
Copy link
Collaborator

@tobitech tobitech commented Feb 25, 2025

Story: https://app.shortcut.com/smileid/story/15206

Summary

  • Use External Storage as primary storage for capture selfie images and default to internal storage in cases where it's not possible to use external storages.
  • Safely handle the scenario where decoding the selfie image file fails (especially when the file url is null) by rejecting the selfie capture so that users capture the selfie again.

Known Issues

N/A.

Test Instructions

  • Now that external app-specific storage is used, run a selfie enrolment job and confirm that the selfie files don't appear anywhere else on your device.
  • To simulate a situation where the selfie image can't be decoded you can set selfieToConfirm to null before the check for file validity in SelfieViewModel.kt.

Screenshot

N/A

@tobitech tobitech self-assigned this Feb 25, 2025
@tobitech tobitech requested a review from a team as a code owner February 25, 2025 19:41
Copy link
Contributor

@JNdhlovu JNdhlovu left a comment

Choose a reason for hiding this comment

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

@tobitech can you please fix lint issues

fileSavePath = context.getDir("SmileID", MODE_PRIVATE).absolutePath
// Primary: /storage/emulated/0/Android/data/<package name>/files/SmileID
// Fallback: /data/user/0/<package name>/app_SmileID
fileSavePath = context.getExternalFilesDir("SmileID")?.absolutePath
Copy link
Member

Choose a reason for hiding this comment

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

the fallback will never work fyi

this is because the app data is always available. you need to change this to implementation to see if the externalFileDir is available, then use it (this will be an api check I believe), then use it otherwise use the primary implementation

Copy link
Contributor

@robin-smileid robin-smileid left a comment

Choose a reason for hiding this comment

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

I believe you have to change the error title @tobitech

robin-smileid
robin-smileid previously approved these changes Mar 6, 2025
Copy link
Contributor

@robin-smileid robin-smileid left a comment

Choose a reason for hiding this comment

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

Code changes look fine to me. Tested and works on my SM M21. Great work @tobitech

SmileIDCrashReporting.hub.captureException(e)
onError(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

@jumaallan I think we might have deleted the onError by accident? We should likely keep it to trigger the error screen.

Copy link
Contributor

@JNdhlovu JNdhlovu left a comment

Choose a reason for hiding this comment

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

Batman lol, left a few comments

try {
BitmapPainter(BitmapFactory.decodeFile(path).asImageBitmap())
BitmapFactory.decodeFile(path)?.let { bitmap: Bitmap ->
Copy link
Contributor

Choose a reason for hiding this comment

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

I think as long as we don't sample this bitmap this will always crash with OOM

  1. We don't need a high quality image for display
  2. This may also need to be done async as well

)
}
// when {
// showInstructions && !acknowledgedInstructions -> SmartSelfieInstructionsScreen(
Copy link
Contributor

Choose a reason for hiding this comment

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

undo/delete?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants