Updating .NET Firefox driver to use standard init and terminate

This commit modifies the Marionette driver to test for initialization
using a URL poll instead of a hard-coded sleep. We poll for the URL to
delete a nonexistent session. This still throws an error, but one that
still lets us know that the HTTP server is available and responding to
HTTP requests.

We also now terminate the Firefox driver executable immediately rather
than attempting to send a shutdown command. Since the Mozilla
implementation slavishly follows the specification, and there is no server
management API in the spec, nor is there likely to be, no matter how
useful such a thing would be, we will settle for forcing the executable
process to terminate. This is a suboptimal approach, but is required by
the current architecture.

Author: jimevans

dotnet/src/webdriver/DriverService.cs

@@ -160,19 +160,27 @@ protected virtual string CommandLineArguments
         /// <summary>
         /// Gets a value indicating the time to wait for an initial connection before timing out.
         /// </summary>
-        protected virtual TimeSpan InitialConnectionTimeout
+        protected virtual TimeSpan InitializationTimeout
         {
             get { return TimeSpan.FromSeconds(20); }
         }
 
+        /// <summary>
+        /// Gets a value indicating the time to wait for the service to terminate before forcing it to terminate.
+        /// </summary>
+        protected virtual TimeSpan TerminationTimeout
+        {
+            get { return TimeSpan.FromSeconds(10); }
+        }
+
         /// <summary>
         /// Gets a value indicating whether the service is responding to HTTP requests.
         /// </summary>
-        protected virtual bool IsAvailable
+        protected virtual bool IsInitialized
         {
             get
             {
-                bool isAvailable = false;
+                bool isInitialized = false;
                 try
                 {
                     Uri serviceHealthUri = new Uri(this.ServiceUrl, new Uri(DriverCommand.Status, UriKind.Relative));
@@ -185,15 +193,15 @@ protected virtual bool IsAvailable
                     // that the HTTP status returned is a 200 status, and that the resposne has the correct
                     // Content-Type header. A more sophisticated check would parse the JSON response and
                     // validate its values. At the moment we do not do this more sophisticated check.
-                    isAvailable = response.StatusCode == HttpStatusCode.OK && response.ContentType.StartsWith("application/json", StringComparison.OrdinalIgnoreCase);
+                    isInitialized = response.StatusCode == HttpStatusCode.OK && response.ContentType.StartsWith("application/json", StringComparison.OrdinalIgnoreCase);
                     response.Close();
                 }
                 catch (WebException ex)
                 {
                     Console.WriteLine(ex.Message);
                 }
 
-                return isAvailable;
+                return isInitialized;
             }
         }
 
@@ -218,7 +226,7 @@ public void Start()
             this.driverServiceProcess.StartInfo.UseShellExecute = false;
             this.driverServiceProcess.StartInfo.CreateNoWindow = this.hideCommandPromptWindow;
             this.driverServiceProcess.Start();
-            bool serviceAvailable = WaitForServiceAvailable();
+            bool serviceAvailable = WaitForServiceInitialization();
 
             if (!serviceAvailable)
             {
@@ -265,12 +273,11 @@ protected virtual void Dispose(bool disposing)
         [SecurityPermission(SecurityAction.Demand)]
         private void Stop()
         {
-            if (this.driverServiceProcess != null && !this.driverServiceProcess.HasExited)
+            if (this.IsRunning)
             {
                 Uri shutdownUrl = new Uri(this.ServiceUrl, "/shutdown");
-                DateTime timeout = DateTime.Now.Add(TimeSpan.FromSeconds(10));
-                bool processStopped = false;
-                while (!processStopped && DateTime.Now < timeout)
+                DateTime timeout = DateTime.Now.Add(this.TerminationTimeout);
+                while (this.IsRunning && DateTime.Now < timeout)
                 {
                     try
                     {
@@ -284,20 +291,18 @@ private void Stop()
                         HttpWebResponse response = request.GetResponse() as HttpWebResponse;
                         response.Close();
                         this.driverServiceProcess.WaitForExit(3000);
-                        processStopped = this.driverServiceProcess.HasExited;
                     }
                     catch (WebException)
                     {
-                        processStopped = true;
                     }
                 }
 
                 // If at this point, the process still hasn't exited, wait for one
                 // last-ditch time, then, if it still hasn't exited, kill it. Note
                 // that falling into this branch of code should be exceedingly rare.
-                if (!this.driverServiceProcess.HasExited)
+                if (this.IsRunning)
                 {
-                    this.driverServiceProcess.WaitForExit(5000);
+                    this.driverServiceProcess.WaitForExit(Convert.ToInt32(this.TerminationTimeout.TotalMilliseconds));
                     if (!this.driverServiceProcess.HasExited)
                     {
                         this.driverServiceProcess.Kill();
@@ -310,27 +315,27 @@ private void Stop()
         }
 
         /// <summary>
-        /// Waits until a the service is available, or the timeout set
-        /// by the <see cref="InitialConnectionTimeout"/> property is reached.
+        /// Waits until a the service is initialized, or the timeout set
+        /// by the <see cref="InitializationTimeout"/> property is reached.
         /// </summary>
         /// <returns><see langword="true"/> if the service is properly started and receiving HTTP requests;
         /// otherwise; <see langword="false"/>.</returns>
-        private bool WaitForServiceAvailable()
+        private bool WaitForServiceInitialization()
         {
-            bool isAvailable = false;
-            DateTime timeout = DateTime.Now.Add(this.InitialConnectionTimeout);
-            while (!isAvailable && DateTime.Now < timeout)
+            bool isInitialized = false;
+            DateTime timeout = DateTime.Now.Add(this.InitializationTimeout);
+            while (!isInitialized && DateTime.Now < timeout)
             {
                 // If the driver service process has exited, we can exit early.
                 if (!this.IsRunning)
                 {
                     break;
                 }
 
-                isAvailable = this.IsAvailable;
+                isInitialized = this.IsInitialized;
             }
 
-            return isAvailable;
+            return isInitialized;
         }
     }
 }

dotnet/src/webdriver/Firefox/FirefoxDriverService.cs

@@ -22,6 +22,8 @@
 using System.IO;
 using System.Text;
 using OpenQA.Selenium.Internal;
+using System.Net;
+using OpenQA.Selenium.Remote;
 
 namespace OpenQA.Selenium.Firefox
 {
@@ -67,20 +69,76 @@ public int BrowserCommunicationPort
         /// <summary>
         /// Gets a value indicating the time to wait for an initial connection before timing out.
         /// </summary>
-        protected override TimeSpan InitialConnectionTimeout
+        protected override TimeSpan InitializationTimeout
         {
             get { return TimeSpan.FromSeconds(2); }
         }
 
+        /// <summary>
+        /// Gets a value indicating the time to wait for the service to terminate before forcing it to terminate.
+        /// </summary>
+        protected override TimeSpan TerminationTimeout
+        {
+            // Use a very small timeout for terminating the Firefox driver,
+            // because the executable does not have a clean shutdown command,
+            // which means we have to kill the process. Using a short timeout
+            // gets us to the termination point much faster.
+            get { return TimeSpan.FromMilliseconds(100); }
+        }
+
         /// <summary>
         /// Gets a value indicating whether the service is responding to HTTP requests.
         /// </summary>
-        protected override bool IsAvailable
+        protected override bool IsInitialized
         {
             get
             {
-                System.Threading.Thread.Sleep(InitialConnectionTimeout);
-                return true;
+                bool isInitialized = false;
+                try
+                {
+                    // Since Firefox driver won't implement the /session end point (because
+                    // the W3C spec working group stupidly decided that it isn't necessary),
+                    // we'll attempt to poll for a different URL which has no side effects.
+                    // We've chosen to poll on the "quit" URL, passing in a nonexistent
+                    // session id.
+                    Uri serviceHealthUri = new Uri(this.ServiceUrl, new Uri("/session/FakeSessionIdForPollingPurposes", UriKind.Relative));
+                    HttpWebRequest request = HttpWebRequest.Create(serviceHealthUri) as HttpWebRequest;
+                    request.KeepAlive = false;
+                    request.Timeout = 5000;
+                    request.Method = "DELETE";
+                    HttpWebResponse response = request.GetResponse() as HttpWebResponse;
+
+                    // Checking the response from deleting a nonexistent session. Note that we are simply
+                    // checking that the HTTP status returned is a 200 status, and that the resposne has
+                    // the correct Content-Type header. A more sophisticated check would parse the JSON
+                    // response and validate its values. At the moment we do not do this more sophisticated
+                    // check.
+                    isInitialized = response.StatusCode == HttpStatusCode.OK && response.ContentType.StartsWith("application/json", StringComparison.OrdinalIgnoreCase);
+                    response.Close();
+                }
+                catch (WebException ex)
+                {
+                    // Because the Firefox driver (incorrectly) does not allow quit on a
+                    // nonexistent session to succeed, this will throw a WebException,
+                    // which means we're reduced to using exception handling for flow control.
+                    // This situation is highly undesirable, and in fact is a horrible code
+                    // smell, but the implementation leaves us no choice. So we will check for
+                    // the known response code and content type header, just like we would for
+                    // the success case. Either way, a valid HTTP response instead of a socket
+                    // error would tell us that the HTTP server is responding to requests, which
+                    // is really what we want anyway.
+                    HttpWebResponse errorResponse = ex.Response as HttpWebResponse;
+                    if (errorResponse != null)
+                    {
+                        isInitialized = errorResponse.StatusCode == HttpStatusCode.InternalServerError && errorResponse.ContentType.StartsWith("application/json", StringComparison.OrdinalIgnoreCase);
+                    }
+                    else
+                    {
+                        Console.WriteLine(ex.Message);
+                    }
+                }
+
+                return isInitialized;
             }
         }