Skip to content

fix: copy the config on browser creation instead of mutating the caller's - #82

Merged
nathanfallet merged 1 commit into
mainfrom
fix/config-must-not-be-mutated-by-start
Aug 26, 2026
Merged

fix: copy the config on browser creation instead of mutating the caller's#82
nathanfallet merged 1 commit into
mainfrom
fix/config-must-not-be-mutated-by-start

Conversation

@nathanfallet

Copy link
Copy Markdown
Member

Problem

DefaultBrowser.start() writes runtime state onto the Config it is given — it assigns host and port, and appends launch arguments:

val connectExisting = config.host != null && config.port != null
if (!connectExisting) {
    config.host = "127.0.0.1"
    config.port = freePort()          // writes into the caller's instance
}
...
if (!connectExisting) {
    process = startProcess(exe, params)   // only launched in this branch
}

So passing the same Config to a second browser makes it take the connectExisting branch: it never launches a browser process, and only tries to reach the previous one's port, which is dead by then. The start then fails after the full connect window, logging Browser process started with PID: null.

This bites anyone retrying a failed start with the same config, which is a natural thing to do.

Fix

DefaultBrowser now works on its own copy:

open class DefaultBrowser(
    val coroutineScope: CoroutineScope,
    config: Config,
) : Browser {
    override val config: Config = config.copy()

Copying rather than making Config immutable keeps this a small, non-breaking change: browser.config.port still returns the port actually in use, and no public API changes shape.

Config.copy() is deep where it matters, with three details worth noting:

  • it duplicates the argument and extension lists, so mutating one config never affects the other;
  • it passes the already-resolved browserExecutablePath, so the copy does not re-run the executable disk search;
  • it copies the userDataDir backing field rather than the getter — reading the getter would materialise a temporary profile directory on a config that may never be started.

Tests

Three tests on Config.copy(): independence of args/host/port, no duplication of the default arguments across repeated copies, and correct userDataDir handling in both the custom and default cases.

ConfigDslTest.testCreateBrowserWithDslConfig asserted assertEquals(cfg, browser.config), which encoded the old instance-sharing behaviour. It now asserts the stronger contract: the browser holds a distinct copy, the values carry over, and the caller's config is left untouched (cfg.port stays null while browser.config.port is set).

Note: :core:build fails on kotlinStoreYarnLock on this branch, but it does so on main too — unrelated to this change. :core:jvmTest passes.

…er's

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.
@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@nathanfallet
nathanfallet merged commit 8f062ea into main Aug 26, 2026
5 checks passed
@nathanfallet
nathanfallet deleted the fix/config-must-not-be-mutated-by-start branch August 26, 2026 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant