[java] Fix 15634/ensure driver closed java (#16038)

iampopovich · pujagani · web-flow · commit e2fc5e825a0a · 2025-07-25T16:32:17.000+05:30
Co-authored-by: Puja Jagani &lt;puja.jagani93@gmail.com&gt;
diff --git 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<String, Object> rawCapabilities = (Map<String, Object>) 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<String, Object> rawCapabilities = (Map<String, Object>) 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
@@ -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();
+  }
+}
diff --git 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
@@ -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;
@@ -46,4 +48,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
@@ -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;
@@ -47,6 +49,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
