diff --git a/core/src/commonMain/kotlin/dev/kdriver/core/tab/DefaultTab.kt b/core/src/commonMain/kotlin/dev/kdriver/core/tab/DefaultTab.kt index db6b3fa41..0229ff779 100644 --- a/core/src/commonMain/kotlin/dev/kdriver/core/tab/DefaultTab.kt +++ b/core/src/commonMain/kotlin/dev/kdriver/core/tab/DefaultTab.kt @@ -379,9 +379,16 @@ open class DefaultTab( override suspend fun querySelectorAll( selector: String, node: NodeOrElement?, - ): List { - val lastMap = mutableMapOf() + ): List = querySelectorAllInternal(selector, node, retried = false) + // `retried` carries the "already retried once" state across the recursive call. It must be a + // parameter, not a local: a local would be reset on every recursion and the guard would never + // trip, recursing without bound on a persistent "could not find node" (ISSUE-7). + private suspend fun querySelectorAllInternal( + selector: String, + node: NodeOrElement?, + retried: Boolean, + ): List { val doc = if (node == null) { dom.getDocument(-1, true).root } else { @@ -393,20 +400,9 @@ open class DefaultTab( dom.querySelectorAll(doc.nodeId, selector) } catch (e: Exception) { if (node != null && e.message?.contains("could not find node", ignoreCase = true) == true) { - val last = lastMap[node.node.nodeId] - - if (last == true) { - // Remove the marker to avoid infinite recursion - lastMap.remove(node.node.nodeId) - return emptyList() - } - + if (retried) return emptyList() if (node is NodeOrElement.WrappedElement) node.element.update() - - // Mark as retried once - lastMap[node.node.nodeId] = true - - return querySelectorAll(selector, node) + return querySelectorAllInternal(selector, node, retried = true) } else { disableDomAgent() throw e @@ -429,11 +425,15 @@ open class DefaultTab( override suspend fun querySelector( selector: String, node: NodeOrElement?, - ): Element? { - val lastMap = mutableMapOf() - - val trimmedSelector = selector.trim() + ): Element? = querySelectorInternal(selector.trim(), node, retried = false) + // See querySelectorAllInternal: `retried` must be a parameter so the retry-once guard survives + // the recursive call (ISSUE-7). + private suspend fun querySelectorInternal( + selector: String, + node: NodeOrElement?, + retried: Boolean, + ): Element? { val doc = if (node == null) { dom.getDocument(-1, true).root } else { @@ -442,23 +442,12 @@ open class DefaultTab( } val nodeId = try { - dom.querySelector(doc.nodeId, trimmedSelector) + dom.querySelector(doc.nodeId, selector) } catch (e: Exception) { if (node != null && e.message?.contains("could not find node", ignoreCase = true) == true) { - val last = lastMap[node.node.nodeId] - - if (last == true) { - // Remove the marker to avoid infinite recursion - lastMap.remove(node.node.nodeId) - return null - } - + if (retried) return null if (node is NodeOrElement.WrappedElement) node.element.update() - - // Mark as retried once - lastMap[node.node.nodeId] = true - - return querySelector(trimmedSelector, node) + return querySelectorInternal(selector, node, retried = true) } else if (e.message?.contains("could not find node", ignoreCase = true) == true) { return null } else { diff --git a/core/src/jvmTest/kotlin/dev/kdriver/core/tab/QuerySelectorRetryTest.kt b/core/src/jvmTest/kotlin/dev/kdriver/core/tab/QuerySelectorRetryTest.kt new file mode 100644 index 000000000..01ef73b71 --- /dev/null +++ b/core/src/jvmTest/kotlin/dev/kdriver/core/tab/QuerySelectorRetryTest.kt @@ -0,0 +1,106 @@ +package dev.kdriver.core.tab + +import dev.kdriver.cdp.CDPException +import dev.kdriver.cdp.CommandMode +import dev.kdriver.core.dom.NodeOrElement +import dev.kdriver.cdp.domain.DOM +import dev.kdriver.cdp.domain.Target +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.test.runTest +import kotlinx.serialization.json.JsonElement +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * Guards the retry behavior of [DefaultTab.querySelector] / [querySelectorAll] (audit ISSUE-7). + * + * When CDP reports "could not find node", the code retries **once** (after refreshing the node) + * and then gives up. The retry-once guard (`lastMap`) was declared as a method-local that is + * recreated on every recursive call, so it never tripped and the method recursed without bound + * (StackOverflowError / hang) whenever the error persisted. + * + * These tests drive the real production methods through a stubbed [callCommand] (no browser), + * with a hard cap that turns runaway recursion into a clear failure instead of a noisy crash. + */ +class QuerySelectorRetryTest { + + private class StubTab( + scope: CoroutineScope, + private val onSelectorCall: () -> Unit, + ) : DefaultTab( + websocketUrl = "ws://stub/devtools/page/stub", + messageListeningScope = scope, + targetInfo = TARGET, + ) { + override suspend fun callCommand( + method: String, + parameter: JsonElement?, + mode: CommandMode, + ): JsonElement? = when (method) { + "DOM.querySelector", "DOM.querySelectorAll" -> { + onSelectorCall() + throw CDPException( + method = method, + code = -32000, + originalMessage = "Could not find node with given id", + data = null, + ) + } + // DOM.disable and anything else: succeed with no result. + else -> null + } + } + + private fun node(nodeId: Int) = DOM.Node( + nodeId = nodeId, + backendNodeId = nodeId, + nodeType = 1, + nodeName = "DIV", + localName = "div", + nodeValue = "", + ) + + @Test + fun querySelector_retriesOnceThenGivesUp_onPersistentNotFound() = runTest { + var calls = 0 + val tab = StubTab(this) { + calls++ + // Safety net: without the fix the recursion never terminates. Fail loudly and + // deterministically instead of letting it grow into a StackOverflowError. + check(calls <= CAP) { "querySelector recursed $calls times — retry guard not working (ISSUE-7)" } + } + + val result = tab.querySelector("div.missing", NodeOrElement.WrappedNode(node(1))) + + assertNull(result, "Persistent 'could not find node' should resolve to null") + assertEquals(2, calls, "Should attempt exactly once + one retry") + } + + @Test + fun querySelectorAll_retriesOnceThenGivesUp_onPersistentNotFound() = runTest { + var calls = 0 + val tab = StubTab(this) { + calls++ + check(calls <= CAP) { "querySelectorAll recursed $calls times — retry guard not working (ISSUE-7)" } + } + + val result = tab.querySelectorAll("div.missing", NodeOrElement.WrappedNode(node(1))) + + assertTrue(result.isEmpty(), "Persistent 'could not find node' should resolve to empty list") + assertEquals(2, calls, "Should attempt exactly once + one retry") + } + + private companion object { + const val CAP = 8 + val TARGET = Target.TargetInfo( + targetId = "stub", + type = "page", + title = "", + url = "about:blank", + attached = true, + canAccessOpener = false, + ) + } +}