Bug 1403923 - Safely shutdown Firefox from in delete_session. r=jgraham
☠☠ backed out by dffc508ff053 ☠ ☠
authorHenrik Skupin <mail@hskupin.info>
Thu, 05 Oct 2017 23:49:17 +0200
changeset 396542 34faef444ae338b25ef7eb33ab15e60e9e52e8b8
parent 396541 ff0c2391364e596ddfddafe31e88466a3c65c811
child 396543 fe85732c14da164231e9ae3889b0f5a5ba5dc3ff
push id57032
push userhskupin@mozilla.com
push dateFri, 15 Dec 2017 14:53:12 +0000
treeherderautoland@34faef444ae3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjgraham
bugs1403923
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1403923 - Safely shutdown Firefox from in delete_session. r=jgraham With the request to shutdown the browser, a given amount of time has to be waited to allow the process to shutdown itself. Only if the process is still running afterward it has to be killed. Firefox has an integrated background monitor which observes long running threads during shutdown, and kills those after 120s. To allow Firefox to shutdown on its own, geckodriver has to wait that time, and some additional seconds. MozReview-Commit-ID: 4LRLQE0jZzw
testing/geckodriver/CHANGES.md
testing/geckodriver/src/marionette.rs
testing/webdriver/src/server.rs
--- a/testing/geckodriver/CHANGES.md
+++ b/testing/geckodriver/CHANGES.md
@@ -9,16 +9,19 @@ Unreleased
 ### Changed
 
 - HTTP status code for the [`StaleElementReference`] error changed
   from 400 (Bad Request) to 404 (Not Found)
 
 - Backtraces from geckodriver no longer substitute for missing
   Marionette stacktraces
 
+- `Delete Session` now allows Firefox to safely shutdown within 130s before
+  force-killing the process
+
 
 0.19.1 (2017-10-30)
 -------------------
 
 ### Changed
 
 - Search suggestions in the location bar turned off as not to
   trigger network connections
--- a/testing/geckodriver/src/marionette.rs
+++ b/testing/geckodriver/src/marionette.rs
@@ -52,16 +52,20 @@ use webdriver::error::{ErrorStatus, WebD
 use webdriver::server::{WebDriverHandler, Session};
 use webdriver::httpapi::{WebDriverExtensionRoute};
 
 use capabilities::{FirefoxCapabilities, FirefoxOptions};
 use prefs;
 
 const DEFAULT_HOST: &'static str = "localhost";
 
+// Firefox' integrated background monitor which observes long running threads during
+// shutdown kills those after 120s. Wait some additional seconds for a safe shutdown
+const TIMEOUT_BROWSER_SHUTDOWN: u64 = 130 * 1000;
+
 pub fn extension_routes() -> Vec<(Method, &'static str, GeckoExtensionRoute)> {
     return vec![(Method::Get, "/session/{sessionId}/moz/context", GeckoExtensionRoute::GetContext),
              (Method::Post, "/session/{sessionId}/moz/context", GeckoExtensionRoute::SetContext),
              (Method::Post,
               "/session/{sessionId}/moz/xbl/{elementId}/anonymous_children",
               GeckoExtensionRoute::XblAnonymousChildren),
              (Method::Post,
               "/session/{sessionId}/moz/xbl/{elementId}/anonymous_by_attribute",
@@ -568,28 +572,61 @@ impl WebDriverHandler<GeckoExtensionRout
             Err(_) => {
                 Err(WebDriverError::new(
                     ErrorStatus::UnknownError,
                     "Failed to aquire Marionette connection"))
             }
         }
     }
 
-    fn delete_session(&mut self, _: &Option<Session>) {
-        if let Ok(connection) = self.connection.lock() {
-            if let Some(ref conn) = *connection {
+    fn delete_session(&mut self, session: &Option<Session>) {
+        // If there is still an active session send a delete session command
+        // and wait for the browser to quit
+        if let Some(ref s) = *session {
+            let delete_session = WebDriverMessage {
+                session_id: Some(s.id.clone()),
+                command: WebDriverCommand::DeleteSession
+            };
+            let _ = self.handle_command(session, delete_session);
+
+            if let Some(ref mut runner) = self.browser {
+                let timeout = TIMEOUT_BROWSER_SHUTDOWN;
+                let poll_interval = 100;
+                let poll_attempts = timeout / poll_interval;
+                let mut poll_attempt = 0;
+
+                while runner.is_running() {
+                    if poll_attempt <= poll_attempts {
+                        debug!("Waiting for the browser process to shutdown");
+                        poll_attempt += 1;
+                        sleep(Duration::from_millis(poll_interval));
+                    } else {
+                        warn!("Browser process did not shutdown");
+                        break;
+                    }
+                }
+            }
+        }
+
+        if let Ok(ref mut connection) = self.connection.lock() {
+            if let Some(conn) = connection.as_mut() {
                 conn.close();
             }
         }
+
+        // If the browser is still open then kill the process
         if let Some(ref mut runner) = self.browser {
-            debug!("Stopping browser process");
-            if runner.stop().is_err() {
-                error!("Failed to kill browser process");
-            };
+            if runner.is_running() {
+                info!("Forcing a shutdown of the browser process");
+                if runner.stop().is_err() {
+                    error!("Failed to kill browser process");
+                };
+            }
         }
+
         self.connection = Mutex::new(None);
         self.browser = None;
     }
 }
 
 pub struct MarionetteSession {
     pub session_id: String,
     protocol: Option<String>,
--- a/testing/webdriver/src/server.rs
+++ b/testing/webdriver/src/server.rs
@@ -20,17 +20,17 @@ use response::{CloseWindowResponse, WebD
 
 enum DispatchMessage<U: WebDriverExtensionRoute> {
     HandleWebDriver(WebDriverMessage<U>, Sender<WebDriverResult<WebDriverResponse>>),
     Quit
 }
 
 #[derive(Clone, Debug, PartialEq)]
 pub struct Session {
-    id: String
+    pub id: String
 }
 
 impl Session {
     fn new(id: String) -> Session {
         Session {
             id: id
         }
     }