Skip to content

fix: avoid crash by converting lateinit to nullable #17482

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.

Sign up for GitHub

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

Jump to bottom
Merged
lukstbit merged 1 commit into from
Nov 24, 2024
Merged

fix: avoid crash by converting lateinit to nullable #17482

lukstbit merged 1 commit into from
Nov 24, 2024

Conversation

@mikehardy
Copy link
Member

@mikehardy mikehardy commented Nov 22, 2024

Purpose / Description

Avoid a crash seen in ACRA by the evil use of lateinit which is just the sleeper NullPointerException of modern Kotlin times

Fixes

https://ankidroid.org/acra/app/1/bug/260934/report/c4097fac-9c67-460b-986c-017cc1a98313

com.ichi2.anki.exception.UserSubmittedException: lateinit property dropDownDecks has not been initialized
	at com.ichi2.anki.DeckSpinnerSelection.selectDeck(DeckSpinnerSelection.kt:201)
	at com.ichi2.anki.DeckSpinnerSelection.selectDeckById(DeckSpinnerSelection.kt:190)
	at com.ichi2.anki.CardBrowser.setupFlows$onDeckIdChanged(CardBrowser.kt:465)
	at com.ichi2.anki.CardBrowser.access$setupFlows$onDeckIdChanged(CardBrowser.kt:155)
	at com.ichi2.anki.CardBrowser$setupFlows$7.invoke(CardBrowser.kt:570)
	at com.ichi2.anki.CardBrowser$setupFlows$7.invoke(CardBrowser.kt:570)
	at com.ichi2.anki.CardBrowser$launchCollectionInLifecycleScope$1$1.emit(CardBrowser.kt:2509)
	at kotlinx.coroutines.flow.StateFlowImpl.collect(StateFlow.kt:396)
	at com.ichi2.anki.CardBrowser$launchCollectionInLifecycleScope$1.invokeSuspend(CardBrowser.kt:2502)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.internal.DispatchedContinuationKt.resumeCancellableWith(DispatchedContinuation.kt:363)
	at kotlinx.coroutines.intrinsics.CancellableKt.startCoroutineCancellable(Cancellable.kt:26)
	at kotlinx.coroutines.intrinsics.CancellableKt.startCoroutineCancellable$default(Cancellable.kt:21)
	at kotlinx.coroutines.CoroutineStart.invoke(CoroutineStart.kt:88)
	at kotlinx.coroutines.AbstractCoroutine.start(AbstractCoroutine.kt:123)
	at kotlinx.coroutines.BuildersKt__Builders_commonKt.launch(Builders.common.kt:52)
	at kotlinx.coroutines.BuildersKt.launch(Builders.kt:1)
	at kotlinx.coroutines.BuildersKt__Builders_commonKt.launch$default(Builders.common.kt:43)
	at kotlinx.coroutines.BuildersKt.launch$default(Builders.kt:1)
	at com.ichi2.anki.CardBrowser.launchCollectionInLifecycleScope(CardBrowser.kt:2501)
	at com.ichi2.anki.CardBrowser.setupFlows(CardBrowser.kt:570)
	at com.ichi2.anki.CardBrowser.onCreate(CardBrowser.kt:426)
	at android.app.Activity.performCreate(Activity.java:8207)
	at android.app.Activity.performCreate(Activity.java:8191)
	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1309)
	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:3811)
	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:4020)
	at android.app.ActivityThread.handleRelaunchActivityInner(ActivityThread.java:6056)
	at android.app.ActivityThread.handleRelaunchActivity(ActivityThread.java:5951)
	at android.app.servertransaction.ActivityRelaunchItem.execute(ActivityRelaunchItem.java:69)
	at android.app.servertransaction.TransactionExecutor.executeCallbacks(TransactionExecutor.java:135)
	at android.app.servertransaction.TransactionExecutor.execute(TransactionExecutor.java:95)
	at android.app.ClientTransactionHandler.executeTransaction(ClientTransactionHandler.java:63)
	at android.app.ActivityThread.handleRelaunchActivityLocally(ActivityThread.java:6015)
	at android.app.ActivityThread.access$3500(ActivityThread.java:301)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:2338)
	at android.os.Handler.dispatchMessage(Handler.java:106)
	at android.os.Looper.loop(Looper.java:246)
	at android.app.ActivityThread.main(ActivityThread.java:8633)
	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)
	Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [p0{Cancelling}@afaf4a2, Dispatchers.Main.immediate]
  • Fixes #

Approach

There is apparently an init pathway where the flows for CardBrowser start up a DeckSpinner and that can try to select where it should be, before onCollectionLoaded is finished, so the spinner doesn't have all the data it expects because it hasn't actually been initialized fully.

Instead of just blindly expecting itself to be initialized now, the deck list in the spinner is nullable, and if it is null, then the spinner just defaults to all decks / position -1

I assume when it is initialized that the spinner will set itself up correctly later, but most importantly it won't crash now

The diff shouldn't look so bad if ignore space is turned on, because mostly I'm encapsulating the init blocks in let {} to handle the new nullability cleanly

How Has This Been Tested?

open the app with this patch installed, go to card browser and check the spinner is still working, it's still working

Learning (optional, can help others)

Anyone accepting PRs that have lateinit, or !! etc are asking for trouble 🧐

Checklist

Please, go through these checks before submitting the PR.

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • 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

@mikehardy mikehardy added this to the 2.19.3 release milestone Nov 22, 2024
BrayanDSO
BrayanDSO approved these changes Nov 23, 2024
View reviewed changes
Copy link
Member

@BrayanDSO BrayanDSO left a comment

Choose a reason for hiding this comment

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

The code doesn't look pretty, but not crashing is better than crashing

@BrayanDSO BrayanDSO added Needs Second Approval Has one approval, one more approval to merge and removed Needs Review labels Nov 23, 2024
@mikehardy
Copy link
Member Author

mikehardy commented Nov 23, 2024

Yeah, I wasn't sure how to handle it without doing a lot of either if (foo !== null) or similar, but I really just wanted to make it not lateinit so that static typing would ensure I got every single spot it might be racy-not-initialized. Was the best I came up with, using my baby kotlin skills

lukstbit
lukstbit requested changes Nov 23, 2024
View reviewed changes
Copy link
Member

@lukstbit lukstbit left a comment •
edited
Loading

Choose a reason for hiding this comment

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

With the changes in this PR when I go in the card browser and select different decks all I see in the toolbar is the title All Decks(it never changes) and the number of cards of the deck that I actually selected in the dialog.

The whole code around deck selection really needs to be refactored, it's just a mess in its current form.

@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Nov 23, 2024
@BrayanDSO BrayanDSO self-requested a review November 23, 2024 10:00
@BrayanDSO BrayanDSO removed the Needs Second Approval Has one approval, one more approval to merge label Nov 23, 2024
@BrayanDSO
Copy link
Member

BrayanDSO commented Nov 23, 2024

The whole code around deck selection really needs to be refactored

agreed

@mikehardy mikehardy force-pushed the lateinit-deckspinner-fix-maybe branch from 53a71a5 to 539cf5c Compare November 23, 2024 14:37
@mikehardy mikehardy requested a review from lukstbit November 23, 2024 14:38
@mikehardy mikehardy added Review High Priority Request for high priority review and removed Needs Author Reply Waiting for a reply from the original author labels Nov 23, 2024
criticalAY
criticalAY approved these changes Nov 23, 2024
View reviewed changes
Copy link
Contributor

@criticalAY criticalAY left a comment

Choose a reason for hiding this comment

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

LGTM!

@BrayanDSO BrayanDSO added the Needs Second Approval Has one approval, one more approval to merge label Nov 23, 2024
lukstbit
lukstbit approved these changes Nov 24, 2024
View reviewed changes
Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

Everything seems to be in order now!

@lukstbit lukstbit added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Second Approval Has one approval, one more approval to merge labels Nov 24, 2024
@lukstbit lukstbit added this pull request to the merge queue Nov 24, 2024
Merged via the queue into ankidroid:main with commit 16e8561 Nov 24, 2024
9 checks passed
@github-actions github-actions bot removed this from the 2.19.3 release milestone Nov 24, 2024
@github-actions github-actions bot added this to the 2.20 Release milestone Nov 24, 2024
@github-actions github-actions bot removed Review High Priority Request for high priority review Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) labels Nov 24, 2024
@mikehardy mikehardy deleted the lateinit-deckspinner-fix-maybe branch November 25, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@criticalAY criticalAY criticalAY approved these changes

@lukstbit lukstbit lukstbit approved these changes

@BrayanDSO BrayanDSO BrayanDSO approved these changes

@david-allison david-allison Awaiting requested review from david-allison

Assignees

No one assigned

Labels

2.19 Cherry Picked to Stable Branch

Projects

None yet

Milestone

2.19.3 release

Development

Successfully merging this pull request may close these issues.

4 participants

@mikehardy @BrayanDSO @criticalAY @lukstbit