Bug 1694171 - [marionette] Notify all observers before calling Services.startup.quit(). r=marionette-reviewers,jdescottes

For a shutdown attempt all windows should be informed first so
that various components can safely do clean-up work.

If something prevents the shutdown Marionette should force it.
This will make sure that no application process remains,
or would have to be force killed externally via its PID.

Differential Revision: https://phabricator.services.mozilla.com/D107044

testing/marionette/client/marionette_driver/marionette.py

@@ -927,7 +927,10 @@ class Marionette(object):
         :throws InvalidArgumentException: If there are multiple
             `shutdown_flags` ending with `"Quit"`.
 
-        :returns: The cause of shutdown.
+        :returns: A dictionary containing details of the application shutdown.
+                  The `cause` property reflects the reason, and `forced` indicates
+                  that something prevented the shutdown and the application had
+                  to be forced to shutdown.
         """
 
         # The vast majority of this function was implemented inside
@@ -941,29 +944,11 @@ class Marionette(object):
         if not any(flag.endswith("Quit") for flag in flags):
             flags = flags | set(("eForceQuit",))
 
-        # Trigger a quit-application-requested observer notification
-        # so that components can safely shutdown before quitting the
-        # application.
-        with self.using_context("chrome"):
-            canceled = self.execute_script(
-                """
-                Components.utils.import("resource://gre/modules/Services.jsm");
-                let cancelQuit = Components.classes["@mozilla.org/supports-PRBool;1"]
-                    .createInstance(Components.interfaces.nsISupportsPRBool);
-                Services.obs.notifyObservers(cancelQuit, "quit-application-requested", null);
-                return cancelQuit.data;
-                """
-            )
-            if canceled:
-                raise errors.MarionetteException(
-                    "Something cancelled the quit application request"
-                )
-
         body = None
         if len(flags) > 0:
             body = {"flags": list(flags)}
 
-        return self._send_message("Marionette:Quit", body, key="cause")
+        return self._send_message("Marionette:Quit", body)
 
     @do_process_check
     def quit(self, clean=False, in_app=False, callback=None):
@@ -981,13 +966,18 @@ class Marionette(object):
                        by killing the process.
         :param callback: If provided and `in_app` is True, the callback will
                          be used to trigger the shutdown.
+
+        :returns: A dictionary containing details of the application shutdown.
+                  The `cause` property reflects the reason, and `forced` indicates
+                  that something prevented the shutdown and the application had
+                  to be forced to shutdown.
         """
         if not self.instance:
             raise errors.MarionetteException(
                 "quit() can only be called " "on Gecko instances launched by Marionette"
             )
 
-        cause = None
+        quit_details = {"cause": "shutdown", "forced": False}
         if in_app:
             if callback is not None and not callable(callback):
                 raise ValueError(
@@ -1002,7 +992,7 @@ class Marionette(object):
                 if callback is not None:
                     callback()
                 else:
-                    cause = self._request_in_app_shutdown()
+                    quit_details = self._request_in_app_shutdown()
 
             except IOError:
                 # A possible IOError should be ignored at this point, given that
@@ -1025,12 +1015,16 @@ class Marionette(object):
             self.delete_session(send_request=False)
             self.instance.close(clean=clean)
 
-        if cause not in (None, "shutdown"):
+            quit_details["forced"] = True
+
+        if quit_details.get("cause") not in (None, "shutdown"):
             raise errors.MarionetteException(
                 "Unexpected shutdown reason '{}' for "
-                "quitting the process.".format(cause)
+                "quitting the process.".format(quit_details["cause"])
             )
 
+        return quit_details
+
     @do_process_check
     def restart(self, clean=False, in_app=False, callback=None):
         """
@@ -1045,6 +1039,11 @@ class Marionette(object):
                        by killing the process.
         :param callback: If provided and `in_app` is True, the callback will be
                          used to trigger the restart.
+
+        :returns: A dictionary containing details of the application restart.
+                  The `cause` property reflects the reason, and `forced` indicates
+                  that something prevented the shutdown and the application had
+                  to be forced to shutdown.
         """
         if not self.instance:
             raise errors.MarionetteException(
@@ -1053,7 +1052,7 @@ class Marionette(object):
             )
         context = self._send_message("Marionette:GetContext", key="value")
 
-        cause = None
+        restart_details = {"cause": "restart", "forced": False}
         if in_app:
             if clean:
                 raise ValueError(
@@ -1073,7 +1072,7 @@ class Marionette(object):
                 if callback is not None:
                     callback()
                 else:
-                    cause = self._request_in_app_shutdown("eRestart")
+                    restart_details = self._request_in_app_shutdown("eRestart")
 
             except IOError:
                 # A possible IOError should be ignored at this point, given that
@@ -1118,10 +1117,12 @@ class Marionette(object):
             self.instance.restart(clean=clean)
             self.raise_for_port(timeout=self.DEFAULT_STARTUP_TIMEOUT)
 
-        if cause not in (None, "restart"):
+            restart_details["forced"] = True
+
+        if restart_details.get("cause") not in (None, "restart"):
             raise errors.MarionetteException(
                 "Unexpected shutdown reason '{}' for "
-                "restarting the process".format(cause)
+                "restarting the process".format(restart_details["cause"])
             )
 
         self.start_session()
@@ -1134,6 +1135,8 @@ class Marionette(object):
             # informing about the new process id.
             self.instance.runner.process_handler.check_for_detached(self.process_id)
 
+        return restart_details
+
     def absolute_url(self, relative_url):
         """
         Returns an absolute url for files served from Marionette's www directory.

testing/marionette/driver.js

@@ -2844,10 +2844,11 @@ GeckoDriver.prototype.acceptConnections = function(cmd) {
  *     Constant name of masks to pass to |Services.startup.quit|.
  *     If empty or undefined, |nsIAppStartup.eAttemptQuit| is used.
  *
- * @return {string}
- *     Explaining the reason why the application quit.  This can be
- *     in response to a normal shutdown or restart, yielding "shutdown"
- *     or "restart", respectively.
+ * @return {Object<string,boolean>}
+ *     Dictionary containing information that explains the shutdown reason.
+ *     The value for `cause` contains the shutdown kind like "shutdown" or
+ *     "restart", while `forced` will indicate if it was a normal or forced
+ *     shutdown of the application.
  *
  * @throws {InvalidArgumentError}
  *     If <var>flags</var> contains unknown or incompatible flags,
@@ -2885,11 +2886,25 @@ GeckoDriver.prototype.quit = async function(cmd) {
   this._server.acceptConnections = false;
   this.deleteSession();
 
+  // Notify all windows that an application quit has been requested.
+  const cancelQuit = Cc["@mozilla.org/supports-PRBool;1"].createInstance(
+    Ci.nsISupportsPRBool
+  );
+  Services.obs.notifyObservers(cancelQuit, "quit-application-requested");
+
+  // If the shutdown of the application is prevented force quit it instead.
+  if (cancelQuit.data) {
+    mode |= Ci.nsIAppStartup.eForceQuit;
+  }
+
   // delay response until the application is about to quit
   let quitApplication = waitForObserverTopic("quit-application");
   Services.startup.quit(mode);
 
-  return { cause: (await quitApplication).data };
+  return {
+    cause: (await quitApplication).data,
+    forced: cancelQuit.data,
+  };
 };
 
 GeckoDriver.prototype.installAddon = function(cmd) {

testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py

@@ -176,7 +176,33 @@ class TestQuitRestart(MarionetteTestCase):
             self.marionette.restart(in_app=True, clean=True)
 
     def test_in_app_restart(self):
-        self.marionette.restart(in_app=True)
+        details = self.marionette.restart(in_app=True)
+
+        self.assertFalse(details["forced"], "Expected non-forced shutdown")
+
+        self.assertEqual(self.marionette.profile, self.profile)
+        self.assertNotEqual(self.marionette.session_id, self.session_id)
+
+        self.assertNotEqual(self.marionette.process_id, self.pid)
+
+        self.assertNotEqual(
+            self.marionette.get_pref("startup.homepage_welcome_url"), "about:about"
+        )
+
+    def test_in_app_restart_component_prevents_shutdown(self):
+        with self.marionette.using_context("chrome"):
+            self.marionette.execute_script(
+                """
+                Services.obs.addObserver(subject => {
+                  let cancelQuit = subject.QueryInterface(Ci.nsISupportsPRBool);
+                  cancelQuit.data = true;
+                }, "quit-application-requested");
+                """
+            )
+
+        details = self.marionette.restart(in_app=True)
+
+        self.assertTrue(details["forced"], "Expected forced shutdown")
 
         self.assertEqual(self.marionette.profile, self.profile)
         self.assertNotEqual(self.marionette.session_id, self.session_id)
@@ -267,7 +293,37 @@ class TestQuitRestart(MarionetteTestCase):
             self.marionette.quit(clean=True)
 
     def test_in_app_quit(self):
-        self.marionette.quit(in_app=True)
+        details = self.marionette.quit(in_app=True)
+
+        self.assertFalse(details["forced"], "Expected non-forced shutdown")
+
+        self.assertEqual(self.marionette.session, None)
+        with self.assertRaisesRegexp(
+            errors.InvalidSessionIdException, "Please start a session"
+        ):
+            self.marionette.get_url()
+
+        self.marionette.start_session()
+        self.assertEqual(self.marionette.profile, self.profile)
+        self.assertNotEqual(self.marionette.session_id, self.session_id)
+        self.assertNotEqual(
+            self.marionette.get_pref("startup.homepage_welcome_url"), "about:about"
+        )
+
+    def test_in_app_quit_component_prevents_shutdown(self):
+        with self.marionette.using_context("chrome"):
+            self.marionette.execute_script(
+                """
+                Services.obs.addObserver(subject => {
+                  let cancelQuit = subject.QueryInterface(Ci.nsISupportsPRBool);
+                  cancelQuit.data = true;
+                }, "quit-application-requested");
+                """
+            )
+
+        details = self.marionette.quit(in_app=True)
+
+        self.assertTrue(details["forced"], "Expected forced shutdown")
 
         self.assertEqual(self.marionette.session, None)
         with self.assertRaisesRegexp(