[dotnet] quit fails after not successful new session

[Indomitable]

User description

If new session creation fails a Quit() method is called which calls Dispose and then Quit command, unfortunately in this case session is still not created and it throws null reference exception and this hides the actual exception

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

Motivation and Context

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.

This PR fixes a bug which calls Quit command after trying to create new session. If session fails it calls the Quit method in order to dispose the driver and the commander, unfortunately since the session id is null the Execute Quit command fails with null reference exception.
For reference in Java client this is solved with another try/catch when calling quit() but I think checking the session id if null is better solution.

---

PR Type

Bug fix

---

Description

• Added a null check for sessionId before executing the Quit command in Dispose method to prevent null reference exceptions when session creation fails.
• Ensures that the actual exception is not hidden by a subsequent null reference exception.

---

Changes walkthrough 📝

	Relevant files
Bug fix	WebDriver.cs Add null check for sessionId before executing Quit command dotnet/src/webdriver/WebDriver.cs Added a null check for sessionId before executing the Quit command. Prevents null reference exception when session creation fails. +4/-1

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[CLAassistant]

All committers have signed the CLA.

[qodo-merge-pro]

PR Reviewer Guide 🔍

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ No key issues to review

[qodo-merge-pro]

PR Code Suggestions ✨

Category	Suggestion	Score
Robustness	Handle exceptions for the Quit command to enhance robustness It's a good practice to handle potential exceptions that might be thrown by the Execute method, especially for critical commands like Quit. Consider wrapping the Execute call in a try-catch block to handle possible exceptions gracefully. dotnet/src/webdriver/WebDriver.cs [706-708] if (this.sessionId is not null) { - this.Execute(DriverCommand.Quit, null); + try + { + this.Execute(DriverCommand.Quit, null); + } + catch (Exception ex) + { + Logger.Log($"Failed to quit the driver session: {ex.Message}"); + } } Apply this suggestion Suggestion importance[1-10]: 8 Why: Wrapping the Execute call in a try-catch block to handle possible exceptions gracefully is a robust practice. It ensures that any issues during the Quit command are logged and managed, improving the reliability of the code.	8
Maintainability	Add logging before executing a command to enhance traceability and debugging Consider adding a logging statement before executing the Quit command. This will help in debugging and understanding the flow, especially when the session is not null but the Quit command fails or behaves unexpectedly. dotnet/src/webdriver/WebDriver.cs [706-708] if (this.sessionId is not null) { + Logger.Log("Attempting to quit the driver session."); this.Execute(DriverCommand.Quit, null); } Apply this suggestion Suggestion importance[1-10]: 7 Why: Adding logging can help in debugging and understanding the flow, especially when the session is not null but the Quit command fails or behaves unexpectedly. This is a good practice for maintainability and traceability.	7
	Refactor session quitting logic into a separate method to improve readability To improve code readability and maintainability, consider refactoring the condition and the command execution into a separate method. This encapsulation can make the Dispose method cleaner and easier to manage. dotnet/src/webdriver/WebDriver.cs [706-708] -if (this.sessionId is not null) +AttemptToQuitSession(); + +// Elsewhere in the class +private void AttemptToQuitSession() { - this.Execute(DriverCommand.Quit, null); + if (this.sessionId is not null) + { + this.Execute(DriverCommand.Quit, null); + } } Apply this suggestion Suggestion importance[1-10]: 6 Why: Refactoring the condition and command execution into a separate method can improve code readability and maintainability. However, it adds an additional method which might be unnecessary for such a small block of code.	6
Best practice	Add a finally block to ensure resources are cleaned up regardless of session existence To ensure that all resources are properly released, consider adding a finally block that handles any necessary cleanup actions, regardless of whether the session exists or not. dotnet/src/webdriver/WebDriver.cs [706-708] if (this.sessionId is not null) { this.Execute(DriverCommand.Quit, null); } +finally +{ + CleanUpResources(); +} Apply this suggestion Suggestion importance[1-10]: 5 Why: While adding a finally block can ensure resources are cleaned up, the suggestion does not provide specific details on what CleanUpResources entails, making it less clear how it improves the current implementation.	5

[nvborisenko]

@Indomitable please share the exception with stacktrace you faced and you are trying to fix. Probably we may propose better solution.

[Indomitable]

@nvborisenko Here is my input:
The exception thrown is NullReferenceException. What is the case.
The request for StartSession throws exception

selenium/dotnet/src/webdriver/WebDriver.cs

Line 651 in 6b28a8c

Response response = this.Execute(DriverCommand.NewSession, parameters);

this leads to calling the Quit() in the catch

selenium/dotnet/src/webdriver/WebDriver.cs

Line 66 in 6b28a8c

this.Quit();

which calls Dispose and then the Quit command

selenium/dotnet/src/webdriver/WebDriver.cs

Line 706 in 6b28a8c

this.Execute(DriverCommand.Quit, null);

but since the sessionId is not initialized yet , a NullReferenceException is thrown when tries to build the URL for the quit command

selenium/dotnet/src/webdriver/HttpCommandInfo.cs

Line 128 in 6b28a8c

propertyValue = commandToExecute.SessionId.ToString();

As I wrote in the Java client this is fixed using second try/catch for the quit method

selenium/java/src/org/openqa/selenium/remote/RemoteWebDriver.java

Lines 160 to 170 in 6b28a8c

	try {
	startSession(capabilities);
	} catch (RuntimeException e) {
	try {
	quit();
	} catch (Exception ignored) {
	// Ignore the clean-up exception. We'll propagate the original failure.
	}
	throw e;
	}

[nvborisenko]

I understand the flow how it is happening. I am curious in the stracktrace (what you see in your output).

My thoughts:

• I see invoking of Dispose in ctor, it is not good
• I see NullReferenceException which probably can be avoided in some another way rather than try/catch

[Indomitable]

Here is the stack trace. I'm using the Appium.NET library which uses Selenium
Actually it looks like I'm a bit of wrong. Since the SessionId is empty it tries to make a DELETE /session/ request.
The result is empty body but with content type of application/json and then in the FromErrorJson it can not convert the body to Dictionary.

Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
   at OpenQA.Selenium.Response.FromErrorJson(String value)
   at OpenQA.Selenium.Remote.HttpCommandExecutor.CreateResponse(HttpResponseInfo responseInfo)
   at OpenQA.Selenium.Remote.HttpCommandExecutor.Execute(Command commandToExecute)
   at OpenQA.Selenium.Appium.Service.AppiumCommandExecutor.Execute(Command commandToExecute)
   at OpenQA.Selenium.WebDriver.Execute(String driverCommandToExecute, Dictionary`2 parameters)
   at OpenQA.Selenium.Appium.AppiumDriver.Execute(String driverCommandToExecute, Dictionary`2 parameters)
   at OpenQA.Selenium.WebDriver.Dispose(Boolean disposing)
   at OpenQA.Selenium.WebDriver.Dispose()
   at OpenQA.Selenium.WebDriver.Quit()
   at OpenQA.Selenium.WebDriver..ctor(ICommandExecutor executor, ICapabilities capabilities)
   at OpenQA.Selenium.Appium.AppiumDriver..ctor(ICommandExecutor commandExecutor, ICapabilities appiumOptions)
   at OpenQA.Selenium.Appium.AppiumDriver..ctor(Uri remoteAddress, ICapabilities appiumOptions, TimeSpan commandTimeout, AppiumClientConfig clientConfig)
   at OpenQA.Selenium.Appium.AppiumDriver..ctor(Uri remoteAddress, ICapabilities appiumOptions, TimeSpan commandTimeout)
   at OpenQA.Selenium.Appium.AppiumDriver..ctor(Uri remoteAddress, ICapabilities appiumOptions)
   at OpenQA.Selenium.Appium.iOS.IOSDriver..ctor(Uri remoteAddress, DriverOptions driverOptions)

[Indomitable]

So it depends from the server. In this case the Appium server returns empty body and we get json deserialization issue. One more problem that I see is that a DELETE /session/ call is made which may result to undefined behavior.

[nvborisenko]

@Indomitable Ok, seems I am right that Dispose() in ctor is unnecessary. But, I don't want to introduce old/new issues :D

I propose to align with java implementation, probably they know hidden stones:

selenium/java/src/org/openqa/selenium/remote/RemoteWebDriver.java

Line 160 in 785914e

try {

So in ctor let's try/catch Quit() invocation and that's it! Your current changes, you made, are also acceptable.

[Indomitable]

PR is updated with the try/catch around the Quit.

[nvborisenko]

Thanks for your contribution!