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 crash in select method in Decks class #10047

Closed
wants to merge 5 commits into from

Conversation

ShridharGoel
Copy link
Member

@ShridharGoel ShridharGoel commented Jan 1, 2022

Purpose / Description

Fix crash in select method in Decks class.

Fixes

Fixes #10046

Approach

Added try-catch block to handle NullPointerException.

Checklist

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

interesting - seems like a reasonable guard

@mikehardy mikehardy added the Needs Second Approval Has one approval, one more approval to merge label Jan 1, 2022
@mikehardy mikehardy added this to the 2.16 release milestone Jan 1, 2022
Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

We should handle the exception here (and ideally add a regression test in a follow up PR).

This is caused by a corrupt collection, we should match Anki Desktop in libAnki code (especially as we lose control of the functionality once the Rust backend is enabled).

We likely want a "corrupt collection" message to appear if a faulty collection is provided.

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

Code for a DB error is one of the below. We want to provide the user with options if there is an issue (including check database):

const val DIALOG_LOAD_FAILED = 0
const val DIALOG_DB_ERROR = 1
const val DIALOG_ERROR_HANDLING = 2
const val DIALOG_REPAIR_COLLECTION = 3
const val DIALOG_RESTORE_BACKUP = 4
const val DIALOG_NEW_COLLECTION = 5
const val DIALOG_CONFIRM_DATABASE_CHECK = 6
const val DIALOG_CONFIRM_RESTORE_BACKUP = 7
const val DIALOG_FULL_SYNC_FROM_SERVER = 8
/** If the database is locked, all we can do is reset the app */
const val DIALOG_DB_LOCKED = 9
/** If the database is at a version higher than what we can currently handle */
const val INCOMPATIBLE_DB_VERSION = 10

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

LGTM will wait to see if this (much improved...) version where we recognize the corruption looks good to David

Copy link
Member

@david-allison david-allison left a comment

Choose a reason for hiding this comment

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

The try/catch doesn't match the stack trace of the issue.

java.lang.NullPointerException: Attempt to invoke virtual method 'java.lang.String com.ichi2.utils.JSONObject.getString(java.lang.String)' on a null object reference
at com.ichi2.libanki.Decks.select(Decks.java:1)
at com.ichi2.libanki.Decks.current(Decks.java:2)
at com.ichi2.anki.DeckPicker.__renderPage(DeckPicker.java:32)
at com.ichi2.anki.DeckPicker$UpdateDeckListListener.actualOnPostExecute(DeckPicker.java:9)
at com.ichi2.anki.DeckPicker$UpdateDeckListListener.actualOnPostExecute(DeckPicker.java:1)
at com.ichi2.async.TaskListenerWithContext.onPostExecute(TaskListenerWithContext.java:2)
at com.ichi2.async.CollectionTask.onPostExecute(CollectionTask.java:3)
at android.os.AsyncTask.finish(AsyncTask.java:771)
at android.os.AsyncTask.access$900(AsyncTask.java:199)
at android.os.AsyncTask$InternalHandler.handleMessage(AsyncTask.java:788)
at android.os.Handler.dispatchMessage(Handler.java:106)
at android.os.Looper.loop(Looper.java:246)
at android.app.ActivityThread.main(ActivityThread.java:8506)
at java.lang.reflect.Method.invoke(Native Method)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:602)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1130) 

showDatabaseErrorDialog(DatabaseErrorDialog.DIALOG_DB_ERROR); is broken, and likely has been for some time:

2022-01-02 23:52:14.109 30620-30620/com.ichi2.anki E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.ichi2.anki, PID: 30620
    java.lang.NullPointerException
        at com.ichi2.anki.dialogs.DatabaseErrorDialog.onCreateDialog(DatabaseErrorDialog.kt:92)
        at com.ichi2.anki.dialogs.DatabaseErrorDialog.onCreateDialog(DatabaseErrorDialog.kt:36)
        at androidx.fragment.app.DialogFragment.prepareDialog(DialogFragment.java:647)
        at androidx.fragment.app.DialogFragment.onGetLayoutInflater(DialogFragment.java:561)
        at androidx.fragment.app.Fragment.performGetLayoutInflater(Fragment.java:1698)
        at androidx.fragment.app.FragmentStateManager.createView(FragmentStateManager.java:492)
        at androidx.fragment.app.FragmentStateManager.moveToExpectedState(FragmentStateManager.java:261)
        at androidx.fragment.app.FragmentManager.executeOpsTogether(FragmentManager.java:1840)
        at androidx.fragment.app.FragmentManager.removeRedundantOperationsAndExecute(FragmentManager.java:1758)
        at androidx.fragment.app.FragmentManager.execPendingActions(FragmentManager.java:1701)
        at androidx.fragment.app.FragmentManager.executePendingTransactions(FragmentManager.java:564)
        at com.ichi2.anki.AnkiActivity.showDialogFragment(AnkiActivity.java:506)
        at com.ichi2.anki.AnkiActivity.showDialogFragment(AnkiActivity.java:491)
        at com.ichi2.anki.AnkiActivity.showDialogFragment(AnkiActivity.java:487)
        at com.ichi2.anki.AnkiActivity.showAsyncDialogFragment(AnkiActivity.java:531)
        at com.ichi2.anki.AnkiActivity.showAsyncDialogFragment(AnkiActivity.java:517)
        at com.ichi2.anki.DeckPicker.showDatabaseErrorDialog(DeckPicker.java:1319)
        at com.ichi2.anki.DeckPicker$UpdateDeckListListener.actualOnPostExecute(DeckPicker.java:2188)
        at com.ichi2.anki.DeckPicker$UpdateDeckListListener.actualOnPostExecute(DeckPicker.java:2154)
        at com.ichi2.async.TaskListenerWithContext.onPostExecute(TaskListenerWithContext.java:66)
        at com.ichi2.async.CollectionTask.onPostExecute(CollectionTask.java:228)
        at android.os.AsyncTask.finish(AsyncTask.java:771)
        at android.os.AsyncTask.access$900(AsyncTask.java:199)
        at android.os.AsyncTask$InternalHandler.handleMessage(AsyncTask.java:788)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loop(Looper.java:223)
        at android.app.ActivityThread.main(ActivityThread.java:7664)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)

getCustomView() returned null

We should probably use DIALOG_LOAD_FAILED instead

It'd be useful to get a regression test here: opening a collection, having all the col json columns blank aside from conf

@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author and removed Needs Second Approval Has one approval, one more approval to merge labels Jan 8, 2022
@github-actions
Copy link
Contributor

Hello 👋, this PR has been opened for more than 2 months with no activity on it. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically

@github-actions github-actions bot added the Stale label Jun 17, 2022
@github-actions github-actions bot closed this Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Author Reply Waiting for a reply from the original author Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Decks.select crash
3 participants