[dotnet] IJavascriptEngine implements IDisposable where available

[RenderMichael]

JavascriptExecutor takes an IWebDriver and, on a lazy basis, opens a DevToolsSession which it currently leaves open. This PR disposes on that DevToolsSession if it was instantiated.

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

Closes the DevToolsSession in JavascriptEngine if it's been opened.

Motivation and Context

cq

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.

[CLAassistant]

All committers have signed the CLA.

[diemol]

Is there a test or something we can see the difference in behavior? The explanation makes sense, but I really don't understand Dispose at all and don't see how the description results in that code?

[RenderMichael]

Is there a test or something we can see the difference in behavior?

This doesn't change behavior, it's just that there's an open session hanging around, beyond the lifespan of the IJavascriptEngine.

I really don't understand Dispose at all and don't see how the description results in that code?

Sure, so the JavascriptEngine class has a private Lazy<DevToolsSession> object named session which is, as the class name suggests, lazily evaluated whenever it's accessed. So there's a DevToolsSession owned by this class.

However, DevToolsSession is itself an IDisposable type, which needs to have Dispose() called on it before it's out of reach. Maybe it has an open network connection, or something that doesn't just go away when it's no longer referenced, not sure.

However, as it stands, DevToolsSession.Dispose() is never called on the session object. Without looking into the implementation details of DevToolsSession, it's hard to say what this means, only that since it implements IDisposable, developers expect Dispose() to be called.

This PR passes that along to the JavascriptEngine class which owns session, such that disposing the JavascriptEngine will dispose the underlying session, if it had been lazily evaluated.

[RenderMichael]

don't see how the description results in that code

This is the standard dispose pattern, where Dispose() is a sealed method, while Dispose(bool) can be overridden by a derived class. So if another class extends JavascriptEngine, they can do

protected override void Dispose(bool disposing)
{
    if (disposing)
    {
        // whatever additional disposal you need
    }
    base.Dispose(disposing);
}

but users of the derived class still only need to call Dispose() to do it all

[diemol]

Thank you for the explanation, makes sense. I will wait for the CI and then merge.

Looking forward to future contributions!