From 518d7c104f7d788f1fa8c0f7f8fb824955afb2b2 Mon Sep 17 00:00:00 2001 From: NathanFallet Date: Wed, 26 Aug 2026 19:04:08 +0200 Subject: [PATCH] fix: copy the config on browser creation instead of mutating the caller's MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DefaultBrowser.start() resolves runtime state onto the Config it is handed: it assigns host and port, and appends launch arguments. Reusing the same Config instance for a second browser therefore made start() take its connectExisting branch, where it never launches a browser process and only tries to reach the previous one's now-dead port. The retry could not succeed, and logged "Browser process started with PID: null" while burning a full connect window. DefaultBrowser now works on its own copy, so callers can safely reuse a Config — including to retry a failed start. Config.copy() duplicates the argument and extension lists, passes the already resolved executable path so the copy does not re-run the disk search, and copies the userDataDir backing field rather than the getter, which would otherwise materialise a temporary profile directory on a config that may never be started. --- .../kotlin/dev/kdriver/core/browser/Config.kt | 39 +++++++++++++++++++ .../kdriver/core/browser/DefaultBrowser.kt | 13 ++++++- .../dev/kdriver/core/browser/ConfigDslTest.kt | 10 ++++- .../dev/kdriver/core/browser/ConfigTest.kt | 39 +++++++++++++++++++ 4 files changed, 99 insertions(+), 2 deletions(-) diff --git a/core/src/commonMain/kotlin/dev/kdriver/core/browser/Config.kt b/core/src/commonMain/kotlin/dev/kdriver/core/browser/Config.kt index 0b24b3929..62190705f 100644 --- a/core/src/commonMain/kotlin/dev/kdriver/core/browser/Config.kt +++ b/core/src/commonMain/kotlin/dev/kdriver/core/browser/Config.kt @@ -105,6 +105,45 @@ class Config( return args } + /** + * Returns an independent copy of this configuration. + * + * [DefaultBrowser.start] resolves and stores runtime state on the config it is given — it writes + * [host] and [port], and appends `--load-extension` / `--lang` arguments. Handing the same + * instance to a second browser would therefore make that browser take the `connectExisting` + * branch: it would never launch a browser process at all, and would instead try to talk to the + * previous one's now-dead port. [DefaultBrowser] copies its config on construction so that + * callers can safely reuse (and retry with) the same [Config] instance. + * + * The copy is deep where it matters: the argument and extension lists are duplicated, so + * mutating one config never affects the other. + */ + fun copy(): Config = Config( + // Pass the already-resolved executable so the copy does not re-run the disk search. + browserExecutablePath = browserExecutablePath, + headless = headless, + userAgent = userAgent, + // _browserArgs, not the browserArgs getter: the getter prepends defaultBrowserArgs, which + // would end up duplicated into the copy's own arg list. + browserArgs = _browserArgs.toList(), + sandbox = sandbox, + lang = lang, + host = host, + port = port, + expert = expert, + browserConnectionTimeout = browserConnectionTimeout, + browserConnectionMaxTries = browserConnectionMaxTries, + commandTimeout = commandTimeout, + autoDiscoverTargets = autoDiscoverTargets, + debugStringLimit = debugStringLimit, + ).also { copy -> + // Copy the backing field, never the userDataDir getter: reading it would materialise a + // temporary profile directory on a config that may never be started. + copy._userDataDir = _userDataDir + copy._customDataDir = _customDataDir + copy._extensions.addAll(_extensions) + } + fun addArgument(arg: String) { val forbiddenArgs = listOf("headless", "data-dir", "data_dir", "no-sandbox", "no_sandbox", "lang") if (forbiddenArgs.any { arg.contains(it, ignoreCase = true) }) { diff --git a/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt b/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt index a922652b4..64a3fff25 100644 --- a/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt +++ b/core/src/commonMain/kotlin/dev/kdriver/core/browser/DefaultBrowser.kt @@ -23,9 +23,20 @@ import kotlin.time.Duration.Companion.seconds */ open class DefaultBrowser( val coroutineScope: CoroutineScope, - override val config: Config, + config: Config, ) : Browser { + /** + * This browser's own copy of the configuration it was created with. + * + * [start] resolves runtime state onto it — it assigns [Config.host] / [Config.port] and appends + * launch arguments — so it must not be the caller's instance. Mutating a shared config would + * make any later browser built from it take the `connectExisting` branch of [start]: it would + * skip launching a browser process entirely and try to reach the previous port, which is dead. + * Copying here makes reusing (and retrying with) the same [Config] safe by construction. + */ + override val config: Config = config.copy() + private val logger = KtorSimpleLogger("Browser") private val updateTargetInfoMutex = Mutex() diff --git a/core/src/jvmTest/kotlin/dev/kdriver/core/browser/ConfigDslTest.kt b/core/src/jvmTest/kotlin/dev/kdriver/core/browser/ConfigDslTest.kt index bbb06e237..748fa9fe6 100644 --- a/core/src/jvmTest/kotlin/dev/kdriver/core/browser/ConfigDslTest.kt +++ b/core/src/jvmTest/kotlin/dev/kdriver/core/browser/ConfigDslTest.kt @@ -5,6 +5,8 @@ import kotlinx.coroutines.runBlocking import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertNotNull +import kotlin.test.assertNotSame +import kotlin.test.assertNull import kotlin.test.assertTrue class ConfigDslTest { @@ -21,7 +23,13 @@ class ConfigDslTest { val tab = browser.get("https://example.com") assertTrue(cfg.headless) - assertEquals(cfg, browser.config) + // The browser works on its own copy, so starting it leaves the caller's config untouched + // and the same instance can be reused for another browser. + assertNotSame(cfg, browser.config) + assertEquals(cfg.headless, browser.config.headless) + assertEquals(cfg.sandbox, browser.config.sandbox) + assertNull(cfg.port) + assertNotNull(browser.config.port) assertNotNull(tab.getContent()) browser.stop() diff --git a/core/src/jvmTest/kotlin/dev/kdriver/core/browser/ConfigTest.kt b/core/src/jvmTest/kotlin/dev/kdriver/core/browser/ConfigTest.kt index dee8604b3..87b7a4a19 100644 --- a/core/src/jvmTest/kotlin/dev/kdriver/core/browser/ConfigTest.kt +++ b/core/src/jvmTest/kotlin/dev/kdriver/core/browser/ConfigTest.kt @@ -4,10 +4,49 @@ import kotlinx.io.files.Path import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertNull import kotlin.test.assertTrue class ConfigTest { + @Test + fun testCopyIsIndependent() { + val original = Config(browserExecutablePath = Path("/usr/bin/chromium")) + original.addArgument("--original-only") + + val copy = original.copy() + copy.addArgument("--copy-only") + copy.host = "127.0.0.1" + copy.port = 1234 + + assertTrue(copy.browserArgs.contains("--original-only")) + assertTrue(copy.browserArgs.contains("--copy-only")) + assertFalse(original.browserArgs.contains("--copy-only")) + assertNull(original.host) + assertNull(original.port) + } + + @Test + fun testCopyDoesNotDuplicateDefaultArguments() { + val original = Config(browserExecutablePath = Path("/usr/bin/chromium")) + + val args = original.copy().copy().browserArgs + + assertEquals(args.distinct().size, args.size) + } + + @Test + fun testCopyKeepsCustomUserDataDirWithoutMaterialisingATemporaryOne() { + val custom = Config(browserExecutablePath = Path("/usr/bin/chromium")) + custom.userDataDir = Path("/tmp/kdriver-test-profile") + assertEquals(custom.userDataDir, custom.copy().userDataDir) + + // A config that was never given one must not have a temp dir created for it by the copy. + val default = Config(browserExecutablePath = Path("/usr/bin/chromium")) + assertFalse(default.copy().usesCustomDataDir) + } + @Test fun testAddArgument() { val config = Config(browserExecutablePath = Path("/usr/bin/chromium"))