Skip to content

Fix : UninitializedPropertyAccessException#6248

Merged
nicolas-raoul merged 5 commits intocommons-app:mainfrom
sonalyadav1:fix_duplicate_file_crash
May 28, 2025
Merged

Fix : UninitializedPropertyAccessException#6248
nicolas-raoul merged 5 commits intocommons-app:mainfrom
sonalyadav1:fix_duplicate_file_crash

Conversation

@sonalyadav1
Copy link
Contributor

Description (required)

When a user uploads a file with a name that already exists, they get a warning about the duplicate file. Clicking "Upload" after this caused a crash.

Fixes #6247

What changes did you make and why?

(1) Fixed crash when uploading after a duplicate filename warning by adding proper checks and error handling.

(2) Ensured the warning message displays correctly and does not interfere with the upload process.

Tests performed (required)

Tested {build variant (BetaDebug)} on {VIVO V25} with API level {34}.

Screenshots (for UI changes only)

Screenrecording_20250314_201059.mp4

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Somehow I still get the same crash with this branch...

@nicolas-raoul
Copy link
Member

screen-20250315-211239.mp4

@sonalyadav1
Copy link
Contributor Author

I tested the same branch on my end, but I’m not experiencing the crash.😓

@psh
Copy link
Collaborator

psh commented Mar 17, 2025

If I read the code correctly, you've only swapped an UninitializedPropertyAccessException for an IllegalStateException without addressing the root cause of why it might be throwing any kind of exception in the first place?

@sonalyadav1
Copy link
Contributor Author

@psh You're absolutely right—swapping exceptions doesn’t address the root cause. My initial approach was incomplete, and I appreciate you pointing that out.

@misaochan
Copy link
Member

Hi @sonalyadav1 , are you still working on this?

@sonalyadav1
Copy link
Contributor Author

@misaochan Due to ongoing university practicals and exams, my availability is limited at the moment. However, I’ll do my best to resolve the issue shortly.

@rohit9625
Copy link
Collaborator

Hey @sonalyadav1, do you need any help with this one?

@sonalyadav1
Copy link
Contributor Author

@rohit9625 Thanks for asking! I just pushed some fixes for the issue. If you get a chance, feel free to take a look and let me know if I missed anything!

@rohit9625
Copy link
Collaborator

I couldn't reproduce this bug on this branch or main. Can you repro this @sonalyadav1, before your fix?

@sonalyadav1
Copy link
Contributor Author

sonalyadav1 commented May 27, 2025

I wasn’t able to reproduce the issue on main or this branch, but I tried to fix the root cause based on the code.

@rohit9625
Copy link
Collaborator

rohit9625 commented May 27, 2025

I wasn’t able to reproduce the issue on the main or this branch, but I tried to fix the root cause based on the code.

No problem, the changes you made are quite reasonable but the issue won't be fixed until you repro this issue yourself and work on the actual root cause. That's why @nicolas-raoul still facing this issue on your branch.

I think we should consider other factors or preferences that we should have while replicating this issue. When I try to upload some pictures, the Location Not Found dialog appears before the Duplicate File Name dialog. But, it won't happen on your device, right @nicolas-raoul?

Edit: The Location Not Found dialog has no role in the crash. Sorry about that.

I guess the images you were trying to upload had location in metadata.

@rohit9625

This comment was marked as outdated.

Copy link
Collaborator

@rohit9625 rohit9625 left a comment

Choose a reason for hiding this comment

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

Any reason for adding this requireFactoryInitialized() function here? I think we don't need this.

Copy link
Collaborator

@rohit9625 rohit9625 left a comment

Choose a reason for hiding this comment

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

Sorry, I missed this change. I'm sure after calling the setupBasicKvStoreFactory function in the onCreate function, the crash won't happen again.

@rohit9625
Copy link
Collaborator

Finally, I got the crash on the main. These are the prerequisites:

  1. You must select at least 3 pictures to upload

  2. You must encounter the Note about multi-uploads dialog every time whenever performing multi-uploads.

  3. And if we press OK then it initializes the basicKvStore here:

    presenter!!.setupBasicKvStoreFactory { BasicKvStore(this@UploadActivity, it) }

  4. However, if we tick Don't show it again and then press OK it won't appear next time, and basicKvStore won't be initialized, eventually causing the crash.

  5. So, to repro this we should close the app and perform the multi-upload steps again (See the screencast).

Note: We cannot encounter the crash if we don't close the app after Step 4 because the basicKvStore object reference doesn't get cleared and that's the reason we have so many memory leaks. We'll need to work refactoring our codebase.

Screencast:

Crash_Repro.mp4

@rohit9625
Copy link
Collaborator

I can confirm that the issue is fixed on @sonalyadav1's branch. We can merge this PR @nicolas-raoul :)

After_Fix_Crash.mp4

Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

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

Thanks a lot @rohit9625 for the detailed reproduction steps and before/after screencasts, that's super helpful!

@sonalyadav1 Would you mind just adding kdoc to new functions, then I can merge. :-)

@sonalyadav1
Copy link
Contributor Author

sonalyadav1 commented May 28, 2025

@sonalyadav1 Would you mind just adding kdoc to new functions, then I can merge. :-)

Done!

@github-actions
Copy link

✅ Generated APK variants!

@nicolas-raoul nicolas-raoul merged commit c49c85e into commons-app:main May 28, 2025
1 check passed
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.

UninitializedPropertyAccessException: lateinit property basicKvStoreFactory has not been initialized at UploadPresenter.getBasicKvStoreFactory

5 participants