Fix memory leak by stopping server in pageFragment by onDestroy

[DrunkenCloud]

Purpose / Description

This pull request addresses a memory leak that occurred when using the AnkiServer in conjunction with PageFragment. The memory leak was solved by explicitly shutting down the AnkiServer instance in the onDestroyView()

Fixes

• Fixes Im not sure?

Approach

The primary cause of the leak was that the AnkiServer held a direct reference to the PageFragment through the postHandler. The server, intended to run in the background, could outlive the fragment, when it was not shutdown.

To fix this, we took the following steps:

1. Using onDestroy: In the AnkiPackageImporterFragment, the onDestroy function is used to remove the OnBackPressedCallback.

These changes ensure that the PageFragment is no longer kept alive by the AnkiServer, allowing the garbage collector to reclaim the memory.

How Has This Been Tested?

1. LeakCanary: Used to verify that no leaks are detected when navigating to and from PageFragment instances.

Before Fix:
https://drive.google.com/file/d/1FjHFKZtKQ-J6JEIFaicgekwLEqO1HONI/view?usp=drive_link

After Fix (Apologies for splitting it across videos, LeakCanary took some time to decompile a mem dump)
https://drive.google.com/file/d/1FikDQn1mKcwQ9bggxFXjzdHMCZosTCZ8/view?usp=sharing
https://drive.google.com/file/d/1FgU01ggq7DLjnpQSTKO1Z5vn4Z1ndwpT/view?usp=sharing
https://drive.google.com/file/d/1FlOGDmmxR5qjcVq3i8H4jLv8fMYMP8zn/view?usp=sharing

Learning (optional, can help others)

Not particularly anything noteworthy. There are some other memory leaks here and there as it can be seen in the videos so I shall work on those as well

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

[welcome]

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

[lukstbit]

Thanks for contributing!

[lukstbit]

This could potentially crash below were it is used with !!.
Isn't just calling stop() in shutdown() enough to close the server and clear the reference to the fragment?

[DrunkenCloud]

Well originally I thought that postHandler will probably never be null so I just left it but you are right. Changed it back.

[lukstbit]

As the method just calls stop() we can just call stop() directly on the AnkiServer instance in the fragment.

[DrunkenCloud]

Yeah

[lukstbit]

LGTM, thank you!

[mikehardy]

Thanks for the contribution
The PR description no longer matches the actual implementation so this is a bit confusing to review. Have you verified that the leak you detected previously is still addressed with the new + smaller code style?

[DrunkenCloud]

Yes, it would still be fixing the leak. Changed the PR as well, my mistake, I misunderstood how I solved it.

[mikehardy]

Nice! Okay, so I see one last thing that may waste developer time in future:

There is an annotation here:

Anki-Android/AnkiDroid/src/main/java/com/ichi2/anki/pages/PageFragment.kt

Line 41 in 1799693

@Suppress("LeakingThis")

that suppresses LeakingThis, but it appears that your PR here is aimed at that specific leak

Can we remove that annotation now? If you try that does lint still pass when you check it locally? If not, is there some additional code that would allow it to pass (like perhaps onDestroyView setting server = null or something (no idea if that would work)?

These are the lint checks

Anki-Android/.github/workflows/lint.yml

Line 55 in 5e34550

run: ./gradlew lintPlayDebug :api:lintDebug ktLintCheck lintVitalFullRelease lint-rules:test --daemon

If there is no way to remove that Suppress then it should at least receive a comment that it does not in fact leak anymore because the server is shutdown in onDestroyView, but the lint check can't determine that so still flags it

[BrayanDSO]

If you want to remove the deprecation suppresion:

Index: AnkiDroid/src/main/java/com/ichi2/anki/pages/PageFragment.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/java/com/ichi2/anki/pages/PageFragment.kt b/AnkiDroid/src/main/java/com/ichi2/anki/pages/PageFragment.kt
--- a/AnkiDroid/src/main/java/com/ichi2/anki/pages/PageFragment.kt	(revision 6979fd8bf916551a778d075c3f5b1bebbbc70f45)
+++ b/AnkiDroid/src/main/java/com/ichi2/anki/pages/PageFragment.kt	(date 1743604073667)
@@ -38,13 +38,12 @@
 /**
  * Base class for displaying Anki HTML pages
  */
-@Suppress("LeakingThis")
 open class PageFragment(
     @LayoutRes contentLayoutId: Int = R.layout.page_fragment,
 ) : Fragment(contentLayoutId),
     PostRequestHandler {
     lateinit var webView: WebView
-    private val server = AnkiServer(this).also { it.start() }
+    private lateinit var server: AnkiServer
 
     /**
      * A loading indicator for the page. May be shown before the WebView is loaded to
@@ -102,6 +101,7 @@
         view: View,
         savedInstanceState: Bundle?,
     ) {
+        server = AnkiServer(this).also { it.start() }
         val pageWebViewClient = onCreateWebViewClient(savedInstanceState)
         webView =
             view.findViewById<WebView>(R.id.webview).apply {

[DrunkenCloud]

Implemented the suggested changes and submitted the updated PR. The @Suppress("LeakingThis") annotation has been removed, and the server initialization has been adjusted accordingly as given. Let me know if anything else needs tweaking!

[mikehardy]

LGTM - and now with one less Lint suppression, thanks!

[mikehardy]

Looks like we have a new flaky test:

CardBrowserTest > FindReplace - replaces text based on regular expression FAILED
    androidx.test.espresso.NoMatchingViewException: No views in hierarchy found matching: an instance of android.widget.TextView and view.getText() with or without transformation to match: is "⁨1⁩ note updated."

[DrunkenCloud]

Saw that. Im thinking maybe its because I used onDestroyView? I am testing it locally using ./gradlew and changing to onDestroy

[DrunkenCloud]

oh nevermind

[david-allison]

Just a general flake, nothing to do with this PR (thanks for this PR!!)

• To be marked as flaky in test: ignore flaky tests #18198
  • Past efforts: test: ignore flaky tests #18093
  • Cause: feat: Add developer option for find and replace #17897

[david-allison]

Hi there @DrunkenCloud! This is the OpenCollective Notice for PRs merged from 2025-04-01 through 2025-04-30

If you are interested in compensation for this work, the process with details is here: https://github.com/ankidroid/Anki-Android/wiki/OpenCollective-Payment-Process#how-to-get-paid

We only post one comment per person per month to avoid spamming you, regardless of the number of PRs merged, but this note applies to all PRs merged for this month

Please understand that our monthly budget is never guaranteed to cover all claims - the cap on payments-per-person may be lower, but we try to make our process as fair and transparent as possible, we just need your understanding.

Thanks!