WIP [dotnet] Reusable DriverService instance by Drivers

[nvborisenko]

User description

DriverService instance seems to be reusable by Drivers, but in fact no. As soon as Quit command is handled by Drivers, then underlying DriverService is disposed silently.

https://github.com/SeleniumHQ/selenium/blame/215e20b139f9af0c2db76b800472daed0633c73c/dotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs#L128 - this is bad design.

Description

Motivation and Context

Fixes #14624 and #14633

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

PR Type

enhancement

---

Description

• Introduced a disposeService parameter across various driver classes to control the disposal of driver services.
• Updated constructors and methods to accommodate the new disposeService parameter.
• Removed the DriverServiceCommandExecutor class and replaced its functionality with direct service management.
• Enhanced the disposal process in the WebDriver class to ensure proper resource management.

---

Changes walkthrough 📝

Relevant files

Enhancement

9 files ChromeDriver.cs Add disposeService parameter to ChromeDriver constructors dotnet/src/webdriver/Chrome/ChromeDriver.cs Added disposeService parameter to constructors. Updated constructor calls to include disposeService. Added method to handle custom Chrome commands. +14/-3    ChromiumDriver.cs Enhance ChromiumDriver with service disposal control          dotnet/src/webdriver/Chromium/ChromiumDriver.cs Introduced disposeDriverService field. Modified command executor method to start service. Updated dispose method to handle driver service disposal. +16/-11  EdgeDriver.cs Update EdgeDriver constructor for service disposal              dotnet/src/webdriver/Edge/EdgeDriver.cs Updated constructor to include disposeService parameter. +1/-1      FirefoxDriver.cs Update FirefoxDriver to start service in executor                dotnet/src/webdriver/Firefox/FirefoxDriver.cs Renamed command executor method to start service. Updated constructor to use new method. +6/-3      InternetExplorerDriver.cs Modify InternetExplorerDriver to start service in executor dotnet/src/webdriver/IE/InternetExplorerDriver.cs Renamed command executor method to start service. Updated constructor to use new method. +6/-3      DriverServiceCommandExecutor.cs Remove DriverServiceCommandExecutor class                                dotnet/src/webdriver/Remote/DriverServiceCommandExecutor.cs Removed DriverServiceCommandExecutor class. +0/-163  SafariDriver.cs Update SafariDriver to start service in executor                  dotnet/src/webdriver/Safari/SafariDriver.cs Renamed command executor method to start service. Updated constructor to use new method. +6/-3      WebDriver.cs Ensure executor disposal in WebDriver class                            dotnet/src/webdriver/WebDriver.cs Added disposal of executor in Dispose method. +1/-0      DriverFactory.cs Modify DriverFactory to pass service in driver instantiation dotnet/test/common/Environment/DriverFactory.cs Updated driver instantiation to include service parameter. +1/-2

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[codiumai-pr-agent-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Resource Leak The driverService field is not being disposed in all scenarios. Ensure proper disposal of resources. Code Duplication Multiple constructors with similar logic. Consider refactoring to reduce duplication. Naming Inconsistency Method renamed from GenerateDriverServiceCommandExecutor to StartDriverServiceCommandExecutor. Ensure consistency across all driver classes.

[codiumai-pr-agent-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Add null check before disposing of the executor to prevent potential exceptions Consider wrapping the executor disposal in a null check to prevent potential null reference exceptions. dotnet/src/webdriver/WebDriver.cs [732] -this.executor.Dispose(); +this.executor?.Dispose(); Apply this suggestion Suggestion importance[1-10]: 8 Why: Adding a null check before disposing of the executor is a good practice to prevent potential null reference exceptions, which could lead to runtime errors. This suggestion addresses a possible issue effectively.	8
	Add null check for the service parameter to prevent potential null reference exceptions Consider adding a null check before using the service parameter in the CreateDriverWithOptions method to prevent potential null reference exceptions. dotnet/test/common/Environment/DriverFactory.cs [158] +if (service == null) +{ + throw new ArgumentNullException(nameof(service)); +} driver = (IWebDriver)Activator.CreateInstance(driverType, service, true); Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion to add a null check for the service parameter is important to prevent null reference exceptions, which could cause runtime errors. This is a valuable addition to ensure robustness in the method.	8
Enhancement	Implement constructor chaining to reduce code duplication and improve maintainability Consider using a constructor chaining pattern to reduce code duplication across the various constructors. This can help maintain consistency and make future updates easier. dotnet/src/webdriver/Chrome/ChromeDriver.cs [138-164] public ChromeDriver(ChromeDriverService service, ChromeOptions options) - : this(service, options, RemoteWebDriver.DefaultCommandTimeout, disposeService: true) + : this(service, options, RemoteWebDriver.DefaultCommandTimeout) { } public ChromeDriver(ChromeDriverService service, ChromeOptions options, bool disposeService) : this(service, options, RemoteWebDriver.DefaultCommandTimeout, disposeService) { } public ChromeDriver(ChromeDriverService service, ChromeOptions options, TimeSpan commandTimeout) - : base(service, options, commandTimeout, disposeService: true) + : this(service, options, commandTimeout, true) { - this.AddCustomChromeCommands(); } public ChromeDriver(ChromeDriverService service, ChromeOptions options, TimeSpan commandTimeout, bool disposeService) : base(service, options, commandTimeout, disposeService) { this.AddCustomChromeCommands(); } Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion to implement constructor chaining is valid as it reduces code duplication and enhances maintainability. The improved code correctly applies the chaining pattern, making the constructors more concise and easier to update in the future.	7
	Use null-coalescing assignment operator for more concise field initialization Consider using a null-coalescing assignment operator ??= to simplify the initialization of the driverService field if it's not already set. dotnet/src/webdriver/Chromium/ChromiumDriver.cs [131-136] protected ChromiumDriver(ChromiumDriverService service, ChromiumOptions options, TimeSpan commandTimeout, bool disposeService) : base(StartDriverServiceCommandExecutor(service, options, commandTimeout), ConvertOptionsToCapabilities(options)) { - this.driverService = service; + this.driverService ??= service; this.disposeDriverService = disposeService; this.optionsCapabilityName = options.CapabilityName; } Apply this suggestion Suggestion importance[1-10]: 5 Why: The suggestion to use a null-coalescing assignment operator is correct and makes the code more concise. However, the impact is minor since the field is being set in the constructor and is unlikely to be null at this point.	5

💡 Need additional feedback ? start a PR chat

[nvborisenko]

To move further we have to decide:

• Just fix "incorrect/old" behavior, which is not source/binary breaking change, but changes "old" behavior
• Or keep "old" behavior and introduce an ability to specify whether ChromeDriver should dispose underlying ChromeDriverService (chromedriver.exe)

@YevgeniyShunevych your input is appreciated here.