Merge branch 'trunk' into pinned-browser-updates

Author: navin772

.skipped-tests

@@ -1,5 +1,6 @@
 -//dotnet/test/common:NetworkInterceptionTests-chrome
 -//dotnet/test/common:NetworkInterceptionTests-edge
+-//dotnet/test/firefox:FirefoxDriverTest-firefox
 -//java/test/org/openqa/selenium/chrome:ChromeDriverFunctionalTest
 -//java/test/org/openqa/selenium/chrome:ChromeDriverFunctionalTest-remote
 -//java/test/org/openqa/selenium/edge:EdgeDriverFunctionalTest

dotnet/src/webdriver/DriverService.cs

@@ -284,7 +284,7 @@ protected virtual void Dispose(bool disposing)
     /// Raises the <see cref="DriverProcessStarting"/> event.
     /// </summary>
     /// <param name="eventArgs">A <see cref="DriverProcessStartingEventArgs"/> that contains the event data.</param>
-    protected void OnDriverProcessStarting(DriverProcessStartingEventArgs eventArgs)
+    protected virtual void OnDriverProcessStarting(DriverProcessStartingEventArgs eventArgs)
     {
         if (eventArgs == null)
         {
@@ -298,7 +298,7 @@ protected void OnDriverProcessStarting(DriverProcessStartingEventArgs eventArgs)
     /// Raises the <see cref="DriverProcessStarted"/> event.
     /// </summary>
     /// <param name="eventArgs">A <see cref="DriverProcessStartedEventArgs"/> that contains the event data.</param>
-    protected void OnDriverProcessStarted(DriverProcessStartedEventArgs eventArgs)
+    protected virtual void OnDriverProcessStarted(DriverProcessStartedEventArgs eventArgs)
     {
         if (eventArgs == null)
         {

dotnet/src/webdriver/Firefox/FirefoxDriverService.cs

@@ -22,6 +22,7 @@
 using System.Globalization;
 using System.IO;
 using System.Text;
+using System.Threading.Tasks;
 
 namespace OpenQA.Selenium.Firefox;
 
@@ -32,6 +33,11 @@ public sealed class FirefoxDriverService : DriverService
 {
     private const string DefaultFirefoxDriverServiceFileName = "geckodriver";
 
+    /// <summary>
+    /// Process management fields for the log writer.
+    /// </summary>
+    private StreamWriter? logWriter;
+
     /// <summary>
     /// Initializes a new instance of the <see cref="FirefoxDriverService"/> class.
     /// </summary>
@@ -87,6 +93,16 @@ protected override DriverOptions GetDefaultDriverOptions()
     /// </summary>
     public bool OpenBrowserToolbox { get; set; }
 
+    /// <summary>
+    /// Gets or sets the file path where log output should be written.
+    /// </summary>
+    /// <remarks>
+    /// A <see langword="null"/> or <see cref="string.Empty"/> value indicates no log file to specify.
+    /// This approach takes the process output and redirects it to a file because GeckoDriver does not
+    /// offer a way to specify a log file path directly.
+    /// </remarks>
+    public string? LogPath { get; set; }
+
     /// <summary>
     /// Gets or sets the level at which log output is displayed.
     /// </summary>
@@ -177,6 +193,75 @@ protected override string CommandLineArguments
         }
     }
 
+    /// <summary>
+    /// Handles the event when the driver service process is starting.
+    /// </summary>
+    /// <param name="eventArgs">The event arguments containing information about the driver service process.</param>
+    /// <remarks>
+    /// This method initializes a log writer if a log path is specified and redirects output streams to capture logs.
+    /// </remarks>
+    protected override void OnDriverProcessStarting(DriverProcessStartingEventArgs eventArgs)
+    {
+        if (!string.IsNullOrEmpty(this.LogPath))
+        {
+            string? directory = Path.GetDirectoryName(this.LogPath);
+            if (!string.IsNullOrEmpty(directory) && !Directory.Exists(directory))
+            {
+                Directory.CreateDirectory(directory);
+            }
+
+            // Initialize the log writer
+            logWriter = new StreamWriter(this.LogPath, append: true) { AutoFlush = true };
+
+            // Configure process to redirect output
+            eventArgs.DriverServiceProcessStartInfo.RedirectStandardOutput = true;
+            eventArgs.DriverServiceProcessStartInfo.RedirectStandardError = true;
+        }
+
+        base.OnDriverProcessStarting(eventArgs);
+    }
+
+    /// <summary>
+    /// Handles the event when the driver process has started.
+    /// </summary>
+    /// <param name="eventArgs">The event arguments containing information about the started driver process.</param>
+    /// <remarks>
+    /// This method reads the output and error streams asynchronously and writes them to the log file if available.
+    /// </remarks>
+    protected override void OnDriverProcessStarted(DriverProcessStartedEventArgs eventArgs)
+    {
+        if (logWriter == null) return;
+        if (eventArgs.StandardOutputStreamReader != null)
+        {
+            _ = Task.Run(() => ReadStreamAsync(eventArgs.StandardOutputStreamReader));
+        }
+
+        if (eventArgs.StandardErrorStreamReader != null)
+        {
+            _ = Task.Run(() => ReadStreamAsync(eventArgs.StandardErrorStreamReader));
+        }
+
+        base.OnDriverProcessStarted(eventArgs);
+    }
+
+    /// <summary>
+    /// Disposes of the resources used by the <see cref="FirefoxDriverService"/> instance.
+    /// </summary>
+    /// <param name="disposing">A value indicating whether the method is being called from Dispose.</param>
+    /// <remarks>
+    /// If disposing is true, it disposes of the log writer if it exists.
+    /// </remarks>
+    protected override void Dispose(bool disposing)
+    {
+        if (logWriter != null && disposing)
+        {
+            logWriter.Dispose();
+            logWriter = null;
+        }
+
+        base.Dispose(disposing);
+    }
+
     /// <summary>
     /// Creates a default instance of the FirefoxDriverService.
     /// </summary>
@@ -258,4 +343,24 @@ private static string FirefoxDriverServiceFileName()
 
         return fileName;
     }
+
+    private async Task ReadStreamAsync(StreamReader reader)
+    {
+        try
+        {
+            string? line;
+            while ((line = await reader.ReadLineAsync()) != null)
+            {
+                if (logWriter != null)
+                {
+                    logWriter.WriteLine($"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} {line}");
+                }
+            }
+        }
+        catch (Exception ex)
+        {
+            // Log or handle the exception appropriately
+            System.Diagnostics.Debug.WriteLine($"Error reading stream: {ex.Message}");
+        }
+    }
 }

dotnet/test/firefox/BUILD.bazel

@@ -0,0 +1,27 @@
+load("//dotnet:defs.bzl", "dotnet_nunit_test_suite", "framework")
+
+dotnet_nunit_test_suite(
+    name = "LargeTests",
+    size = "large",
+    srcs = glob(
+        [
+            "**/*Test.cs",
+            "**/*Tests.cs",
+        ],
+    ) + [
+        "//dotnet/test/common:assembly-fixtures",
+    ],
+    browsers = [
+        "firefox",
+    ],
+    data = [
+        "//dotnet/test/common:test-data",
+    ],
+    target_frameworks = ["net8.0"],
+    deps = [
+        "//dotnet/src/support",
+        "//dotnet/src/webdriver:webdriver-net8.0",
+        "//dotnet/test/common:fixtures",
+        framework("nuget", "NUnit"),
+    ],
+)

dotnet/test/firefox/FirefoxDriverServiceTest.cs

@@ -0,0 +1,53 @@
+// <copyright file="FirefoxDriverServiceTest.cs" company="Selenium Committers">
+// 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.
+// </copyright>
+
+using NUnit.Framework;
+using System.IO;
+
+namespace OpenQA.Selenium.Firefox;
+
+[TestFixture]
+public class FirefoxDriverServiceTest : DriverTestFixture
+{
+    [Test]
+    public void ShouldRedirectGeckoDriverLogsToFile()
+    {
+        FirefoxOptions options = new FirefoxOptions();
+        string logPath = Path.GetTempFileName();
+        options.LogLevel = FirefoxDriverLogLevel.Trace;
+
+        FirefoxDriverService service = FirefoxDriverService.CreateDefaultService();
+        service.LogPath = logPath;
+
+        IWebDriver driver2 = new FirefoxDriver(service, options);
+
+        try
+        {
+            Assert.That(File.Exists(logPath), Is.True);
+            string logContent = File.ReadAllText(logPath);
+            Assert.That(logContent, Does.Contain("geckodriver"));
+        }
+        finally
+        {
+            driver2.Quit();
+            File.Delete(logPath);
+        }
+    }
+
+}

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() {

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();
+  }
+}