FirefoxDriverService ProcessId property getting cleared when creating a FirefoxDriver

[jordan-mace]

🐛 Bug Report

The ProcessId of a FirefoxDriverService is getting replaced with 0 after creating a FirefoxDriver using the service.

From my investigation, this appears to be caused by DriverService.IsRunning. this.driverServiceProcess.HasExited begins returning true as soon as the FirefoxDriver is created despite the geckodriver process being alive. IsRunning therefore returns false, so ProcessId returns 0.

It's worth noting that I have seen this occur on both .NET Core 2.2 and 3.1 (the repro project)

To Reproduce

Effectively,

1. Create a FirefoxDriverService
2. Using that service, create a new FirefoxDriver
3. Access the ProcessId property on the FirefoxDriverService

Expected behavior

The FirefoxDriverService ProcessId should not change as long as the geckodriver is running.

Test script or set of commands reproducing this issue

Repo: https://github.com/jordanm-mel/process-id-bug-repro

Environment

OS: Windows 10
Browser: Firefox
Browser version: 72.0.2
Browser Driver version: GeckoDriver 0.26.0
Language Bindings version: C# 3.141.0
Selenium Grid version (if applicable): N/A

[jimevans]

@jordanm-mel Given that HasExited is a method on the framework-provided System.Diagnostics.Process object, and that the .NET bindings have no control over what that property returns, how would you suggest we proceed?

[jordan-mace]

The reason I raise it with the .NET bindings is because at first the service has all of the correct properties (leading me to believe that System.Diagnostics.Process is not broken). Once new FirefoxDriver() is called it fires off DriverCommand.NewSession, this calls DriverService.Start() (whereas the service has already been started at this point).

This then replaces the value of this.driverServiceProcess (which is where I believe this issue is originating. Creating a Process and then replacing it with a new Process). I would likely then just propose a simple not null check within DriverService.Start to return at the beginning of the method if this.driverServiceProcess is not null. I'd be happy to put that PR together if you'd like

[jimevans]

Okay, I’ve looked at your repro case, and you’re using FirefoxDriverService contrary to how it’s intended to be used. If you’re using FirefoxDriver, don’t call the Start() method on the service. By using the strongly-typed driver class, the bindings are expecting that you want them to manage the lifetime of the geckodriver process, including launching it and terminating it. The method is provided as a public method so it can be used with the RemoteWebDriver class, which expects that you, not the bindings, will manage the lifetime of the process.

Yes, there’s a bug here, but given the intended use cases, I’m not sure we’ve fully considered all of the potential ramifications of implementing the fix you propose. You’re welcome to submit a PR, but in the interest of transparency, reviewing and merging it will not be a priority for me due to other commitments at this time.

In the meantime, don’t manually call the Start() method in your code. After the new session has been created, you’ll still have access to the ID of the process via the service object instance’s properties.

[jordan-mace]

@jimevans created a PR. Turns out this issue has already been fixed in the Java bindings (null check, so I'm pretty confident that this shouldn't introduce any unintended effects)

Understandable about your priorities. Not fussed if this one waits a while (since a pretty simple workaround exists, as you have pointed out)

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.