From 3ad3ee7c811227775124faef58182fe072fda1b9 Mon Sep 17 00:00:00 2001 From: Alex Popov Date: Thu, 10 Jul 2025 23:23:57 +0700 Subject: [PATCH 1/2] Implement session cleanup in ChromeDriverService when session creation fails --- .../selenium/remote/RemoteWebDriver.java | 79 +++++++++++-------- .../ChromeDriverServiceCleanupTest.java | 48 +++++++++++ 2 files changed, 94 insertions(+), 33 deletions(-) create mode 100644 java/test/org/openqa/selenium/chrome/ChromeDriverServiceCleanupTest.java diff --git a/java/src/org/openqa/selenium/remote/RemoteWebDriver.java b/java/src/org/openqa/selenium/remote/RemoteWebDriver.java index b840936a41c66..1dc42ed132c55 100644 --- a/java/src/org/openqa/selenium/remote/RemoteWebDriver.java +++ b/java/src/org/openqa/selenium/remote/RemoteWebDriver.java @@ -88,6 +88,7 @@ import org.openqa.selenium.remote.http.ClientConfig; import org.openqa.selenium.remote.http.ConnectionFailedException; import org.openqa.selenium.remote.http.HttpClient; +import org.openqa.selenium.remote.service.DriverCommandExecutor; import org.openqa.selenium.remote.tracing.TracedHttpClient; import org.openqa.selenium.remote.tracing.Tracer; import org.openqa.selenium.remote.tracing.opentelemetry.OpenTelemetryTracer; @@ -241,46 +242,58 @@ protected void startSession(Capabilities capabilities) { checkNonW3CCapabilities(capabilities); checkChromeW3CFalse(capabilities); - Response response = execute(DriverCommand.NEW_SESSION(singleton(capabilities))); + try { + Response response = execute(DriverCommand.NEW_SESSION(singleton(capabilities))); - if (response == null) { - throw new SessionNotCreatedException( - "The underlying command executor returned a null response."); - } + if (response == null) { + throw new SessionNotCreatedException( + "The underlying command executor returned a null response."); + } - Object responseValue = response.getValue(); + Object responseValue = response.getValue(); - if (responseValue == null) { - throw new SessionNotCreatedException( - "The underlying command executor returned a response without payload: " + response); - } + if (responseValue == null) { + throw new SessionNotCreatedException( + "The underlying command executor returned a response without payload: " + response); + } - if (!(responseValue instanceof Map)) { - throw new SessionNotCreatedException( - "The underlying command executor returned a response with a non well formed payload: " - + response); - } + if (!(responseValue instanceof Map)) { + throw new SessionNotCreatedException( + "The underlying command executor returned a response with a non well formed payload: " + + response); + } - @SuppressWarnings("unchecked") - Map rawCapabilities = (Map) responseValue; - MutableCapabilities returnedCapabilities = new MutableCapabilities(rawCapabilities); - String platformString = (String) rawCapabilities.get(PLATFORM_NAME); - Platform platform; - try { - if (platformString == null || platformString.isEmpty()) { - platform = Platform.ANY; - } else { - platform = Platform.fromString(platformString); + @SuppressWarnings("unchecked") + Map rawCapabilities = (Map) responseValue; + MutableCapabilities returnedCapabilities = new MutableCapabilities(rawCapabilities); + String platformString = (String) rawCapabilities.get(PLATFORM_NAME); + Platform platform; + try { + if (platformString == null || platformString.isEmpty()) { + platform = Platform.ANY; + } else { + platform = Platform.fromString(platformString); + } + } catch (WebDriverException e) { + // The server probably responded with a name matching the os.name + // system property. Try to recover and parse this. + platform = Platform.extractFromSysProperty(platformString); } - } catch (WebDriverException e) { - // The server probably responded with a name matching the os.name - // system property. Try to recover and parse this. - platform = Platform.extractFromSysProperty(platformString); + returnedCapabilities.setCapability(PLATFORM_NAME, platform); + + this.capabilities = returnedCapabilities; + sessionId = new SessionId(response.getSessionId()); + } catch (Exception e) { + // If session creation fails, stop the driver service to prevent zombie processes + if (executor instanceof DriverCommandExecutor) { + try { + ((DriverCommandExecutor) executor).close(); + } catch (Exception ignored) { + // Ignore cleanup exceptions, we'll propagate the original failure + } + } + throw e; } - returnedCapabilities.setCapability(PLATFORM_NAME, platform); - - this.capabilities = returnedCapabilities; - sessionId = new SessionId(response.getSessionId()); } public ErrorHandler getErrorHandler() { diff --git a/java/test/org/openqa/selenium/chrome/ChromeDriverServiceCleanupTest.java b/java/test/org/openqa/selenium/chrome/ChromeDriverServiceCleanupTest.java new file mode 100644 index 0000000000000..d2b2be0d9593c --- /dev/null +++ b/java/test/org/openqa/selenium/chrome/ChromeDriverServiceCleanupTest.java @@ -0,0 +1,48 @@ +// Licensed to the Software Freedom Conservancy (SFC) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The SFC licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.openqa.selenium.chrome; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.mockito.Mockito.spy; + +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.openqa.selenium.SessionNotCreatedException; + +@Tag("UnitTests") +class ChromeDriverServiceCleanupTest { + + @Test + void shouldStopServiceWhenSessionCreationFails() { + // Create a Chrome options that will cause session creation to fail + ChromeOptions options = new ChromeOptions(); + options.addArguments("--user-data-dir=/no/such/location"); + + // Create a service + ChromeDriverService service = ChromeDriverService.createDefaultService(); + ChromeDriverService serviceSpy = spy(service); + + // Attempt to create driver - should fail and cleanup the service + assertThatExceptionOfType(SessionNotCreatedException.class) + .isThrownBy(() -> new ChromeDriver(serviceSpy, options)); + + // Verify that the service was stopped + assertThat(serviceSpy.isRunning()).isFalse(); + } +} From 05adcd4dd46af167de1ca604687876adabf69d37 Mon Sep 17 00:00:00 2001 From: Alex Popov Date: Thu, 10 Jul 2025 23:46:46 +0700 Subject: [PATCH 2/2] Add tests to ensure services stop when session creation fails for Edge, Firefox, and Safari --- .../selenium/edge/EdgeDriverServiceTest.java | 20 ++++++++++++++++++ .../firefox/GeckoDriverServiceTest.java | 21 +++++++++++++++++++ .../safari/SafariDriverServiceTest.java | 21 +++++++++++++++++++ 3 files changed, 62 insertions(+) diff --git a/java/test/org/openqa/selenium/edge/EdgeDriverServiceTest.java b/java/test/org/openqa/selenium/edge/EdgeDriverServiceTest.java index bc15c9f7d6ff4..b014e7417d1ca 100644 --- a/java/test/org/openqa/selenium/edge/EdgeDriverServiceTest.java +++ b/java/test/org/openqa/selenium/edge/EdgeDriverServiceTest.java @@ -18,6 +18,7 @@ package org.openqa.selenium.edge; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; @@ -30,6 +31,7 @@ import java.util.List; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; +import org.openqa.selenium.SessionNotCreatedException; import org.openqa.selenium.chromium.ChromiumDriverLogLevel; @Tag("UnitTests") @@ -87,4 +89,22 @@ void ignoreFalseLogging() { builderMock.withLoglevel(ChromiumDriverLogLevel.DEBUG).usingPort(1).withSilent(false).build(); verify(builderMock).createDriverService(any(), anyInt(), any(), eq(falseSilent), any()); } + + @Test + void shouldStopServiceWhenSessionCreationFails() { + // Create Edge options that will cause session creation to fail + EdgeOptions options = new EdgeOptions(); + options.addArguments("--user-data-dir=/no/such/location"); + + // Create a service + EdgeDriverService service = EdgeDriverService.createDefaultService(); + EdgeDriverService serviceSpy = spy(service); + + // Attempt to create driver - should fail and cleanup the service + assertThatExceptionOfType(SessionNotCreatedException.class) + .isThrownBy(() -> new EdgeDriver(serviceSpy, options)); + + // Verify that the service was stopped + assertThat(serviceSpy.isRunning()).isFalse(); + } } diff --git a/java/test/org/openqa/selenium/firefox/GeckoDriverServiceTest.java b/java/test/org/openqa/selenium/firefox/GeckoDriverServiceTest.java index a53badbc7b216..d6e3660dd7c7a 100644 --- a/java/test/org/openqa/selenium/firefox/GeckoDriverServiceTest.java +++ b/java/test/org/openqa/selenium/firefox/GeckoDriverServiceTest.java @@ -17,6 +17,8 @@ package org.openqa.selenium.firefox; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; @@ -27,6 +29,7 @@ import java.time.Duration; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; +import org.openqa.selenium.SessionNotCreatedException; @Tag("UnitTests") class GeckoDriverServiceTest { @@ -46,4 +49,22 @@ void builderPassesTimeoutToDriverService() { builderMock.build(); verify(builderMock).createDriverService(any(), anyInt(), eq(customTimeout), any(), any()); } + + @Test + void shouldStopServiceWhenSessionCreationFails() { + // Create Firefox options that will cause session creation to fail + FirefoxOptions options = new FirefoxOptions(); + options.setBinary("/path/to/nonexistent/firefox/binary"); + + // Create a service + GeckoDriverService service = GeckoDriverService.createDefaultService(); + GeckoDriverService serviceSpy = spy(service); + + // Attempt to create driver - should fail and cleanup the service + assertThatExceptionOfType(Exception.class) + .isThrownBy(() -> new FirefoxDriver(serviceSpy, options)); + + // Verify that the service was stopped + assertThat(serviceSpy.isRunning()).isFalse(); + } } diff --git a/java/test/org/openqa/selenium/safari/SafariDriverServiceTest.java b/java/test/org/openqa/selenium/safari/SafariDriverServiceTest.java index 5f1a124580b58..e4dbe76dcc1d5 100644 --- a/java/test/org/openqa/selenium/safari/SafariDriverServiceTest.java +++ b/java/test/org/openqa/selenium/safari/SafariDriverServiceTest.java @@ -17,6 +17,8 @@ package org.openqa.selenium.safari; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.eq; @@ -27,6 +29,7 @@ import java.time.Duration; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; +import org.openqa.selenium.SessionNotCreatedException; @Tag("UnitTests") class SafariDriverServiceTest { @@ -47,6 +50,24 @@ void builderPassesTimeoutToDriverService() { verify(builderMock).createDriverService(any(), anyInt(), eq(customTimeout), any(), any()); } + @Test + void shouldStopServiceWhenSessionCreationFails() { + // Create Safari options that will cause session creation to fail + SafariOptions options = new SafariOptions(); + options.setCapability("invalidCapability", "invalidValue"); + + // Create a service + SafariDriverService service = SafariDriverService.createDefaultService(); + SafariDriverService serviceSpy = spy(service); + + // Attempt to create driver - should fail and cleanup the service + assertThatExceptionOfType(Exception.class) + .isThrownBy(() -> new SafariDriver(serviceSpy, options)); + + // Verify that the service was stopped + assertThat(serviceSpy.isRunning()).isFalse(); + } + public static class MockSafariDriverServiceBuilder extends SafariDriverService.Builder { @Override