From f4bef2fe288b1f206e4fe706b8ae11fbd6edd1a1 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 1 Sep 2026 18:15:18 -0700 Subject: [PATCH 01/10] ADFA-3418: Spotless reformat DebuggerViewModel, no functional change Editing this file for the LeakCanary fix enrolls it in the file-level ratchet, which reformats it in full. Committed standalone so the behavioral change that follows stays reviewable. Co-Authored-By: Claude Opus 5 (1M context) --- .../androidide/viewmodel/DebuggerViewModel.kt | 219 ++++++++++-------- 1 file changed, 118 insertions(+), 101 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/viewmodel/DebuggerViewModel.kt b/app/src/main/java/com/itsaky/androidide/viewmodel/DebuggerViewModel.kt index 731cc9a9bf..3022ebdda1 100644 --- a/app/src/main/java/com/itsaky/androidide/viewmodel/DebuggerViewModel.kt +++ b/app/src/main/java/com/itsaky/androidide/viewmodel/DebuggerViewModel.kt @@ -113,59 +113,66 @@ class DebuggerViewModel : ViewModel() { _debugeePackage.update { value } } - val allThreads = state.map { - logger.debug("Updating all threads") - it.threads - }.stateIn( - scope = viewModelScope, - started = SharingStarted.Eagerly, - initialValue = emptyList(), - ) - - val selectedThread = state - .map { state -> - state.selectedThread to state.threadIndex - }.stateIn( - scope = viewModelScope, - started = SharingStarted.Eagerly, - initialValue = null to -1, - ) + val allThreads = + state + .map { + logger.debug("Updating all threads") + it.threads + }.stateIn( + scope = viewModelScope, + started = SharingStarted.Eagerly, + initialValue = emptyList(), + ) - val allFrames = selectedThread - .map { (thread, _) -> - thread?.getFrames() ?: emptyList() - }.stateIn( - scope = viewModelScope, - started = SharingStarted.Eagerly, - initialValue = emptyList(), - ) + val selectedThread = + state + .map { state -> + state.selectedThread to state.threadIndex + }.stateIn( + scope = viewModelScope, + started = SharingStarted.Eagerly, + initialValue = null to -1, + ) - val selectedFrame = state - .map { state -> - state.selectedFrame() to state.frameIndex - }.stateIn( - scope = viewModelScope, - started = SharingStarted.Eagerly, - initialValue = null to -1, - ) + val allFrames = + selectedThread + .map { (thread, _) -> + thread?.getFrames() ?: emptyList() + }.stateIn( + scope = viewModelScope, + started = SharingStarted.Eagerly, + initialValue = emptyList(), + ) - val selectedFrameVariables = selectedFrame - .map { (frame, _) -> - frame?.getVariables() ?: emptyList() - }.stateIn( - scope = viewModelScope, - started = SharingStarted.Eagerly, - initialValue = emptyList(), - ) + val selectedFrame = + state + .map { state -> + state.selectedFrame() to state.frameIndex + }.stateIn( + scope = viewModelScope, + started = SharingStarted.Eagerly, + initialValue = null to -1, + ) - val variablesTree = state - .map { state -> - state.variablesTree - }.stateIn( - scope = viewModelScope, - started = SharingStarted.Eagerly, - initialValue = DebuggerState.DEFAULT.variablesTree, - ) + val selectedFrameVariables = + selectedFrame + .map { (frame, _) -> + frame?.getVariables() ?: emptyList() + }.stateIn( + scope = viewModelScope, + started = SharingStarted.Eagerly, + initialValue = emptyList(), + ) + + val variablesTree = + state + .map { state -> + state.variablesTree + }.stateIn( + scope = viewModelScope, + started = SharingStarted.Eagerly, + initialValue = DebuggerState.DEFAULT.variablesTree, + ) override fun onCleared() { super.onCleared() @@ -176,14 +183,17 @@ class DebuggerViewModel : ViewModel() { _connectionState.update { state } } - private fun setDebuggerState(newState: DebuggerState, because: String) { + private fun setDebuggerState( + newState: DebuggerState, + because: String, + ) { logger.debug("Updating debugger state because {}: {}", because, newState) state.update { newState } } private suspend inline fun setDebuggerState( because: String, - crossinline newState: suspend (DebuggerState) -> DebuggerState + crossinline newState: suspend (DebuggerState) -> DebuggerState, ) { val currentState = state.value val newState = newState(currentState) @@ -229,13 +239,13 @@ class DebuggerViewModel : ViewModel() { suspend fun setThreads( threads: List, selectedThreadIndex: Int = -1, - selectedFrameIndex: Int = -1 + selectedFrameIndex: Int = -1, ) { logger.debug( "setThreads(selectedThreadIndex={}, selectedFrameIndex={}, threads={})", selectedThreadIndex, selectedFrameIndex, - threads + threads, ) withContext(Dispatchers.IO) { @@ -270,11 +280,16 @@ class DebuggerViewModel : ViewModel() { var frameIndex = selectedFrameIndex if (frameIndex < 0) { - frameIndex = if (resolvableThreads - .getOrNull(threadIndex) - ?.getFrames() - ?.firstOrNull() != null - ) 0 else -1 + frameIndex = + if (resolvableThreads + .getOrNull(threadIndex) + ?.getFrames() + ?.firstOrNull() != null + ) { + 0 + } else { + -1 + } } DebuggerState( @@ -343,38 +358,39 @@ class DebuggerViewModel : ViewModel() { } } - suspend fun setSelectedThreadIndex(index: Int) = withContext(Dispatchers.IO) { - setDebuggerState(because = "selected thread index changed") { current -> - check(index in 0.. + check(index in 0.. - check(index in 0..<(current.selectedThread?.getFrames()?.size ?: 0)) { - "Invalid frame index: $index" - } + suspend fun setSelectedFrameIndex(index: Int) = + withContext(Dispatchers.IO) { + setDebuggerState(because = "selected frame index changed") { current -> + check(index in 0..<(current.selectedThread?.getFrames()?.size ?: 0)) { + "Invalid frame index: $index" + } - current.copy( - frameIndex = index, - variablesTree = - createVariablesTree( - current.threads, - current.threadIndex, - index, - ), - ) + current.copy( + frameIndex = index, + variablesTree = + createVariablesTree( + current.threads, + current.threadIndex, + index, + ), + ) + } } - } @OptIn(ExperimentalStdlibApi::class) fun observeLatestSelectedFrame( From 24c88a28961eeee12444afe7ea2153b850fdd381 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 1 Sep 2026 18:16:16 -0700 Subject: [PATCH 02/10] ADFA-3418: Fix two memory leaks found with LeakCanary Both were confirmed on a Galaxy Note 20 Ultra (Android 13) by opening and closing a project seven times and reading the heap analysis. EditorActivity leaked ~1 MB per open/close, without bound. AdbMdns took NsdManager from the caller's Context, and the caller is the editor activity (BaseEditorActivity -> WADBConnectionViewModel.start). NsdManager is cached per-Context and holds mContext for its lifetime, while the framework pins its NsdCallbackImpl from a native global ref, so nothing released it - the existing stop() in preDestroy stops discovery but cannot help. Taking the manager from the application context fixes all three call sites at once: the view model, AdbPairingService and BootCompleteReceiver. Before: 4 leaked activities, 4,076,219 bytes. After: no NsdManager in any trace across seven cycles. DebuggerViewModel leaked through EventBus. IDEDebugClientImpl calls register() in its init block but onCleared() only unregistered from Lookup, so the default EventBus kept the client and through it the view model. Before: 5 instances, 7,290 bytes. After: watched seven times, never in a trace. EditorActivityLifecyclerObserver was the only other EventReceiver and is already balanced across onStart/onStop. One Activity leak remains, unfixed and unrelated to these: static ActionsRegistry holds ShowTooltipAction, whose context is the editor activity, because both clear() and clearActions() deliberately skip EDITOR_TEXT_ACTIONS so language-server actions survive. It is bounded at one activity (~713 KB, flat across dumps) rather than accumulating. Co-Authored-By: Claude Opus 5 (1M context) --- .../com/itsaky/androidide/viewmodel/DebuggerViewModel.kt | 3 +++ .../src/main/java/moe/shizuku/manager/adb/AdbMdns.kt | 6 +++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/itsaky/androidide/viewmodel/DebuggerViewModel.kt b/app/src/main/java/com/itsaky/androidide/viewmodel/DebuggerViewModel.kt index 3022ebdda1..65962c38e1 100644 --- a/app/src/main/java/com/itsaky/androidide/viewmodel/DebuggerViewModel.kt +++ b/app/src/main/java/com/itsaky/androidide/viewmodel/DebuggerViewModel.kt @@ -177,6 +177,9 @@ class DebuggerViewModel : ViewModel() { override fun onCleared() { super.onCleared() Lookup.getDefault().unregister(IDEDebugClientImpl::class.java) + // The client registered itself with EventBus in its init block; without this the default + // EventBus keeps it, and through it this view model, for the life of the process. + debugClient.unregister() } fun setConnectionState(state: DebuggerConnectionState) { diff --git a/subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbMdns.kt b/subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbMdns.kt index 9aac9a38a1..7ec690c8f0 100644 --- a/subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbMdns.kt +++ b/subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbMdns.kt @@ -20,7 +20,11 @@ class AdbMdns( ) { constructor(context: Context, serviceType: String, observer: Consumer) : this( - nsdManager = context.getSystemService(NsdManager::class.java), + // Application context, not the caller's: NsdManager is cached per-Context and keeps a + // strong mContext reference, while the framework holds its NsdCallbackImpl from a native + // global ref for the manager's lifetime. Obtaining it from an Activity leaks that + // Activity for the life of the process, and stopServiceDiscovery() does not release it. + nsdManager = context.applicationContext.getSystemService(NsdManager::class.java), serviceType = serviceType, observer = observer, ) From 967739f0a4a067376d92bfdff34b0b208a9d06c0 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 1 Sep 2026 18:19:10 -0700 Subject: [PATCH 03/10] ADFA-3418: Spotless reformat ShowTooltipAction, no functional change The file used four-space indentation, so touching it for the leak fix pulls the whole file under the ratchet. Committed standalone. Co-Authored-By: Claude Opus 5 (1M context) --- .../actions/file/ShowTooltipAction.kt | 153 +++++++++--------- 1 file changed, 77 insertions(+), 76 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt index 5b6d8779ff..ed1a4f1465 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt @@ -30,85 +30,86 @@ import com.itsaky.androidide.idetooltips.TooltipCategory import com.itsaky.androidide.idetooltips.TooltipManager import com.itsaky.androidide.idetooltips.TooltipTag -class ShowTooltipAction(private val context: Context, override val order: Int) : - BaseEditorAction() { - - companion object { - const val ID = "ide.editor.code.text.show_tooltip" - } - - override val id: String = ID - override var location: ActionItem.Location = ActionItem.Location.EDITOR_TEXT_ACTIONS - - init { - label = context.getString(R.string.title_show_tooltip) - val drawable = ContextCompat.getDrawable(context, R.drawable.ic_action_help_outlined) - icon = drawable?.let { tintDrawable(context, it) } - } - - override fun prepare(data: ActionData) { - super.prepare(data) - if (!visible) return - - val target = getTextTarget(data) - visible = target != null - enabled = visible - } - - override suspend fun execAction(data: ActionData): Boolean { - val target = getTextTarget(data) ?: return false - val anchorView = target.getAnchorView() ?: return false - val editor = getEditor(data) - - val categoryAndTag = - if (editor != null) { - val category = tooltipCategoryForExtension(editor.file?.extension) - resolveTooltipTag( - category = category, - selectedText = target.getSelectedText(), - editorTag = editor.tag?.toString(), - isXmlAttribute = category == TooltipCategory.CATEGORY_XML && editor.isXmlAttribute(), - ).let { tag -> category to tag } - } else { - TooltipCategory.CATEGORY_IDE to TooltipTag.DIALOG_FIND_IN_PROJECT - } - val (category, tag) = categoryAndTag - - if (tag.isEmpty()) return false - - TooltipManager.showTooltip( - context = anchorView.context, - anchorView = anchorView, - category = category, - tag = tag, - ) - - return true - } - - override fun retrieveTooltipTag(isAlternateContext: Boolean) = TooltipTag.EDITOR_TOOLBAR_HELP +class ShowTooltipAction( + private val context: Context, + override val order: Int, +) : BaseEditorAction() { + companion object { + const val ID = "ide.editor.code.text.show_tooltip" + } + + override val id: String = ID + override var location: ActionItem.Location = ActionItem.Location.EDITOR_TEXT_ACTIONS + + init { + label = context.getString(R.string.title_show_tooltip) + val drawable = ContextCompat.getDrawable(context, R.drawable.ic_action_help_outlined) + icon = drawable?.let { tintDrawable(context, it) } + } + + override fun prepare(data: ActionData) { + super.prepare(data) + if (!visible) return + + val target = getTextTarget(data) + visible = target != null + enabled = visible + } + + override suspend fun execAction(data: ActionData): Boolean { + val target = getTextTarget(data) ?: return false + val anchorView = target.getAnchorView() ?: return false + val editor = getEditor(data) + + val categoryAndTag = + if (editor != null) { + val category = tooltipCategoryForExtension(editor.file?.extension) + resolveTooltipTag( + category = category, + selectedText = target.getSelectedText(), + editorTag = editor.tag?.toString(), + isXmlAttribute = category == TooltipCategory.CATEGORY_XML && editor.isXmlAttribute(), + ).let { tag -> category to tag } + } else { + TooltipCategory.CATEGORY_IDE to TooltipTag.DIALOG_FIND_IN_PROJECT + } + val (category, tag) = categoryAndTag + + if (tag.isEmpty()) return false + + TooltipManager.showTooltip( + context = anchorView.context, + anchorView = anchorView, + category = category, + tag = tag, + ) + + return true + } + + override fun retrieveTooltipTag(isAlternateContext: Boolean) = TooltipTag.EDITOR_TOOLBAR_HELP } internal fun tooltipCategoryForExtension(extension: String?): String = - when (extension) { - "java" -> TooltipCategory.CATEGORY_JAVA - "kt" -> TooltipCategory.CATEGORY_KOTLIN - "xml" -> TooltipCategory.CATEGORY_XML - else -> TooltipCategory.CATEGORY_IDE - } + when (extension) { + "java" -> TooltipCategory.CATEGORY_JAVA + "kt" -> TooltipCategory.CATEGORY_KOTLIN + "xml" -> TooltipCategory.CATEGORY_XML + else -> TooltipCategory.CATEGORY_IDE + } internal fun resolveTooltipTag( - category: String, - selectedText: String?, - editorTag: String?, - isXmlAttribute: Boolean, + category: String, + selectedText: String?, + editorTag: String?, + isXmlAttribute: Boolean, ): String { - val textToUse = selectedText ?: "" - return when { - !editorTag.isNullOrEmpty() -> editorTag - category == TooltipCategory.CATEGORY_XML && isXmlAttribute -> textToUse.substringAfterLast(":") - category == TooltipCategory.CATEGORY_KOTLIN && isKotlinOperatorToken(textToUse) -> "kotlin.operator.$textToUse" - category == TooltipCategory.CATEGORY_JAVA && isJavaOperatorToken(textToUse) -> "java.operator.$textToUse" - else -> textToUse - } -} \ No newline at end of file + val textToUse = selectedText ?: "" + return when { + !editorTag.isNullOrEmpty() -> editorTag + category == TooltipCategory.CATEGORY_XML && isXmlAttribute -> textToUse.substringAfterLast(":") + category == TooltipCategory.CATEGORY_KOTLIN && isKotlinOperatorToken(textToUse) -> "kotlin.operator.$textToUse" + category == TooltipCategory.CATEGORY_JAVA && isJavaOperatorToken(textToUse) -> "java.operator.$textToUse" + else -> textToUse + } +} From 51aed758e2247b427eab91996aa72f09997056c5 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 1 Sep 2026 18:22:33 -0700 Subject: [PATCH 04/10] ADFA-3418: Stop ShowTooltipAction retaining the editor activity The static ActionsRegistry holds every action for the life of the process, and EDITOR_TEXT_ACTIONS is deliberately never cleared so language-server actions survive - so an action that stores its Context keeps that activity alive after onDestroy. ShowTooltipAction was the only one of the eight editor text actions declaring `private val context`; the other seven take the parameter and drop it. It did not need to keep it either: the context is read only in init, for the label and icon, while execAction uses anchorView.context. Verified on a Galaxy Note 20 Ultra (Android 13) with the same seven open/close cycles used for the other two leaks. Before: 1 leaked EditorActivity, 713,292 bytes, signature a4098715. After: seven activities watched, zero retained, no heap dump triggered - and the build immediately before this one reported the leak twice under the identical loop, so the loop does detect it when present. Co-Authored-By: Claude Opus 5 (1M context) --- .../com/itsaky/androidide/actions/file/ShowTooltipAction.kt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt index ed1a4f1465..225d0a1e8b 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt @@ -30,8 +30,11 @@ import com.itsaky.androidide.idetooltips.TooltipCategory import com.itsaky.androidide.idetooltips.TooltipManager import com.itsaky.androidide.idetooltips.TooltipTag +// The context is used only to build the label and icon below; it is deliberately not stored. +// EDITOR_TEXT_ACTIONS is never cleared (see EditorActivityActions.clear), so the static +// ActionsRegistry outlives every editor activity - retaining one here leaks that activity. class ShowTooltipAction( - private val context: Context, + context: Context, override val order: Int, ) : BaseEditorAction() { companion object { From 97b2df4efde6516f66509695593529197df6062c Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 1 Sep 2026 18:50:55 -0700 Subject: [PATCH 05/10] ADFA-3418: Spotless reformat WADBConnectionViewModel, no functional change Blank lines between when branches, added by ktlint once the file entered the ratchet. Committed standalone ahead of the behavioral change. Co-Authored-By: Claude Opus 5 (1M context) --- .../com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/app/src/main/java/com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt b/app/src/main/java/com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt index 398b413df0..095d74c71e 100644 --- a/app/src/main/java/com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt +++ b/app/src/main/java/com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt @@ -191,6 +191,7 @@ class WADBConnectionViewModel : ViewModel() { AdbPairingService.ACTION_PAIR_STARTED -> { _status.update { ConnectionStatus.Pairing } } + AdbPairingService.ACTION_PAIR_SUCCEEDED -> { _status.update { ConnectionStatus.Paired } From 2f6d9f9030525dbed0f84ddaf2f25f66c144b947 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 1 Sep 2026 18:54:07 -0700 Subject: [PATCH 06/10] ADFA-3418: Apply the NsdManager fix at the call site, not in AdbMdns Forcing the application context inside AdbMdns's constructor made every AdbMdns in the process share one cached NsdManager. Before that change the editor activity and AdbPairingService held two distinct instances, so they were two distinct clients of NsdService; merging them risks the pairing and connect discoveries colliding on the one-resolve-at-a-time limit that NsdManager has below Android 13, where ResolveListener.onResolveFailed is an empty body and the failure would be silent. The editor activity was the only leaking caller - AdbPairingService and BootCompleteReceiver already pass contexts that outlive the object - so passing applicationContext from WADBConnectionViewModel fixes the leak and keeps the two managers separate. AdbMdns documents the requirement instead of enforcing it. Also corrects the explanation. The previous comment named NsdCallbackImpl, which is the Android 13 shape seen in the test device's leak trace but does not exist on API 30-32, the rest of this @RequiresApi(R) class's range. The version-independent fact is the one that matters: getSystemService caches NsdManager per Context and the manager holds that Context for its own lifetime. Re-verified after the change, same seven open/close cycles: seven activities watched, zero retained, no NsdManager in any trace. Converts the ShowTooltipAction comment block to KDoc on the public class, per review. Co-Authored-By: Claude Opus 5 (1M context) --- .../androidide/actions/file/ShowTooltipAction.kt | 10 +++++++--- .../viewmodel/WADBConnectionViewModel.kt | 4 +++- .../main/java/moe/shizuku/manager/adb/AdbMdns.kt | 16 +++++++++++----- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt index 225d0a1e8b..3dcd2c31ad 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt @@ -30,9 +30,13 @@ import com.itsaky.androidide.idetooltips.TooltipCategory import com.itsaky.androidide.idetooltips.TooltipManager import com.itsaky.androidide.idetooltips.TooltipTag -// The context is used only to build the label and icon below; it is deliberately not stored. -// EDITOR_TEXT_ACTIONS is never cleared (see EditorActivityActions.clear), so the static -// ActionsRegistry outlives every editor activity - retaining one here leaks that activity. +/** + * Editor text action that shows the tooltip for the current selection. + * + * [context] initializes [label] and [icon] only and must not be retained: EDITOR_TEXT_ACTIONS is + * never cleared (see EditorActivityActions.clear), so the static ActionsRegistry outlives every + * editor activity, and an action holding one leaks it. [execAction] uses the anchor view's context. + */ class ShowTooltipAction( context: Context, override val order: Int, diff --git a/app/src/main/java/com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt b/app/src/main/java/com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt index 095d74c71e..8c75753e62 100644 --- a/app/src/main/java/com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt +++ b/app/src/main/java/com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt @@ -151,7 +151,9 @@ class WADBConnectionViewModel : ViewModel() { if (_adbMdnsConnector == null) { _adbMdnsConnector = AdbMdns( - context = context, + // The caller is the editor activity, and AdbMdns's NsdManager outlives it + // while holding whatever Context it was created from - see AdbMdns. + context = context.applicationContext, serviceType = AdbMdns.TLS_CONNECT, observer = adbConnectListener, ) diff --git a/subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbMdns.kt b/subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbMdns.kt index 7ec690c8f0..0bbbddebcf 100644 --- a/subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbMdns.kt +++ b/subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbMdns.kt @@ -18,13 +18,19 @@ class AdbMdns( private val serviceType: String, private val observer: Consumer, ) { + /** + * Pass a [context] that outlives this object - an Application or a Service, never an Activity. + * getSystemService caches NsdManager per Context, and the manager holds that Context for its + * own lifetime, which the framework keeps alive; stopServiceDiscovery() does not release it. + * An Activity passed here is therefore retained for the life of the process, so a caller + * holding only an Activity should pass its applicationContext. + * + * The application context is deliberately not forced here: each Context gets its own + * NsdManager, and the pairing and connect discoveries want to stay separate clients. + */ constructor(context: Context, serviceType: String, observer: Consumer) : this( - // Application context, not the caller's: NsdManager is cached per-Context and keeps a - // strong mContext reference, while the framework holds its NsdCallbackImpl from a native - // global ref for the manager's lifetime. Obtaining it from an Activity leaks that - // Activity for the life of the process, and stopServiceDiscovery() does not release it. - nsdManager = context.applicationContext.getSystemService(NsdManager::class.java), + nsdManager = context.getSystemService(NsdManager::class.java), serviceType = serviceType, observer = observer, ) From 6e96aadd95ce865c4710675e1d78916da6f30cce Mon Sep 17 00:00:00 2001 From: David Schachter Date: Tue, 1 Sep 2026 23:04:19 -0700 Subject: [PATCH 07/10] ADFA-3418: Enforce the NsdManager context rule instead of documenting it The previous commit moved the fix to the call site and documented the rule in AdbMdns's KDoc: pass an Application or a Service, never an Activity. That was wrong twice over, and review caught both. A Service is not safe. AdbPairingService passes `this` and calls stopSelf, and a Service has its own ContextImpl whose mOuterContext is the Service, so by the same mechanism as the Activity it is stranded for the life of the process - one per pairing session. The commit message asserted these callers "already pass contexts that outlive the object" without checking. Documenting an invariant also left it unenforced, and its first caller already violated it. So AdbMdns now derives the manager itself: applicationContext, then createAttributionContext(null), then getSystemService. Nothing a caller passes can be retained, and because the service cache is per-ContextImpl (ContextImpl.java, mServiceCache) and createAttributionContext builds a new one, each AdbMdns stays a distinct NsdService client. That preserves the separation the previous commit wanted - two concurrent resolves on one manager fail with FAILURE_ALREADY_ACTIVE below Android 13 - which the previous shape only achieved for the pairing service, since a broadcast context's getSystemService delegates to the application ContextImpl and so gave BootCompleteReceiver the same manager as the view model. createAttributionContext is API 30, matching this class's @RequiresApi(R). ResolveListener.onResolveFailed had an empty body, so every resolve failure was silent - no port delivered, caller times out with nothing logged. It now logs, including the error code that identifies the collision above. Adds the regression test for the ShowTooltipAction fix that this branch lacked: reflection over the EDITOR_TEXT_ACTIONS classes asserting none declares a Context field. It covers the whole bucket rather than the one action that leaked, and it fails as `[ShowTooltipAction.context]` with the fix reverted. The equivalent test for the EventBus fix is not here: constructing DebuggerViewModel touches android.util.SparseArray through the tree-view library, and Robolectric's sandbox does not take effect in this module's JUnit Platform setup. Recorded in ADFA-5382. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HeQ4YGq9oFjNMBPojB4MY1 --- .../viewmodel/WADBConnectionViewModel.kt | 4 +- .../actions/EditorTextActionContextTest.kt | 50 +++++++++++++++++++ .../java/moe/shizuku/manager/adb/AdbMdns.kt | 27 ++++++---- 3 files changed, 68 insertions(+), 13 deletions(-) create mode 100644 app/src/test/java/com/itsaky/androidide/actions/EditorTextActionContextTest.kt diff --git a/app/src/main/java/com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt b/app/src/main/java/com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt index 8c75753e62..095d74c71e 100644 --- a/app/src/main/java/com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt +++ b/app/src/main/java/com/itsaky/androidide/viewmodel/WADBConnectionViewModel.kt @@ -151,9 +151,7 @@ class WADBConnectionViewModel : ViewModel() { if (_adbMdnsConnector == null) { _adbMdnsConnector = AdbMdns( - // The caller is the editor activity, and AdbMdns's NsdManager outlives it - // while holding whatever Context it was created from - see AdbMdns. - context = context.applicationContext, + context = context, serviceType = AdbMdns.TLS_CONNECT, observer = adbConnectListener, ) diff --git a/app/src/test/java/com/itsaky/androidide/actions/EditorTextActionContextTest.kt b/app/src/test/java/com/itsaky/androidide/actions/EditorTextActionContextTest.kt new file mode 100644 index 0000000000..f498ad65e7 --- /dev/null +++ b/app/src/test/java/com/itsaky/androidide/actions/EditorTextActionContextTest.kt @@ -0,0 +1,50 @@ +package com.itsaky.androidide.actions + +import android.content.Context +import com.google.common.truth.Truth.assertThat +import com.itsaky.androidide.actions.editor.CopyAction +import com.itsaky.androidide.actions.editor.CutAction +import com.itsaky.androidide.actions.editor.ExpandSelectionAction +import com.itsaky.androidide.actions.editor.LongSelectAction +import com.itsaky.androidide.actions.editor.PasteAction +import com.itsaky.androidide.actions.editor.SelectAllAction +import com.itsaky.androidide.actions.file.FormatCodeAction +import com.itsaky.androidide.actions.file.ShowTooltipAction +import org.junit.Test + +/** + * `ActionsRegistry` is a process-lifetime singleton and EDITOR_TEXT_ACTIONS is deliberately never + * cleared, so language-server actions survive - see `EditorActivityActions.clear`. An action in that + * bucket that stores its `Context` therefore keeps one editor activity alive for the whole process, + * which is what `ShowTooltipAction` did. + * + * Reflection rather than a heap assertion: it names the mistake directly, costs nothing, and covers + * every action in the bucket instead of the one that happened to leak. Add new EDITOR_TEXT_ACTIONS + * entries here. + */ +class EditorTextActionContextTest { + private val editorTextActions = + listOf( + ExpandSelectionAction::class.java, + SelectAllAction::class.java, + LongSelectAction::class.java, + CutAction::class.java, + CopyAction::class.java, + PasteAction::class.java, + FormatCodeAction::class.java, + ShowTooltipAction::class.java, + ) + + @Test + fun givenTheEditorTextActions_whenInspected_thenNoneDeclaresAContextField() { + val offenders = + editorTextActions + .flatMap { action -> + action.declaredFields + .filter { Context::class.java.isAssignableFrom(it.type) } + .map { "${action.simpleName}.${it.name}" } + } + + assertThat(offenders).isEmpty() + } +} diff --git a/subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbMdns.kt b/subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbMdns.kt index 0bbbddebcf..e4719d3464 100644 --- a/subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbMdns.kt +++ b/subprojects/shizuku-manager/src/main/java/moe/shizuku/manager/adb/AdbMdns.kt @@ -19,18 +19,21 @@ class AdbMdns( private val observer: Consumer, ) { /** - * Pass a [context] that outlives this object - an Application or a Service, never an Activity. - * getSystemService caches NsdManager per Context, and the manager holds that Context for its - * own lifetime, which the framework keeps alive; stopServiceDiscovery() does not release it. - * An Activity passed here is therefore retained for the life of the process, so a caller - * holding only an Activity should pass its applicationContext. + * Takes an NsdManager that neither retains [context] nor is shared with another [AdbMdns]. * - * The application context is deliberately not forced here: each Context gets its own - * NsdManager, and the pairing and connect discoveries want to stay separate clients. + * getSystemService caches NsdManager per Context, and the manager holds that Context for its + * own lifetime while the framework keeps the manager alive, so one taken from an Activity or a + * Service retains it for the whole process; stopServiceDiscovery() does not release it. A + * fresh attribution context off the application context avoids that, and since the cache is + * per-ContextImpl it also keeps each instance a separate NsdService client - which matters + * below Android 13, where a second concurrent resolveService fails with FAILURE_ALREADY_ACTIVE. */ constructor(context: Context, serviceType: String, observer: Consumer) : this( - nsdManager = context.getSystemService(NsdManager::class.java), + nsdManager = + context.applicationContext + .createAttributionContext(null) + .getSystemService(NsdManager::class.java), serviceType = serviceType, observer = observer, ) @@ -151,8 +154,12 @@ class AdbMdns( ) : NsdManager.ResolveListener { override fun onResolveFailed( nsdServiceInfo: NsdServiceInfo, - i: Int, - ) {} + errorCode: Int, + ) { + // An empty body here hid every resolve failure: no port is delivered and the caller + // just times out. FAILURE_ALREADY_ACTIVE (3) means two resolves raced on one manager. + Log.w(TAG, "onResolveFailed: ${nsdServiceInfo.serviceName}, $errorCode") + } override fun onServiceResolved(nsdServiceInfo: NsdServiceInfo) { adbMdns.onServiceResolved(nsdServiceInfo) From 41f7d1f089d53742ff1bc3e330460c2c9b5d220b Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 2 Sep 2026 10:24:11 -0700 Subject: [PATCH 08/10] ADFA-3418: Use @param for the ShowTooltipAction context doc [context] does not resolve as a KDoc link now that it is a plain constructor parameter rather than a property, so rendered docs showed it unlinked. Per review. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HeQ4YGq9oFjNMBPojB4MY1 --- .../itsaky/androidide/actions/file/ShowTooltipAction.kt | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt b/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt index 3dcd2c31ad..c2a99585d7 100644 --- a/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt +++ b/app/src/main/java/com/itsaky/androidide/actions/file/ShowTooltipAction.kt @@ -33,9 +33,11 @@ import com.itsaky.androidide.idetooltips.TooltipTag /** * Editor text action that shows the tooltip for the current selection. * - * [context] initializes [label] and [icon] only and must not be retained: EDITOR_TEXT_ACTIONS is - * never cleared (see EditorActivityActions.clear), so the static ActionsRegistry outlives every - * editor activity, and an action holding one leaks it. [execAction] uses the anchor view's context. + * EDITOR_TEXT_ACTIONS is never cleared (see EditorActivityActions.clear), so the static + * ActionsRegistry outlives every editor activity and an action holding a Context leaks it. + * [execAction] uses the anchor view's context instead. + * + * @param context initializes [label] and [icon] only; deliberately not retained. */ class ShowTooltipAction( context: Context, From 16495ef0b8ea4c195846f80e4d8077d4d039a026 Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 2 Sep 2026 18:30:47 -0700 Subject: [PATCH 09/10] ADFA-3418: Inspect inherited fields in the editor action Context test Review caught that the test read only Class.getDeclaredFields(), which stops at the class itself. A Context held by a shared base class - the more likely hiding place than a copy in each action - would have passed unnoticed, so the test could have gone green while the leak it exists to catch was present. It now walks the superclass chain, and reports the declaring class in the failure message so an inherited offender is not attributed to the wrong action. Adds a second test that pins that behaviour with a fixture whose Context sits on a superclass. Verified it fails with declaredFields alone and passes with the walk, so the fix has a test rather than an assertion about itself. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011Crj29d6Q2DGjioWPxtuSR --- .../actions/EditorTextActionContextTest.kt | 35 +++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/app/src/test/java/com/itsaky/androidide/actions/EditorTextActionContextTest.kt b/app/src/test/java/com/itsaky/androidide/actions/EditorTextActionContextTest.kt index f498ad65e7..158a5be129 100644 --- a/app/src/test/java/com/itsaky/androidide/actions/EditorTextActionContextTest.kt +++ b/app/src/test/java/com/itsaky/androidide/actions/EditorTextActionContextTest.kt @@ -11,6 +11,7 @@ import com.itsaky.androidide.actions.editor.SelectAllAction import com.itsaky.androidide.actions.file.FormatCodeAction import com.itsaky.androidide.actions.file.ShowTooltipAction import org.junit.Test +import java.lang.reflect.Field /** * `ActionsRegistry` is a process-lifetime singleton and EDITOR_TEXT_ACTIONS is deliberately never @@ -40,11 +41,41 @@ class EditorTextActionContextTest { val offenders = editorTextActions .flatMap { action -> - action.declaredFields + action + .fieldsIncludingInherited() .filter { Context::class.java.isAssignableFrom(it.type) } - .map { "${action.simpleName}.${it.name}" } + .map { "${action.simpleName} (from ${it.declaringClass.simpleName}).${it.name}" } } assertThat(offenders).isEmpty() } + + @Test + fun givenAContextFieldOnASuperclass_whenInspected_thenItIsStillFound() { + // Pins the reason this walks the hierarchy: with declaredFields alone this finds nothing, so + // an action inheriting a Context would have passed the test above. + val found = + ActionInheritingAContext::class.java + .fieldsIncludingInherited() + .filter { Context::class.java.isAssignableFrom(it.type) } + + assertThat(found).isNotEmpty() + } + + private open class BaseHoldingAContext { + @Suppress("unused") + private val context: Context? = null + } + + private class ActionInheritingAContext : BaseHoldingAContext() + + /** + * Declared fields of this class and of every superclass. [Class.getDeclaredFields] stops at the + * class itself, so a Context held by a shared base class - the more likely place for one to hide + * than in each action - would pass unnoticed. + */ + private fun Class<*>.fieldsIncludingInherited(): List = + generateSequence(this) { it.superclass } + .flatMap { it.declaredFields.asSequence() } + .toList() } From 75ae7d36daaa725a54e96309bfd700e582cfe0fe Mon Sep 17 00:00:00 2001 From: David Schachter Date: Wed, 2 Sep 2026 18:48:24 -0700 Subject: [PATCH 10/10] ADFA-3418: Cover the ninth EDITOR_TEXT_ACTIONS entry, and stop overclaiming Two corrections to this test, both mine. CodeActionsMenu also declares location = EDITOR_TEXT_ACTIONS, so the bucket has nine entries, not eight. It is a Kotlin object - process lifetime - which makes it the most dangerous entry in the bucket rather than one to leave out. Added. The KDoc claimed the test "covers every action in the bucket instead of the one that happened to leak". That was false in three ways, now written down instead of asserted away: the list is hand-maintained and drifts as soon as someone registers a tenth action; the LSP actions that LSPEditorActions nests under CodeActionsMenu.children at runtime are not reachable from a static list at all; and a Context held by a companion object or captured by a lambda in a field lives on a synthetic class, so a field-type check does not see it. The test is still worth having - it catches the spelling of the mistake that actually happened - but a green run means less than the old comment implied. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011Crj29d6Q2DGjioWPxtuSR --- .../actions/EditorTextActionContextTest.kt | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/app/src/test/java/com/itsaky/androidide/actions/EditorTextActionContextTest.kt b/app/src/test/java/com/itsaky/androidide/actions/EditorTextActionContextTest.kt index 158a5be129..2c54417ded 100644 --- a/app/src/test/java/com/itsaky/androidide/actions/EditorTextActionContextTest.kt +++ b/app/src/test/java/com/itsaky/androidide/actions/EditorTextActionContextTest.kt @@ -10,6 +10,7 @@ import com.itsaky.androidide.actions.editor.PasteAction import com.itsaky.androidide.actions.editor.SelectAllAction import com.itsaky.androidide.actions.file.FormatCodeAction import com.itsaky.androidide.actions.file.ShowTooltipAction +import com.itsaky.androidide.actions.locations.CodeActionsMenu import org.junit.Test import java.lang.reflect.Field @@ -19,9 +20,13 @@ import java.lang.reflect.Field * bucket that stores its `Context` therefore keeps one editor activity alive for the whole process, * which is what `ShowTooltipAction` did. * - * Reflection rather than a heap assertion: it names the mistake directly, costs nothing, and covers - * every action in the bucket instead of the one that happened to leak. Add new EDITOR_TEXT_ACTIONS - * entries here. + * Reflection rather than a heap assertion: it names the mistake directly and costs nothing. + * + * What it does not cover, so nobody reads more into a green run than is there: + * - the list is hand-maintained, so a newly registered action is uncovered until it is added here; + * - the LSP actions that `LSPEditorActions` nests under [CodeActionsMenu.children] at runtime; + * - a `Context` reached indirectly - held by a companion object, or captured by a lambda stored in + * a field - since those live on synthetic classes, not on a field of the action typed `Context`. */ class EditorTextActionContextTest { private val editorTextActions = @@ -34,6 +39,9 @@ class EditorTextActionContextTest { PasteAction::class.java, FormatCodeAction::class.java, ShowTooltipAction::class.java, + // A process-lifetime `object`, so the most dangerous entry in the bucket rather than one + // to leave out. + CodeActionsMenu::class.java, ) @Test