-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Fix fatal crash when getExternalFilesDir returns null in ManageSpaceActivity #19831
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
base: main
Are you sure you want to change the base?
Fix fatal crash when getExternalFilesDir returns null in ManageSpaceActivity #19831
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes a crash in ManageSpaceActivity when getExternalFilesDir() returns null despite external storage being mounted. The fix catches the fatal storage exception during activity startup and displays an error dialog instead of crashing.
- Added error handling in
onCreate()to catch and handle storage-related RuntimeExceptions - Implemented a fatal error dialog that finishes the activity after user acknowledgement
- Added comprehensive Robolectric tests to verify the error handling behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ManageSpaceActivity.kt | Added error handling in onCreate to catch storage exceptions and display fatal error dialog |
| ManageSpaceActivityNoExternalFilesDirTest.kt | Added test coverage for null getExternalFilesDir scenario and dialog behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/managespace/ManageSpaceActivity.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/managespace/ManageSpaceActivity.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/managespace/ManageSpaceActivity.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/test/java/com/ichi2/anki/ui/windows/ManageSpaceActivityNoExternalFilesDirTest.kt
Outdated
Show resolved
Hide resolved
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| try { | ||
| super.onCreate(savedInstanceState) | ||
| } catch (e: RuntimeException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
The exception is being thrown during the creation of the ManageSpaceActivity and if its not throwing an exception then I think the super.onCreate needs to be called if the onCreate function is being overriden. So either the exception needs to be handled in ManageSpaceActivity or somewhere before where it actually occurs and being thrown but that would need to handle it at multiple places . So if you suggest any better way to handle it I will surely implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sanjaysargam This exception comes from:
Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/SingleFragmentActivity.kt
Lines 53 to 55 in 1223e1b
| if (!ensureStoragePermissions()) { | |
| return | |
| } |
Could you take a look through the logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the exception happens in ensureStoragePermissions() before ManageSpaceActivity.onCreate() is reached.
So the crash happens earlier in the call chain, and our try-catch in ManageSpaceActivity never runs.
I think the best fix is to catch SystemStorageException in IntentHandler.grantedStoragePermissions() and treat it as "permission not granted" so the activity finishes cleanly instead of crashing.
We can keep the ManageSpaceActivity fix as a safety net, but handling it in IntentHandler feels like the right primary fix.
|
If you're going to put an AI reviewer on the PR, don't leave the comments unhandled. The code style is unusual - why would you catch |
|
The exception is being thrown during the creation of the ManageSpaceActivity and if its not throwing an exception then I think the super.onCreate needs to be called if the onCreate function is being overriden. So either the exception needs to be handled in ManageSpaceActivity or somewhere before where it actually occurs and being thrown but that would need to handle it at multiple places . So if you suggest any better way to handle it I will surely implement it. |
|
Could you please add |
|
|
||
| private fun isFatalStorageError(e: Throwable): Boolean = | ||
| e.message?.contains( | ||
| "getExternalFilesDir unexpectedly returned null", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string might change across Android versions. I'd suggest you to use a more robust detection method
| private fun showFatalErrorDialog(e: Throwable) { | ||
| AlertDialog | ||
| .Builder(this) | ||
| .setTitle("Fatal error") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be localized
AnkiDroid/src/test/java/com/ichi2/anki/ui/windows/ManageSpaceActivityNoExternalFilesDirTest.kt
Outdated
Show resolved
Hide resolved
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| try { | ||
| super.onCreate(savedInstanceState) | ||
| } catch (e: RuntimeException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the exception happens in ensureStoragePermissions() before ManageSpaceActivity.onCreate() is reached.
So the crash happens earlier in the call chain, and our try-catch in ManageSpaceActivity never runs.
I think the best fix is to catch SystemStorageException in IntentHandler.grantedStoragePermissions() and treat it as "permission not granted" so the activity finishes cleanly instead of crashing.
We can keep the ManageSpaceActivity fix as a safety net, but handling it in IntentHandler feels like the right primary fix.
AnkiDroid/src/test/java/com/ichi2/anki/ui/windows/ManageSpaceActivityNoExternalFilesDirTest.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/test/java/com/ichi2/anki/ui/windows/ManageSpaceActivityNoExternalFilesDirTest.kt
Outdated
Show resolved
Hide resolved
|
@sanjaysargam |
|
Please squash your commits into one. If you need any help regarding this, let me know. |
|
@sanjaysargam |
|
@Kartikey15dem
|
929085f to
2d989ad
Compare
|
@sanjaysargam |
|
just a small request. Tag only when it's really urgent. Maintainer review PR, when they are free. |
Oh really sorry, it was my first PR so I thought I have to notify the maintainer to get my PR merged. Will keep in mind next time. |
|
We shouldn't have a try...catch around super.onCreate. This case should be handled properly |
Storage exception handled inside intenthandler
c7172fc to
78419fb
Compare
I have removed it . I was just using it as a last resort but I think its unnecessary as I am I handling the exception where it is being thrown . All my checks have passed . |
Fixes - #19553
ManageSpaceActivity crashes on startup when
getExternalFilesDir()unexpectedlyreturns null.
Solution
Tests
Notes
-Keeps error handling as close to the permission/intent flow as possible, improving UX and limiting the need for global fallbacks.