[dotnet] Simplify DriverService and DriverFinder

[RenderMichael]

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Removes code duplication by consolidating GenerateDriverServiceCommandExecutor into an instance method on DriverService. Also adds Try* method to DriverFinder to simplify lookups.

Motivation and Context

Some quick code cleanup opportunity I noticed.

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

• Consolidated GenerateDriverServiceCommandExecutor into DriverService as CreateCommandExecutor.

• Simplified driver initialization across multiple driver classes.

• Enhanced DriverFinder with nullability, error handling, and a TryGetBrowserPath method.

• Improved code readability and reduced duplication in driver service handling.

---

Changes walkthrough 📝

Relevant files

Enhancement

ChromiumDriver.cs Simplified ChromiumDriver initialization.                                dotnet/src/webdriver/Chromium/ChromiumDriver.cs Replaced GenerateDriverServiceCommandExecutor with CreateCommandExecutor. Removed redundant private method for command executor generation. +1/-25    DriverFinder.cs Enhanced DriverFinder with nullability and new methods.    dotnet/src/webdriver/DriverFinder.cs Added TryGetBrowserPath method for safer browser path retrieval. Improved nullability handling and error checks. Refactored BinaryPaths for better readability and caching. +38/-14  DriverService.cs Centralized command executor creation in DriverService.    dotnet/src/webdriver/DriverService.cs Added CreateCommandExecutor method to centralize command executor creation. Integrated browser path search logic into DriverService. +24/-0    FirefoxDriver.cs Simplified FirefoxDriver initialization.                                  dotnet/src/webdriver/Firefox/FirefoxDriver.cs Replaced GenerateDriverServiceCommandExecutor with CreateCommandExecutor. Removed redundant private method for command executor generation. +1/-25    InternetExplorerDriver.cs Simplified InternetExplorerDriver initialization.                dotnet/src/webdriver/IE/InternetExplorerDriver.cs Replaced GenerateDriverServiceCommandExecutor with CreateCommandExecutor. Removed redundant private method for command executor generation. +1/-20    SafariDriver.cs Simplified SafariDriver initialization.                                    dotnet/src/webdriver/Safari/SafariDriver.cs Replaced GenerateDriverServiceCommandExecutor with CreateCommandExecutor. Removed redundant private method for command executor generation. +1/-20

---

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

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The BinaryPaths method may throw exceptions when browser path is not found, but the TryGetBrowserPath method silently returns false. This inconsistency in error handling could lead to unexpected behavior. private Dictionary<string, string> BinaryPaths() { if (paths.TryGetValue(DriverPathKey, out string? cachedDriverPath) && !string.IsNullOrWhiteSpace(cachedDriverPath)) { return paths; } Dictionary<string, string> binaryPaths = SeleniumManager.BinaryPaths(CreateArguments()); string driverPath = binaryPaths[DriverPathKey]; string browserPath = binaryPaths[BrowserPathKey]; if (!File.Exists(driverPath)) { throw new NoSuchDriverException($"The driver path is not a valid file: {driverPath}"); } if (!File.Exists(browserPath)) { throw new NoSuchDriverException($"The browser path is not a valid file: {browserPath}"); } paths.Add(DriverPathKey, driverPath); paths.Add(BrowserPathKey, browserPath); return paths; } Resource Management The CreateCommandExecutor method creates a new DriverFinder instance on each call without proper disposal. Consider caching the finder or ensuring proper cleanup of resources. internal DriverServiceCommandExecutor CreateCommandExecutor(DriverOptions options, TimeSpan commandTimeout, bool searchForBrowserPath) { if (DriverServicePath == null) { DriverFinder finder = new DriverFinder(options); string fullServicePath = finder.GetDriverPath(); DriverServicePath = Path.GetDirectoryName(fullServicePath); DriverServiceExecutableName = Path.GetFileName(fullServicePath); if (searchForBrowserPath && finder.TryGetBrowserPath(out string browserPath)) { options.BinaryLocation = browserPath; options.BrowserVersion = null; } } return new DriverServiceCommandExecutor(this, commandTimeout); }

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Add defensive null checks when accessing dictionary values to prevent runtime exceptions Add null check for the driverPath and browserPath values returned from SeleniumManager.BinaryPaths() before accessing them to prevent potential NullReferenceException. dotnet/src/webdriver/DriverFinder.cs [115-117] Dictionary<string, string> binaryPaths = SeleniumManager.BinaryPaths(CreateArguments()); -string driverPath = binaryPaths[DriverPathKey]; -string browserPath = binaryPaths[BrowserPathKey]; +if (!binaryPaths.TryGetValue(DriverPathKey, out string driverPath) || string.IsNullOrEmpty(driverPath)) +{ + throw new NoSuchDriverException($"No driver path was returned by Selenium Manager"); +} +if (!binaryPaths.TryGetValue(BrowserPathKey, out string browserPath) || string.IsNullOrEmpty(browserPath)) +{ + throw new NoSuchDriverException($"No browser path was returned by Selenium Manager"); +} Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion addresses a potential runtime vulnerability by adding proper null checks for dictionary values. This is critical for preventing NullReferenceException and improving error handling.	8
	Add thread synchronization to prevent race conditions in shared dictionary access Add thread synchronization around the paths dictionary access to ensure thread-safety when multiple threads access the cache simultaneously. dotnet/src/webdriver/DriverFinder.cs [109-113] -if (paths.TryGetValue(DriverPathKey, out string? cachedDriverPath) - && !string.IsNullOrWhiteSpace(cachedDriverPath)) +lock (paths) { - return paths; + if (paths.TryGetValue(DriverPathKey, out string? cachedDriverPath) + && !string.IsNullOrWhiteSpace(cachedDriverPath)) + { + return new Dictionary<string, string>(paths); + } } Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion improves thread safety by adding proper synchronization around the shared dictionary access, preventing potential race conditions in multi-threaded scenarios.	7
	Add parameter validation to prevent null reference exceptions Add null check for options parameter in CreateCommandExecutor method to prevent potential NullReferenceException. dotnet/src/webdriver/DriverService.cs [439-443] internal DriverServiceCommandExecutor CreateCommandExecutor(DriverOptions options, TimeSpan commandTimeout, bool searchForBrowserPath) { + if (options == null) + { + throw new ArgumentNullException(nameof(options)); + } if (DriverServicePath == null) { DriverFinder finder = new DriverFinder(options); Apply this suggestion Suggestion importance[1-10]: 3 Why: While the suggestion adds a null check, it's less critical since the DriverFinder constructor already includes null validation for the options parameter, making this check redundant.	3

[RenderMichael]

Best to add both after exceptions, because paths is a shared state which can be corrupted if used wrong.

[nvborisenko]

Setting binary location for IE is not supported (at runtime).

[RenderMichael]

I have the bool searchForBrowserPath set to false on IE and Safari, so this should not be a problem. https://github.com/SeleniumHQ/selenium/pull/15028/files#diff-628f59035c50241ea1a690b5e923fccb569486eea8d138a6a1c08b9bbf78258dR145

[diemol]

@RenderMichael @nvborisenko will this be merged or should we close it?

[nvborisenko]

This is something from minor priority box. Personally I don't like the idea to introduce flags in constructors. Up to Mike, please re-evaluate it and go ahead.

[RenderMichael]

This seemed like a nice and simple code deduplication. I don’t have strong opinions about the code being this way vs. the current duplication. Since there’s pushback, I’ll close this.