From 6735746f308a0483da1a4c7724953864d03bbfd6 Mon Sep 17 00:00:00 2001 From: Henrik Skupin Date: Thu, 4 Mar 2021 20:55:24 +0000 Subject: [PATCH] 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 --- .../client/marionette_driver/marionette.py | 59 +++++++++--------- testing/marionette/driver.js | 25 ++++++-- .../tests/unit/test_quit_restart.py | 60 ++++++++++++++++++- 3 files changed, 109 insertions(+), 35 deletions(-) diff --git a/testing/marionette/client/marionette_driver/marionette.py b/testing/marionette/client/marionette_driver/marionette.py index ce3f67f19d90..83418cc495ed 100644 --- a/testing/marionette/client/marionette_driver/marionette.py +++ b/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. diff --git a/testing/marionette/driver.js b/testing/marionette/driver.js index 4e7a10dfb92b..86812e4cbdbf 100644 --- a/testing/marionette/driver.js +++ b/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} + * 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 flags 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) { diff --git a/testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py b/testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py index 378482ed502f..1b15e8e9dba2 100644 --- a/testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py +++ b/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(