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: ignore shared deck download and dialog dismissal at inappropriate lifecycle states #17483

Merged

Conversation

mikehardy
Copy link
Member

Purpose / Description

Fix a crash where the shared deck download state is such that the WebView is attempting to instantiate our shared fragment download listener, but activity/fragment lifecycle state is already saved during teardown process

Fixes

ACRA crash https://ankidroid.org/acra/app/1/bug/260938/report/667f6607-e575-4499-8860-2bb61b5db51b

java.lang.IllegalStateException: Can not perform this action after onSaveInstanceState
	at androidx.fragment.app.FragmentManager.checkStateLoss(FragmentManager.java:1861)
	at androidx.fragment.app.FragmentManager.enqueueAction(FragmentManager.java:1901)
	at androidx.fragment.app.BackStackRecord.commitInternal(BackStackRecord.java:342)
	at androidx.fragment.app.BackStackRecord.commit(BackStackRecord.java:306)
	at com.ichi2.anki.SharedDecksActivity.onCreate$lambda$1(SharedDecksActivity.kt:328)
	at WV.h7.handleMessage(chromium-TrichromeWebViewGoogle6432.aab-stable-672310733:483)
	at android.os.Handler.dispatchMessage(Handler.java:107)
	at android.os.Looper.loopOnce(Looper.java:232)
	at android.os.Looper.loop(Looper.java:317)
	at android.app.ActivityThread.main(ActivityThread.java:8705)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:580)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:886)

Second commit also fixes a bunch of other crashes where dialog dismissal is attempted but state is already saved

Approach

Read up on lifecycle stuff for Fragments, think about how to most-correctly just cut out of the download if for some reason state is saved but the download listener is still called

Couldn't directly reproduce the issue but my reasoning is that we should ignore a download completely if the activity is already in teardown?

Also saw a bunch of others were dialog dismissal was attempted but the exceptions in the crashes were clearly indicating it just wasn't possible, and just cut that attempted dismissal out if state wouldn't permit it

How Has This Been Tested?

Verified shared deck downloads still work with this change in place, so no regression

I tried to alter emulator network settings to open up possibility of triggering the race condition that caused this while simulating a slower / crappier network but couldn't trigger it

Learning (optional, can help others)

Race conditions are fierce

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

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.

Fine for both changes.

To be honest, with the current code around shared decks, I'm surprised with the small amounts of related bug reports that we get.

@lukstbit lukstbit 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

To be honest, with the current code around shared decks, I'm surprised with the small amounts of related bug reports that we get.

After looking at it, I'm not sure why any of the code exists. I don't know how we're doing anything better than just letting the WebView download the thing and then having the user open it ? It's a lot of code and custom UI and stuff for what exactly? If it's the user experience of starting the import immediately when download is complete, couldn't it just do that instead - like a completion listener vs the full custom UI

Anyway, with this it won't crash

@mikehardy mikehardy requested a review from BrayanDSO November 23, 2024 14:24
@david-allison david-allison added this pull request to the merge queue Nov 24, 2024
@david-allison david-allison removed this pull request from the merge queue due to a manual request Nov 24, 2024
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.

@mikehardy You added Brayan as a reviewer, so I'm holding off on the merge button

@david-allison david-allison added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) Needs reviewer reply Waiting for a reply from another reviewer and removed Needs Second Approval Has one approval, one more approval to merge labels Nov 24, 2024
FragmentManager.POP_BACK_STACK_INCLUSIVE
)
// trying to pop fragment manager back state crashes if state already saved
if (!supportFragmentManager.isStateSaved) {
Copy link
Member

Choose a reason for hiding this comment

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

Normally, the callers shouldn't be trying to manually dismiss a dialog when an activity is being destroyed.

Given our need to fix an issue and the current disorder in AnkiActivity, this is reasonable. But, the whole area needs some work in the future.

@BrayanDSO BrayanDSO added this pull request to the merge queue Nov 25, 2024
Merged via the queue into ankidroid:main with commit 83b75bf Nov 25, 2024
12 checks passed
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Nov 25, 2024
@github-actions github-actions bot modified the milestones: 2.19.3 release, 2.20 Release Nov 25, 2024
@mikehardy mikehardy deleted the fix-shareddownload-lifecycle-crash branch November 25, 2024 22:53
@mikehardy mikehardy removed the Needs reviewer reply Waiting for a reply from another reviewer label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants