Marionette’s quitApplication command accepts an optional array of arguments for Services.startup.quit. But nsIAppStartup.eAttemptQuit is always appended to the bitwise flags that is constructed from it, even though the nsIAppStartup documentation clearly says: > One and only one of the "Quit" flags must be specified. The eRestart flag may be bit-wise combined with one of the "Quit" flags to cause the application to restart after it quits. More relevant discussion in https://github.com/mozilla/geckodriver/issues/285#issuecomment-278306849. When passed a flags argument, Marionette should not add eAttemptQuit. When passed nothing, we can use eAttemptQuit as the default.

Comment on attachment 8835626 [details] Bug 1337743 - Count nsJSIID objects as objects; https://reviewboard.mozilla.org/r/111298/#review112800 ::: testing/marionette/assert.js:234 (Diff revision 2) > */ > assert.object = function (obj, msg = "") { > msg = msg || error.pprint`Expected ${obj} to be an object`; > - return assert.that(o => > - Object.prototype.toString.call(o) == "[object Object]", msg)(obj); > + return assert.that(o => { > + let s = Object.prototype.toString.call(o); > + return s == "[object Object]" || s == "[object nsJSIID]"; Hm. I don't see nsJSIID usage anywhere outside of C/C++ code. When exactly do you get this?

Comment on attachment 8835627 [details] Bug 1337743 - Command parameters may be null; https://reviewboard.mozilla.org/r/111300/#review112802 ::: testing/marionette/message.js:101 (Diff revision 2) > * Command name. > * @param {Object<string, ?>} params > * Command parameters. > */ > this.Command = class { > - constructor(msgId, name, params={}) { > + constructor(msgID, name, params = {}) { We are using `Id` everywhere else. So I would stick with this nomenclature, and if really necessary change them all at once. Also please mark params as optional in the docs. ::: testing/marionette/message.js:145 (Diff revision 2) > - return new Command(msg[1], msg[2], msg[3]); > + let [msgID, name, params] = [msg[1], msg[2], msg[3]]; > + > + // if parameters are given but null, treat them as undefined > + if (params === null) { > + params = undefined; > + } FYI we could get around this when we would do comparisions like "a == null" or "a == undefined". Both cases would also include the other value in the check.

Comment on attachment 8835628 [details] Bug 1337743 - Make session and param checks safer against falsy values; https://reviewboard.mozilla.org/r/111302/#review112804

Comment on attachment 8835629 [details] Bug 1337743 - More data validation checks for Marionette messages; https://reviewboard.mozilla.org/r/111304/#review112806 ::: testing/marionette/message.js:134 (Diff revision 2) > > - toMsg() { > + toMsg () { > return [Command.TYPE, this.id, this.name, this.parameters]; > } > > - toString() { > + toString () { Those two changes are not necessary. It's a function name we have here, so no extra space has to be added. ::: testing/marionette/message.js:213 (Diff revision 2) > * response for. > * @param {function(Response|Message)} respHandler > * Function callback called on sending the response. > */ > this.Response = class { > - constructor(msgId, respHandler) { > + constructor (msgID, respHandler) { Same here re "ID" vs "Id" ::: testing/marionette/message.js:222 (Diff revision 2) > this.body = ResponseBody(); > > this.origin = MessageOrigin.Server; > this.sent = false; > > - this.respHandler_ = respHandler; > + this.respHandler_ = assert.defined(respHandler); Better check if its callable? ::: testing/marionette/message.js:231 (Diff revision 2) > * Sends response conditionally, given a predicate. > * > * @param {function(Response): boolean} predicate > * A predicate taking a Response object and returning a boolean. > */ > - sendConditionally(predicate) { > + sendConditionally (predicate) { Please revert ::: testing/marionette/message.js:243 (Diff revision 2) > * Sends response using the response handler provided on construction. > * > * @throws {RangeError} > * If the response has already been sent. > */ > - send() { > + send () { Please revert ::: testing/marionette/message.js:265 (Diff revision 2) > * > * @throws {Error} > * If the {@code error} is not a WebDriverError, the error is > * propagated. > */ > - sendError(err) { > + sendError (err) { Please revert ::: testing/marionette/message.js:283 (Diff revision 2) > > - toMsg() { > + toMsg () { > return [Response.TYPE, this.id, this.error, this.body]; > } > > - toString() { > + toString () { Please revert both. ::: testing/marionette/message.js:289 (Diff revision 2) > return "Response {id: " + this.id + ", " + > "error: " + JSON.stringify(this.error) + ", " + > "body: " + JSON.stringify(this.body) + "}"; > } > > - static fromMsg(msg) { > + static fromMsg (msg) { Please revert ::: testing/marionette/message.js:294 (Diff revision 2) > - static fromMsg(msg) { > - let resp = new Response(msg[1], null); > - resp.error = msg[2]; > - resp.body = msg[3]; > + static fromMsg (msg) { > + let [type, msgID, err, body] = msg; > + assert.that(n => n === Response.TYPE)(type); > + > + let resp = new Response(msgID, null); > + resp.error = assert.string(err); Can `err` never be null?

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review112812 ::: testing/marionette/client/marionette_driver/marionette.py:1164 (Diff revision 2) > + "Something cancelled the quit application request") > > - self._send_message("quitApplication", {"flags": list(flags)}) > + body = None > + if len(flags) > 0: > + body = {"flags": list(flags)} > + self._send_message("quitApplication", body) As mentioed above, I would love to see this whole method gone and integrated into the server code. The cancelQuit observer notification should really be send in the server, so that every consumer and not only Marionette client can benefit from a safer shutdown. ::: testing/marionette/driver.js:2530 (Diff revision 2) > - * session. > + * > + * Marionette will stop accepting new connections before ending the > + * current session, and finally attempting to quit the application. > + * > + * Optional {@code nsIAppStartup} flags may be provided as > + * an array of masks, and these will be combined by ANDing s/ANDing/ORing ::: testing/marionette/driver.js:2554 (Diff revision 2) > > - let flags = Ci.nsIAppStartup.eAttemptQuit; > - for (let k of cmd.parameters.flags || []) { > - flags |= Ci.nsIAppStartup[k]; > + let flags; > + if (typeof cmd.parameters.flags != "undefined") { > + flags = assert.array(cmd.parameters.flags); > + } else { > + flags = []; You can initialize flags with an empty array directly when defining it, so we can safe this extra else case. ::: testing/marionette/driver.js:2585 (Diff revision 2) > + try { > + this._server.acceptConnections = false; > - this.deleteSession(); > + this.deleteSession(); > - Services.startup.quit(flags); > + resp.send(); > + } finally { > + Services.startup.quit(mode); Before we actually do a quit or restart we should request an application shutdown. Please have a look into marionette.py:_request_in_app_shutdown(). I feel we should finally integrate this code here, and leave it for compatibility in the Python code until 55.0. ::: testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py:10 (Diff revision 2) > -from marionette_driver.errors import MarionetteException > +from marionette_driver import errors > > from marionette_harness import MarionetteTestCase > > > +class TestQuitApplication(MarionetteTestCase): The tests in that class seem to be better placed as a XPCShell test. Why did you those for the client? ::: testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py:67 (Diff revision 2) > MarionetteTestCase.setUp(self) > > self.pid = self.marionette.process_id > self.session_id = self.marionette.session_id > > - self.assertNotEqual(self.marionette.get_pref("browser.startup.page"), 3) > + self.assertNotEqual( I don't see why those changes are necessary. Please revert this and the other lines.

Comment on attachment 8835630 [details] Bug 1337743 - Handle corrupt packets to Marionette; https://reviewboard.mozilla.org/r/111306/#review112818 ::: testing/marionette/dispatcher.js:28 (Diff revision 2) > > /** > * Manages a Marionette connection, and dispatches packets received to > * their correct destinations. > * > - * @param {number} connId > + * @param {number} connID Same re `ID` vs. `Id`. ::: testing/marionette/dispatcher.js:171 (Diff revision 2) > + * Message ID to respond to. If it is not a number, -1 is used. > + * > + * @return {message.Response} > + * Response to the message with |msgID|. > + */ > +Dispatcher.prototype.newResponse = function (msgID) { Shouldn't we better call this `createResponse`?

Comment on attachment 8835630 [details] Bug 1337743 - Handle corrupt packets to Marionette; https://reviewboard.mozilla.org/r/111306/#review112820

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review112822

Comment on attachment 8835629 [details] Bug 1337743 - More data validation checks for Marionette messages; https://reviewboard.mozilla.org/r/111304/#review112824

Comment on attachment 8835627 [details] Bug 1337743 - Command parameters may be null; https://reviewboard.mozilla.org/r/111300/#review112826

Comment on attachment 8835626 [details] Bug 1337743 - Count nsJSIID objects as objects; https://reviewboard.mozilla.org/r/111298/#review112800 > Hm. I don't see nsJSIID usage anywhere outside of C/C++ code. When exactly do you get this? Well it’s used in the XPC bridge for any C++ object that is wrapped in JS. Any IDL can potentially surface in chrome-scope JavaScript, and this does so if with `assert.in(k, Ci.nsIAppStartup)` since `Ci.nsIAppStartup` is not an `object`, but `nsJSIID`.

Comment on attachment 8835627 [details] Bug 1337743 - Command parameters may be null; https://reviewboard.mozilla.org/r/111300/#review112802 > We are using `Id` everywhere else. So I would stick with this nomenclature, and if really necessary change them all at once. > > Also please mark params as optional in the docs. We should use `ID` for consistency with other JS code in Gecko (e.g. `outerWindowID`) and DOM. I’ll drop the remaining issues related to this. > FYI we could get around this when we would do comparisions like "a == null" or "a == undefined". Both cases would also include the other value in the check. (Please highlight all the relevant lines when you file issues in mozreview, otherwise the quoted diff will be hard to read.) I think implied type checks ought to be avoided. Once we enable eslint for testing/marionette, that kind of tests will also be disallowed.

Comment on attachment 8835629 [details] Bug 1337743 - More data validation checks for Marionette messages; https://reviewboard.mozilla.org/r/111304/#review112806 > Those two changes are not necessary. It's a function name we have here, so no extra space has to be added. `toString()` does look like it’s calling a function `toString`, though. Dropping all similar issues. > Better check if its callable? That is a fair point. I will introduce `assert.callable` which will do a test on `typeof obj == "function"`. > Can `err` never be null? I guess it can if you send `[null, …, …, …]`.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review112812 > As mentioed above, I would love to see this whole method gone and integrated into the server code. > > The cancelQuit observer notification should really be send in the server, so that every consumer and not only Marionette client can benefit from a safer shutdown. OK, I can probably fix that. I will do so in a separate commit. > Before we actually do a quit or restart we should request an application shutdown. Please have a look into marionette.py:_request_in_app_shutdown(). I feel we should finally integrate this code here, and leave it for compatibility in the Python code until 55.0. I can have a look at doing that in a follow-up commit. This shoudln’t regress any existing behaviour in the server, so I will resolve this issue. > The tests in that class seem to be better placed as a XPCShell test. Why did you those for the client? They test that quitting Firefox works. I can’t see how I can do that, or access `quitApplication` as an xpcshell test. > I don't see why those changes are necessary. Please revert this and the other lines. I probably ran the whole file through autopep8 and reverting them is more work than I’m willing to put in, so dropping this issue.

Comment on attachment 8836203 [details] Bug 1337743 - Add assert.callable for checking callbacks; https://reviewboard.mozilla.org/r/111652/#review113038

Comment on attachment 8835626 [details] Bug 1337743 - Count nsJSIID objects as objects; https://reviewboard.mozilla.org/r/111298/#review112800 > Well it’s used in the XPC bridge for any C++ object that is wrapped in JS. Any IDL can potentially surface in chrome-scope JavaScript, and this does so if with `assert.in(k, Ci.nsIAppStartup)` since `Ci.nsIAppStartup` is not an `object`, but `nsJSIID`. I just did a `alert(typeof Ci.nsIAppStartup)` in the scratchpad and it returns `Object` to me. So what's the benefit of using `Object.prototype.toString.call()` compared to just `typeof`? Also using `o instanceof Object` in `assert.object()` might be better, nor?

Comment on attachment 8835627 [details] Bug 1337743 - Command parameters may be null; https://reviewboard.mozilla.org/r/111300/#review113240

Comment on attachment 8835629 [details] Bug 1337743 - More data validation checks for Marionette messages; https://reviewboard.mozilla.org/r/111304/#review112806 > `toString()` does look like it’s calling a function `toString`, though. Dropping all similar issues. No, please fix that. It's a function definition and there is no distinction between that and the invocation. Only `function` itself makes a difference. > I guess it can if you send `[null, …, …, …]`. Which should then fail?

Comment on attachment 8835629 [details] Bug 1337743 - More data validation checks for Marionette messages; https://reviewboard.mozilla.org/r/111304/#review113244

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review112812 > They test that quitting Firefox works. I can’t see how I can do that, or access `quitApplication` as an xpcshell test. We have a good coverage of restart and quit tests already by the other test class in the same file. So if you really need a restart we could leave them here, but for just the invalid parameter checks (which do not cause a shutdown), there shouldn't be a need to have them as Python tests. > I probably ran the whole file through autopep8 and reverting them is more work than I’m willing to put in, so dropping this issue. Just take a copy from central, and add your other test class (if necessary as listed above). That would automatically revert all that.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113248

Comment on attachment 8836204 [details] Bug 1337743 - Return error if quit request is cancelled; https://reviewboard.mozilla.org/r/111654/#review113250 ::: testing/marionette/client/marionette_driver/marionette.py:1145 (Diff revision 1) > > """ > + > + # The vast majority of this function was implemented inside > + # the quitApplication command as part of bug 1337743, and can be > + # removed from here in Firefox 54 at the latest. No, its Firefox 55 as the earliest if you have a chance to uplift all the changes to 52 before its release. ::: testing/marionette/driver.js:2553 (Diff revision 1) > const quits = ["eConsiderQuit", "eAttemptQuit", "eForceQuit"]; > > + // return error if the quit request is cancelled > + Services.obs.addObserver(() => { > + throw new UnknownError("Quit was aborted"); > + }, "quit-application-requested", false); You do not want to listen for this observer notification when you want to know if a request has been aborted. Also you even do not check the `aSubject.data` parameter here.

Comment on attachment 8836204 [details] Bug 1337743 - Return error if quit request is cancelled; https://reviewboard.mozilla.org/r/111654/#review113250 > You do not want to listen for this observer notification when you want to know if a request has been aborted. Also you even do not check the `aSubject.data` parameter here. So you really want to use the code as which is in use in marionette.py: `Services.obs.notifyObservers(cancelQuit, ...)`.

Comment on attachment 8836205 [details] Bug 1337743 - Send response request on quit-application event; https://reviewboard.mozilla.org/r/111656/#review113276 ::: testing/marionette/driver.js:2586 (Diff revision 1) > } > > try { > this._server.acceptConnections = false; > this.deleteSession(); > - resp.send(); > + Services.obs.addObserver(resp.send, "quit-application", false); Is it guaranteed that this response goes through? I feel that we still run into a race condition here.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113294 ::: testing/marionette/client/marionette_driver/marionette.py:1144 (Diff revision 3) > - of the application. Possible values here correspond > - to constants in nsIAppStartup: http://mzl.la/1X0JZsC. > """ > - flags = set([]) > + flags = set(("eAttemptQuit",)) > if shutdown_flags: > - flags.add(shutdown_flags) > + flags = set(list(flags) + list(shutdown_flags)) Surely this should remove duplicate `*Quit` flags? Also, you probably want `set(shutdown_flags) | flags`. ::: testing/marionette/client/marionette_driver/marionette.py:1230 (Diff revision 3) > > if callable(callback): > self._send_message("acceptConnections", {"value": False}) > callback() > else: > - self._request_in_app_shutdown("eRestart") > + self._request_in_app_shutdown(("eRestart",)) I think this suggests that allowing a plain string as the argument would be a more robust API. Otherwise people will accidentially send `["e", "R", "e", "s", "t", "a", "r", "t"]` over the wire, only for it to fail. ::: testing/marionette/driver.js:2547 (Diff revision 3) > + * @throws {InvalidArgumentError} > + * If |flags| contains unknown or incompatible flags, for example > + * multiple Quit flags. > */ > GeckoDriver.prototype.quitApplication = function (cmd, resp) { > - assert.firefox("Bug 1298921 - In app initiated quit not yet available beside Firefox") > + const {eAttemptQuit, eForceQuit} = Ci.nsIAppStartup; If this works the way I think it does then you don't use `eForceQuit` anywhere.

Comment on attachment 8835626 [details] Bug 1337743 - Count nsJSIID objects as objects; https://reviewboard.mozilla.org/r/111298/#review112800 > I just did a `alert(typeof Ci.nsIAppStartup)` in the scratchpad and it returns `Object` to me. So what's the benefit of using `Object.prototype.toString.call()` compared to just `typeof`? Also using `o instanceof Object` in `assert.object()` might be better, nor? `typeof` reports "object" for a great many different types. `instanceof` does not work across process/context boundaries, so `let a = {}; a instanceof Object` might work in content, but not chrome for inexplicable reasons. This is the same problem we are having with determining if an object is an error.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review112812 > We have a good coverage of restart and quit tests already by the other test class in the same file. So if you really need a restart we could leave them here, but for just the invalid parameter checks (which do not cause a shutdown), there shouldn't be a need to have them as Python tests. I think real-world test coverage for this command is needed, so I will leave them as they are.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113294 > Surely this should remove duplicate `*Quit` flags? > > Also, you probably want `set(shutdown_flags) | flags`. Thanks. > I think this suggests that allowing a plain string as the argument would be a more robust API. Otherwise people will accidentially send `["e", "R", "e", "s", "t", "a", "r", "t"]` over the wire, only for it to fail. Since this method is going away I guess it’s debatable whether changing it is necessary, but I can make it accept a vararg so that multiple flags can be sent while still retaining API compatibility for this internal function. > If this works the way I think it does then you don't use `eForceQuit` anywhere. Removed.

Comment on attachment 8835629 [details] Bug 1337743 - More data validation checks for Marionette messages; https://reviewboard.mozilla.org/r/111304/#review112806 > No, please fix that. It's a function definition and there is no distinction between that and the invocation. Only `function` itself makes a difference. I think you will find this will cause problems when we enable eslint, unless you use one of special configurations shown in http://eslint.org/docs/rules/space-before-function-paren to enforce spaces before () in functions but not in class methods.

Comment on attachment 8836204 [details] Bug 1337743 - Return error if quit request is cancelled; https://reviewboard.mozilla.org/r/111654/#review113250 > So you really want to use the code as which is in use in marionette.py: `Services.obs.notifyObservers(cancelQuit, ...)`. As far as I can tell from https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIObserverService, `notifyObserver` lets other registered observers know that an event is occurring. Here, we want to let the consumer of `quitApplication` know that one of the attached observers cancelled the quit, not to inform them that a quit was requested. I agree I’m failing to check the subject here.

Comment on attachment 8836205 [details] Bug 1337743 - Send response request on quit-application event; https://reviewboard.mozilla.org/r/111656/#review113276 > Is it guaranteed that this response goes through? I feel that we still run into a race condition here. I’ve slightly modified this in the original patch. This does work as intended, but it doesn’t block the `quitApplication` method from returning, so the latest change I made wraps this in a promise.

Comment on attachment 8835626 [details] Bug 1337743 - Count nsJSIID objects as objects; https://reviewboard.mozilla.org/r/111298/#review112800 > `typeof` reports "object" for a great many different types. `instanceof` does not work across process/context boundaries, so `let a = {}; a instanceof Object` might work in content, but not chrome for inexplicable reasons. This is the same problem we are having with determining if an object is an error. I don't see any problem with instanceof at all in chrome scope. Is that just a thing you remember from ages ago, or did you face that lately? I would suggest that you actually check that. And if it really turns out to be a problem, give me some code which I can use to reproduce. So far I have no way.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review112812 > I think real-world test coverage for this command is needed, so I will leave them as they are. Re-checking the tests in detail I have to say that I'm not with you on that. Especially with the custom `quit()` method we are no longer testing the real code of Marionette.quit(), which makes the tests as run from the client useless. Also the others for invalid arguments do NOT restart the browser, so they should really be xpcshell tests. I'm really in favor of getting a clear separation of tests. For those tests which need a restart, why can't you put those in the already existent test class? Also lets try to not play ping pong on opening/closing issues. Lets get them discussed and we can finally close them once all are happy.

Comment on attachment 8835629 [details] Bug 1337743 - More data validation checks for Marionette messages; https://reviewboard.mozilla.org/r/111304/#review112806 > I think you will find this will cause problems when we enable eslint, unless you use one of special configurations shown in http://eslint.org/docs/rules/space-before-function-paren to enforce spaces before () in functions but not in class methods. We don't know yet what our defaults will be given that we don't really obey that across our current code base. But well, being closer to the defined standard is always good. Thanks for the link.

Comment on attachment 8835629 [details] Bug 1337743 - More data validation checks for Marionette messages; https://reviewboard.mozilla.org/r/111304/#review113644 ::: testing/marionette/message.js:122 (Diff revisions 3 - 5) > * > * @param {Response} resp > * The response to pass on to the result or error to the > * {@code onerror} or {@code onresult} handlers to. > */ > - onresponse (resp) { > + onresponse(resp) { Hm, I thought you were against using no space here? Why did you revert it? ::: testing/marionette/message.js:222 (Diff revision 5) > this.body = ResponseBody(); > > this.origin = MessageOrigin.Server; > this.sent = false; > > - this.respHandler_ = respHandler; > + this.respHandler_ = assert.callable(respHandler); Please move it up right next to the other assert check.

Comment on attachment 8836204 [details] Bug 1337743 - Return error if quit request is cancelled; https://reviewboard.mozilla.org/r/111654/#review113250 > As far as I can tell from https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIObserverService, `notifyObserver` lets other registered observers know that an event is occurring. Here, we want to let the consumer of `quitApplication` know that one of the attached observers cancelled the quit, not to inform them that a quit was requested. > > I agree I’m failing to check the subject here. You only have to register for quit-application-requested if there is a reason to cancel the quit request. In case of our quit() method, I don't see why we want to do that. With 'notifyObservers()' you will request a quit, and this call is blocking until every consumer of quit-application-requested has been responded. As result you have to observe 'cancelQuit.data' to figure out if the request has been canceled. So this is all aligned with the shutdown code sequence.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113650 ::: testing/marionette/client/marionette_driver/marionette.py:1154 (Diff revisions 3 - 5) > # 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); Please really copy this block to the JS code in driver.js. If you don't do it now, we cannot get it removed in Firefox 55. ::: testing/marionette/driver.js:2585 (Diff revisions 3 - 5) > + // delay response until the application is about to quit > + let quit = new Promise(resolve => { > + Services.obs.addObserver(() => { > - resp.send(); > + resp.send(); > - } finally { > + resolve(); > + }, "quit-application", false); Once we have the check for cancelQuit.data from notifyObservers it should be unlikely that the quit does not happen, but we should account for and remove the added notification. ::: testing/marionette/driver.js:2589 (Diff revisions 3 - 5) > - } finally { > + resolve(); > + }, "quit-application", false); > + }); > + > - Services.startup.quit(mode); > + Services.startup.quit(mode); > - } > + yield quit; You are yielding in a non-generator method. How does that work?

Comment on attachment 8835626 [details] Bug 1337743 - Count nsJSIID objects as objects; https://reviewboard.mozilla.org/r/111298/#review112800 > I don't see any problem with instanceof at all in chrome scope. Is that just a thing you remember from ages ago, or did you face that lately? I would suggest that you actually check that. And if it really turns out to be a problem, give me some code which I can use to reproduce. So far I have no way. Any object that is passed to us from the devtools transport actor fail to pass the `o instanceof Object` test, presumably because it originates elsewhere and because `instanceof` isn’t safe across that context shift. I don’t know _why_ that is the case, but it is reproducible only in chrome with objects we get from the transport layer: ``` 1487075787563 Marionette DEBUG Accepted connection conn2 from 127.0.0.1:54772 data => 0,1,newSession,[object Object] JSON.stringify(data) => [0,1,"newSession",{"sessionId":null,"capabilities":null}] o => [object Object] typeof o => object o.toString() => [object Object] o.toSource() => ({sessionId:null, capabilities:null}) Object.prototype.toString.call(o) => [object Object] o instanceof Object => false JSON.stringify(o) => {"sessionId":null,"capabilities":null} 1487075787567 Marionette TRACE conn2 <- [1,1,{"error":"invalid argument","message":"","stacktrace":"this.WebDriverError@chrome://marionette/content/error.js:214:17\nthis.InvalidArgumentError@chrome://marionette/content/error.js:250:3\nassert.that/<@chrome://marionette/content/assert.js:335:13\nassert.object@chrome://marionette/content/assert.js:251:10\nthis.Command<@chrome://marionette/content/message.js:105:23\nfromMsg@chrome://marionette/content/message.js:149:12\nMessage.fromMsg@chrome://marionette/content/message.js:48:14\nDispatcher.prototype.onPacket@chrome://marionette/content/dispatcher.js:92:11\n_onJSONObjectReady/<@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:483:11\nexports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14\nexports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14\n"},null] Error running mach: ['marionette-test', 'testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py', '--gecko-log', '-', '-vv'] The error occurred in code that was called by the mach command. This is either a bug in the called code itself or in the way that mach is calling it. You should consider filing a bug for this issue. If filing a bug, please include the full output of mach, including this error message. The details of the failure are as follows: InvalidArgumentException: stacktrace: this.WebDriverError@chrome://marionette/content/error.js:214:17 this.InvalidArgumentError@chrome://marionette/content/error.js:250:3 assert.that/<@chrome://marionette/content/assert.js:335:13 assert.object@chrome://marionette/content/assert.js:251:10 this.Command<@chrome://marionette/content/message.js:105:23 fromMsg@chrome://marionette/content/message.js:149:12 Message.fromMsg@chrome://marionette/content/message.js:48:14 Dispatcher.prototype.onPacket@chrome://marionette/content/dispatcher.js:92:11 _onJSONObjectReady/<@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:483:11 exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14 exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14 File "/home/ato/src/gecko/testing/marionette/mach_commands.py", line 81, in run_marionette_test return run_marionette(tests, topsrcdir=self.topsrcdir, **kwargs) File "/home/ato/src/gecko/testing/marionette/mach_commands.py", line 59, in run_marionette failed = MarionetteHarness(MarionetteTestRunner, args=vars(args)).run() File "/home/ato/src/gecko/testing/marionette/harness/marionette_harness/runtests.py", line 72, in run runner.run_tests(tests) File "/home/ato/src/gecko/testing/marionette/harness/marionette_harness/runner/base.py", line 823, in run_tests self.fixture_servers = self.start_fixture_servers() File "/home/ato/src/gecko/testing/marionette/harness/marionette_harness/runner/base.py", line 916, in start_fixture_servers if self.appName == "fennec": File "/home/ato/src/gecko/testing/marionette/harness/marionette_harness/runner/base.py", line 676, in appName self._appName = self.capabilities.get('browserName') File "/home/ato/src/gecko/testing/marionette/harness/marionette_harness/runner/base.py", line 649, in capabilities self.marionette.start_session() File "/home/ato/src/gecko/testing/marionette/client/marionette_driver/decorators.py", line 23, in _ return func(*args, **kwargs) File "/home/ato/src/gecko/testing/marionette/client/marionette_driver/marionette.py", line 1301, in start_session resp = self._send_message("newSession", body) File "/home/ato/src/gecko/testing/marionette/client/marionette_driver/decorators.py", line 23, in _ return func(*args, **kwargs) File "/home/ato/src/gecko/testing/marionette/client/marionette_driver/marionette.py", line 729, in _send_message self._handle_error(err) File "/home/ato/src/gecko/testing/marionette/client/marionette_driver/marionette.py", line 762, in _handle_error raise errors.lookup(error)(message, stacktrace=stacktrace) ``` Even `eval(o.toSource())` and `JSON.parse(JSON.stringify(o))` fail the `instanceof Object` test, which leaves me at a loss. As I mentioned earlier, this is the same issue we were having earlier with `instanceof Error` and `instanceof WebDriverError` which leads me to implement http://searchfox.org/mozilla-central/source/testing/marionette/error.js#48-73. Back then I suspect it was because prototype chains could not be compared across `<xul:browser>` instances, but this is a far-fetched theory. There is _some_ subtlety about `instanceof` in chrome scope that I’m unaware of, and this works around this problem.

Comment on attachment 8835629 [details] Bug 1337743 - More data validation checks for Marionette messages; https://reviewboard.mozilla.org/r/111304/#review113644 > Hm, I thought you were against using no space here? Why did you revert it? Because you said so… > Please move it up right next to the other assert check. What purpose does that serve?

Comment on attachment 8835629 [details] Bug 1337743 - More data validation checks for Marionette messages; https://reviewboard.mozilla.org/r/111304/#review112806 > We don't know yet what our defaults will be given that we don't really obey that across our current code base. But well, being closer to the defined standard is always good. Thanks for the link. Picking the most common denominator for eslint configuration makes sense. There is a fairly loose outline of a global config with component-specific overrides, such as for /devtools. > Which should then fail? Sorry, earlier I meant `[…, …, null, …]`. The three first elements of the command array are required, and the two first and the either the third or the fourth of the response array. They are, in order: 0. Message type (command = 0, response = 0) 1. Message ID (incrementing counter) 2. Command name/response error code 3. Command parameters/response body

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113650 > Please really copy this block to the JS code in driver.js. If you don't do it now, we cannot get it removed in Firefox 55. I have ported this code over to the server in the next patch (https://reviewboard.mozilla.org/r/111654/diff/#index_header). Since the client will still be used with earlier Firefoxen, I suppose we can’t remove it from here quite yet? > Once we have the check for cancelQuit.data from notifyObservers it should be unlikely that the quit does not happen, but we should account for and remove the added notification. I don’t understand what you’re saying. > You are yielding in a non-generator method. How does that work? I think it works by chance right now because Gecko chrome JS runs under legacy JS1.7 compatibility mode. :arai is working on replacing the legacy generators with ES6 generators in https://bugzilla.mozilla.org/show_bug.cgi?id=968038. I’ve rectified this and added a `*` to make this an ES6 generator function.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review112812 > Re-checking the tests in detail I have to say that I'm not with you on that. Especially with the custom `quit()` method we are no longer testing the real code of Marionette.quit(), which makes the tests as run from the client useless. Also the others for invalid arguments do NOT restart the browser, so they should really be xpcshell tests. I'm really in favor of getting a clear separation of tests. > > For those tests which need a restart, why can't you put those in the already existent test class? > > Also lets try to not play ping pong on opening/closing issues. Lets get them discussed and we can finally close them once all are happy. > Especially with the custom quit() method we are no longer testing the real code of Marionette.quit() I did not remove any tests, so test coverage should not have declined. The tests I have added are not testing the Python client’s version of `Marionette.quit` because it does not expose the barbones of the `quitApplication` command. For example, I wish to call `self.marionette.instance.runner.wait()` explicitly and ensure session state in the client torn down. I couldn’t find a way to do that with the private `Marionette._request_in_app_quit` method. > Also the others for invalid arguments do NOT restart the browser, so they should really be xpcshell tests. We don’t have a good way of testing methods in testing/marionette/driver.js and the `GeckoDriver` object. There is precedence for testing invalid input through functional tests in Marionette, and although I agree an xpcshell test would be ideal, what I have presented here works right now and is immediately useful. In the longer term we should seek to replace many of the Mn tests with WPT WebDriver tests, at which point they must be run externally anyway to test WebDriver protocol conformance. I don’t see how this approach is wrong in this context. > For those tests which need a restart, why can't you put those in the already existent test class? Because I wanted a clear separation between those testing the Python client’s restart API and those calling `quitApplication` directly, and it was easier to write a new class for the teardown methods I needed than to try to modify the existing `TestQuitRestart` class. > Also lets try to not play ping pong on opening/closing issues. Lets get them discussed and we can finally close them once all are happy. Fine. When I have left issues open in the past you have occasionally failed to return to close them. I will let guard the closure of all issues.

Comment on attachment 8836204 [details] Bug 1337743 - Return error if quit request is cancelled; https://reviewboard.mozilla.org/r/111654/#review113250 > You only have to register for quit-application-requested if there is a reason to cancel the quit request. In case of our quit() method, I don't see why we want to do that. With 'notifyObservers()' you will request a quit, and this call is blocking until every consumer of quit-application-requested has been responded. As result you have to observe 'cancelQuit.data' to figure out if the request has been canceled. So this is all aligned with the shutdown code sequence. > You only have to register for quit-application-requested if there is a reason to cancel the quit request. In case of our quit() method, I don't see why we want to do that. Correct me if I’m wrong, but I thought the intention here was to send back an error if the request to quit the application _is_ cancelled by some other component? Potentially there are other observers elsewhere in the application that for their own reasons may request the quit to be cancelled, and Marionette should handle that. > With 'notifyObservers()' you will request a quit, and this call is blocking until every consumer of quit-application-requested has been responded. It doesn’t say anywhere in the observer documentation that `notifyObservers` actually _requests_ a quit? It says that it will _notify_ the _observers_ of given event. You have to explicitly call `Services.startup.quit` to actually initiate the quit procedures as far as I’m aware? Also, where is it documented that `notifyObservers` blocks until observers listening for `quit-application-requested` has processed? Wouldn’t this be to the contrary of how the asynchonicity of JavaScript works?

Comment on attachment 8835626 [details] Bug 1337743 - Count nsJSIID objects as objects; https://reviewboard.mozilla.org/r/111298/#review112800 > Any object that is passed to us from the devtools transport actor fail to pass the `o instanceof Object` test, presumably because it originates elsewhere and because `instanceof` isn’t safe across that context shift. I don’t know _why_ that is the case, but it is reproducible only in chrome with objects we get from the transport layer: > > ``` > 1487075787563 Marionette DEBUG Accepted connection conn2 from 127.0.0.1:54772 > data => 0,1,newSession,[object Object] > JSON.stringify(data) => [0,1,"newSession",{"sessionId":null,"capabilities":null}] > o => [object Object] > typeof o => object > o.toString() => [object Object] > o.toSource() => ({sessionId:null, capabilities:null}) > Object.prototype.toString.call(o) => [object Object] > o instanceof Object => false > JSON.stringify(o) => {"sessionId":null,"capabilities":null} > 1487075787567 Marionette TRACE conn2 <- [1,1,{"error":"invalid argument","message":"","stacktrace":"this.WebDriverError@chrome://marionette/content/error.js:214:17\nthis.InvalidArgumentError@chrome://marionette/content/error.js:250:3\nassert.that/<@chrome://marionette/content/assert.js:335:13\nassert.object@chrome://marionette/content/assert.js:251:10\nthis.Command<@chrome://marionette/content/message.js:105:23\nfromMsg@chrome://marionette/content/message.js:149:12\nMessage.fromMsg@chrome://marionette/content/message.js:48:14\nDispatcher.prototype.onPacket@chrome://marionette/content/dispatcher.js:92:11\n_onJSONObjectReady/<@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:483:11\nexports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14\nexports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14\n"},null] > Error running mach: > > ['marionette-test', 'testing/marionette/harness/marionette_harness/tests/unit/test_quit_restart.py', '--gecko-log', '-', '-vv'] > > The error occurred in code that was called by the mach command. This is either > a bug in the called code itself or in the way that mach is calling it. > > You should consider filing a bug for this issue. > > If filing a bug, please include the full output of mach, including this error > message. > > The details of the failure are as follows: > > InvalidArgumentException: > stacktrace: > this.WebDriverError@chrome://marionette/content/error.js:214:17 > this.InvalidArgumentError@chrome://marionette/content/error.js:250:3 > assert.that/<@chrome://marionette/content/assert.js:335:13 > assert.object@chrome://marionette/content/assert.js:251:10 > this.Command<@chrome://marionette/content/message.js:105:23 > fromMsg@chrome://marionette/content/message.js:149:12 > Message.fromMsg@chrome://marionette/content/message.js:48:14 > Dispatcher.prototype.onPacket@chrome://marionette/content/dispatcher.js:92:11 > _onJSONObjectReady/<@chrome://marionette/content/server.js -> resource://devtools/shared/transport/transport.js:483:11 > exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14 > exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14 > > > File "/home/ato/src/gecko/testing/marionette/mach_commands.py", line 81, in run_marionette_test > return run_marionette(tests, topsrcdir=self.topsrcdir, **kwargs) > File "/home/ato/src/gecko/testing/marionette/mach_commands.py", line 59, in run_marionette > failed = MarionetteHarness(MarionetteTestRunner, args=vars(args)).run() > File "/home/ato/src/gecko/testing/marionette/harness/marionette_harness/runtests.py", line 72, in run > runner.run_tests(tests) > File "/home/ato/src/gecko/testing/marionette/harness/marionette_harness/runner/base.py", line 823, in run_tests > self.fixture_servers = self.start_fixture_servers() > File "/home/ato/src/gecko/testing/marionette/harness/marionette_harness/runner/base.py", line 916, in start_fixture_servers > if self.appName == "fennec": > File "/home/ato/src/gecko/testing/marionette/harness/marionette_harness/runner/base.py", line 676, in appName > self._appName = self.capabilities.get('browserName') > File "/home/ato/src/gecko/testing/marionette/harness/marionette_harness/runner/base.py", line 649, in capabilities > self.marionette.start_session() > File "/home/ato/src/gecko/testing/marionette/client/marionette_driver/decorators.py", line 23, in _ > return func(*args, **kwargs) > File "/home/ato/src/gecko/testing/marionette/client/marionette_driver/marionette.py", line 1301, in start_session > resp = self._send_message("newSession", body) > File "/home/ato/src/gecko/testing/marionette/client/marionette_driver/decorators.py", line 23, in _ > return func(*args, **kwargs) > File "/home/ato/src/gecko/testing/marionette/client/marionette_driver/marionette.py", line 729, in _send_message > self._handle_error(err) > File "/home/ato/src/gecko/testing/marionette/client/marionette_driver/marionette.py", line 762, in _handle_error > raise errors.lookup(error)(message, stacktrace=stacktrace) > ``` > > Even `eval(o.toSource())` and `JSON.parse(JSON.stringify(o))` fail the `instanceof Object` test, which leaves me at a loss. > > As I mentioned earlier, this is the same issue we were having earlier with `instanceof Error` and `instanceof WebDriverError` which leads me to implement http://searchfox.org/mozilla-central/source/testing/marionette/error.js#48-73. Back then I suspect it was because prototype chains could not be compared across `<xul:browser>` instances, but this is a far-fetched theory. There is _some_ subtlety about `instanceof` in chrome scope that I’m unaware of, and this works around this problem. `instanceof` works by comparing the prototype of the left hand side with the `prototype` property from the right hand side. If the lhs and rhs come from different globals this check will always fail because the two objects will not have the same identity. Therefore it isn't safe to use in any multi-global situation (e.g. in content across multiple `Window` objects).

Comment on attachment 8835626 [details] Bug 1337743 - Count nsJSIID objects as objects; https://reviewboard.mozilla.org/r/111298/#review112800 > `instanceof` works by comparing the prototype of the left hand side with the `prototype` property from the right hand side. If the lhs and rhs come from different globals this check will always fail because the two objects will not have the same identity. Therefore it isn't safe to use in any multi-global situation (e.g. in content across multiple `Window` objects). Thanks, James! That makes sense. I have appended another commit to this changeset which clears up the documentation in the cases we use this for posterity’s sake.

Comment on attachment 8835626 [details] Bug 1337743 - Count nsJSIID objects as objects; https://reviewboard.mozilla.org/r/111298/#review114288

Comment on attachment 8837199 [details] Bug 1337743 - Document misuse of instanceof in Marionette; https://reviewboard.mozilla.org/r/112392/#review114292

Comment on attachment 8835629 [details] Bug 1337743 - More data validation checks for Marionette messages; https://reviewboard.mozilla.org/r/111304/#review113644 > Because you said so… In the other commit's issue you declined it. That's why I'm wondering now. > What purpose does that serve? We wouldn't have to spend time on constructing a ResponseBody first, and do all the assignments if the response handler is not callable.

Comment on attachment 8835629 [details] Bug 1337743 - More data validation checks for Marionette messages; https://reviewboard.mozilla.org/r/111304/#review112806 > Sorry, earlier I meant `[…, …, null, …]`. The three first elements of the command array are required, and the two first and the either the third or the fourth of the response array. They are, in order: > > 0. Message type (command = 0, response = 0) > 1. Message ID (incrementing counter) > 2. Command name/response error code > 3. Command parameters/response body But then it would fail the assertion given that you require the error to be a string.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113650 > I have ported this code over to the server in the next patch (https://reviewboard.mozilla.org/r/111654/diff/#index_header). Since the client will still be used with earlier Firefoxen, I suppose we can’t remove it from here quite yet? Hm, too many commits for similar code changes is confusing. We should try to make those changes in the same commit. > I don’t understand what you’re saying. In the case no quit will happen, we leak the added observer. Whereby checking again we would basically hang forever.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113650 > Hm, too many commits for similar code changes is confusing. We should try to make those changes in the same commit. Oh, and yes we cannot remove it yet, but we could add a version check in quit() and restart() to not have to call _request_in_app_shutdown() at all. The flags should be handled in both the public methods now anyway. So keeping the private method for legacy only.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review112812 > > Especially with the custom quit() method we are no longer testing the real code of Marionette.quit() > > I did not remove any tests, so test coverage should not have declined. > > The tests I have added are not testing the Python client’s version of `Marionette.quit` because it does not expose the barbones of the `quitApplication` command. For example, I wish to call `self.marionette.instance.runner.wait()` explicitly and ensure session state in the client torn down. I couldn’t find a way to do that with the private `Marionette._request_in_app_quit` method. > > > Also the others for invalid arguments do NOT restart the browser, so they should really be xpcshell tests. > > We don’t have a good way of testing methods in testing/marionette/driver.js and the `GeckoDriver` object. There is precedence for testing invalid input through functional tests in Marionette, and although I agree an xpcshell test would be ideal, what I have presented here works right now and is immediately useful. > > In the longer term we should seek to replace many of the Mn tests with WPT WebDriver tests, at which point they must be run externally anyway to test WebDriver protocol conformance. I don’t see how this approach is wrong in this context. > > > For those tests which need a restart, why can't you put those in the already existent test class? > > Because I wanted a clear separation between those testing the Python client’s restart API and those calling `quitApplication` directly, and it was easier to write a new class for the teardown methods I needed than to try to modify the existing `TestQuitRestart` class. > > > Also lets try to not play ping pong on opening/closing issues. Lets get them discussed and we can finally close them once all are happy. > > Fine. When I have left issues open in the past you have occasionally failed to return to close them. I will let guard the closure of all issues. > For example, I wish to call self.marionette.instance.runner.wait() explicitly and ensure session state in the client torn down. I couldn’t find a way to do that with the private Marionette._request_in_app_quit method. As mentioned in the other commit we should get rid of calling this method for newer versions of Firefox which contain your changes. Maybe that would help here? > We don’t have a good way of testing methods in testing/marionette/driver.js and the GeckoDriver object. That's something I didn't know. In that case it's indeed the better way for testing. > .. and those calling quitApplication directly, and it was easier to write a new class .. Maybe including the server part in the test class name like TestServerQuitApplication might make sense? We could more easily find those special tests later.

Comment on attachment 8836204 [details] Bug 1337743 - Return error if quit request is cancelled; https://reviewboard.mozilla.org/r/111654/#review113250 > > You only have to register for quit-application-requested if there is a reason to cancel the quit request. In case of our quit() method, I don't see why we want to do that. > > Correct me if I’m wrong, but I thought the intention here was to send back an error if the request to quit the application _is_ cancelled by some other component? Potentially there are other observers elsewhere in the application that for their own reasons may request the quit to be cancelled, and Marionette should handle that. > > > With 'notifyObservers()' you will request a quit, and this call is blocking until every consumer of quit-application-requested has been responded. > > It doesn’t say anywhere in the observer documentation that `notifyObservers` actually _requests_ a quit? It says that it will _notify_ the _observers_ of given event. You have to explicitly call `Services.startup.quit` to actually initiate the quit procedures as far as I’m aware? > > Also, where is it documented that `notifyObservers` blocks until observers listening for `quit-application-requested` has processed? Wouldn’t this be to the contrary of how the asynchonicity of JavaScript works? > Correct me if I’m wrong, but I thought the intention here was to send back an error if the request to quit the application is cancelled by some other component? That's correct, but it doesn't work with the quit-application-requested topic. Here you can only discard the quit yourself, but you do not get the feedback if other components canceled it. > Also, where is it documented that notifyObservers blocks until observers listening for quit-application-requested has processed? Wouldn’t this be to the contrary of how the asynchonicity of JavaScript works? It's just how it works. See all the other code examples on DXR too. I'm not sure where it is documented, sorry. You can just try it out by canceling the quit request in your handler for the quit-application-requested topic.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review114826 Removing my r? request due to still open issues.

Comment on attachment 8836204 [details] Bug 1337743 - Return error if quit request is cancelled; https://reviewboard.mozilla.org/r/111654/#review114828 As mentioned on the other commit this is not the correct way and will not work at all.

Comment on attachment 8835630 [details] Bug 1337743 - Handle corrupt packets to Marionette; https://reviewboard.mozilla.org/r/111306/#review114830 ::: testing/marionette/dispatcher.js:174 (Diff revision 6) > + * Response to the message with |msgID|. > + */ > +Dispatcher.prototype.createResponse = function (msgID) { > + if (typeof msgID != "number") { > + msgID = -1; > + } Is there a reason why this check cannot be part of the constructor of the Response class? Why do we explicitely need this extra factory method?

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review114858 Just the one issue about the Python side. ::: testing/marionette/client/marionette_driver/marionette.py:1145 (Diff revision 6) > + > + # The vast majority of this function was implemented inside > + # the quitApplication command as part of bug 1337743, and can be > + # removed from here in Firefox 54 at the latest. > + > + flags = set(shutdown_flags) | set(("eAttemptQuit",)) If you pass in `"eForceQuit"` this will always fail with a duplicate flag, right? ::: testing/marionette/driver.js:2627 (Diff revision 6) > + this.deleteSession(); > + > + // delay response until the application is about to quit > + let quit = new Promise(resolve => { > + Services.obs.addObserver(() => { > - resp.send(); > + resp.send(); I assume that at this later time we know the socket is still open? I didn't read the discussion about why this is prefered compared to just sending the response asap and deleting the session.

Comment on attachment 8835630 [details] Bug 1337743 - Handle corrupt packets to Marionette; https://reviewboard.mozilla.org/r/111306/#review114830 > Is there a reason why this check cannot be part of the constructor of the Response class? Why do we explicitely need this extra factory method? Because `Response` expects to be constructed with an integer. Disallowing any other kind of input adds a layer of security where it cannot be constructed with invalid data. `createResponse` is used specifically in `Dispatcher` to craft a response when we do not know the message ID because we were unable to deserialise the packet that came in. This logic seems better contained within `Dispatcher` than to make it generally available, since after the packet has been marshaled into a `Message`, the message ID should always be present and valid – and the assertion in `Response`’s ctor is there to test that only valid responses are constructed.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review114858 > If you pass in `"eForceQuit"` this will always fail with a duplicate flag, right? I believe that was fixed in the first iteration of this patch but then dropped in the second revision of it. The intention here is indeed to not add `"eAttemptQuit"` if any other `*Quit` flag is given. However, it is not meant to stop the user from doing stupid things, such as passing in `("eAttemptQuit", "eForceQuit")`, for which one would rightly expect an error to be thrown. I’ve updated the method to (1) remove duplicates, and (2) add `"eAttemptQuit"` if it is not there _and_ there are no other `*Quit`s. Verified it with this test program: ```python def quit(*flags): # remove duplicates flags = set(flags) # add eAttemptQuit if there are no *Quits if not any(flag.endswith("Quit") for flag in flags): flags = flags | set(("eAttemptQuit",)) return flags assert quit() == set(["eAttemptQuit"]) assert quit("eAttemptQuit") == set(["eAttemptQuit"]) assert quit("eAttemptQuit", "eAttemptQuit") == set(["eAttemptQuit"]) assert quit("eRestart", "eAttemptQuit") == set(["eRestart", "eAttemptQuit"]) assert quit("eRestart") == set(["eRestart", "eAttemptQuit"]) assert quit("eForceQuit") == set(["eForceQuit"]) assert quit("eForceQuit", "eAttemptQuit") == set(["eForceQuit", "eAttemptQuit"]) ``` > I assume that at this later time we know the socket is still open? I didn't read the discussion about why this is prefered compared to just sending the response asap and deleting the session. There are two problems we are trying to address here: 1. Ensuring the response is successfully sent back to the client before the application shuts down. 2. Ensuring the response is not sent too early so that the client sends us another command before we have time to shut down. When to call `resp.send` is a balancing act, but sending it just before the `quit-application` notification should be safe with regards to the socket still being open, since XPCOM isn’t shutdown until after `xpcom-will-shutdown` and before `xpcom-shutdown`. Indeed it could be argued it is preferable to send back the response as late as possible, or as close to the application quit as possible, to minimise the chance of a race condition occurring by letting the client believe that Firefox has quit before it really has. Related to this, we ought to have a way of telling `Dispatcher` to not accept any new messages after `quitApplication` has been called, but I looked in to this and it is not currently possible because `GeckoDriver` does not hold a reference to `Dispatcher`.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113650 > Oh, and yes we cannot remove it yet, but we could add a version check in quit() and restart() to not have to call _request_in_app_shutdown() at all. The flags should be handled in both the public methods now anyway. So keeping the private method for legacy only. > Hm, too many commits for similar code changes is confusing. We should try to make those changes in the same commit. I have rectified this, and all logical changes related to `_request_in_app_shutdown` are now made in this patch. > Oh, and yes we cannot remove it yet, but we could add a version check in `quit()` and `restart()` to not have to call `_request_in_app_shutdown()` at all. The flags should be handled in both the public methods now anyway. So keeping the private method for legacy only. I think this will make uplifts confusing since the version check would have to be altered for each of them. I also don’t see the benefit if they both act the same. `_request_in_app_shutdown` only adds some extra padding on the client side before calling `quitApplication` which cannot hurt. Once the changes to `quitApplication` ride all the trains to stable, we can remove this padding and expect it to continue to function.

Comment on attachment 8836204 [details] Bug 1337743 - Return error if quit request is cancelled; https://reviewboard.mozilla.org/r/111654/#review113250 > > Correct me if I’m wrong, but I thought the intention here was to send back an error if the request to quit the application is cancelled by some other component? > > That's correct, but it doesn't work with the quit-application-requested topic. Here you can only discard the quit yourself, but you do not get the feedback if other components canceled it. > > > Also, where is it documented that notifyObservers blocks until observers listening for quit-application-requested has processed? Wouldn’t this be to the contrary of how the asynchonicity of JavaScript works? > > It's just how it works. See all the other code examples on DXR too. I'm not sure where it is documented, sorry. > > You can just try it out by canceling the quit request in your handler for the quit-application-requested topic. > That's correct, but it doesn't work with the quit-application-requested topic. Here you can only discard the quit yourself, but you do not get the feedback if other components canceled it. Which topic should I use instead? `quit-application-requested` is the topic currently used in the Python client: http://searchfox.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1145 > It's just how it works. See all the other code examples on DXR too. I'm not sure where it is documented, sorry. But herein lies the problem: I don’t want to _notify_ potential observers that I’m initiating a shutdown, I want _to be_ notified if anything _causes_ a shutdown so that I can return an error. Presumably `Services.startup.quit` issues `quit-apllication-requested` and `quit-application` notifications.

Comment on attachment 8835630 [details] Bug 1337743 - Handle corrupt packets to Marionette; https://reviewboard.mozilla.org/r/111306/#review114830 > Because `Response` expects to be constructed with an integer. Disallowing any other kind of input adds a layer of security where it cannot be constructed with invalid data. `createResponse` is used specifically in `Dispatcher` to craft a response when we do not know the message ID because we were unable to deserialise the packet that came in. > > This logic seems better contained within `Dispatcher` than to make it generally available, since after the packet has been marshaled into a `Message`, the message ID should always be present and valid – and the assertion in `Response`’s ctor is there to test that only valid responses are constructed. Ok, sounds fine with me then. Thanks.

Comment on attachment 8835630 [details] Bug 1337743 - Handle corrupt packets to Marionette; https://reviewboard.mozilla.org/r/111306/#review114902

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113650 > > Hm, too many commits for similar code changes is confusing. We should try to make those changes in the same commit. > > I have rectified this, and all logical changes related to `_request_in_app_shutdown` are now made in this patch. > > > Oh, and yes we cannot remove it yet, but we could add a version check in `quit()` and `restart()` to not have to call `_request_in_app_shutdown()` at all. The flags should be handled in both the public methods now anyway. So keeping the private method for legacy only. > > I think this will make uplifts confusing since the version check would have to be altered for each of them. > > I also don’t see the benefit if they both act the same. `_request_in_app_shutdown` only adds some extra padding on the client side before calling `quitApplication` which cannot hurt. Once the changes to `quitApplication` ride all the trains to stable, we can remove this padding and expect it to continue to function. > _request_in_app_shutdown only adds some extra padding on the client side before calling quitApplication which cannot hurt. If we have the same code in both the Python client and the server, Marionette would send the observer notification twice. I hope that all components are happy with that by such a fast triggering. Regarding the comment lets just tell when this gets removed and we should be fine. I hope we can get this uplifted to 52 still.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113650 > > _request_in_app_shutdown only adds some extra padding on the client side before calling quitApplication which cannot hurt. > > If we have the same code in both the Python client and the server, Marionette would send the observer notification twice. I hope that all components are happy with that by such a fast triggering. > > Regarding the comment lets just tell when this gets removed and we should be fine. I hope we can get this uplifted to 52 still. OK, so to summarise: No changes are required here, and this issue can be resolved? > In the case no quit will happen, we leak the added observer. Whereby checking again we would basically hang forever. It’s still not clear to me what action I need to take here. Should I remove the observer when it fires?

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review115402

Comment on attachment 8836204 [details] Bug 1337743 - Return error if quit request is cancelled; https://reviewboard.mozilla.org/r/111654/#review114832 I wish you could merge this commit with the former commit in this series. Also see the inline comments. ::: testing/marionette/client/marionette_driver/marionette.py:1150 (Diff revision 8) > > """ > > # The vast majority of this function was implemented inside > # the quitApplication command as part of bug 1337743, and can be > - # removed from here in Firefox 54 at the latest. > + # removed from here in Firefox 55 at the earliest. This needs to be changed in the previous commit, not here. ::: testing/marionette/driver.js:2596 (Diff revision 8) > const quits = ["eConsiderQuit", "eAttemptQuit", "eForceQuit"]; > > + // return error if the quit request is cancelled > + Services.obs.addObserver(() => { > + throw new UnknownError("Quit was aborted"); > + }, "quit-application-requested", false); I don't see the usage of `notifyObservers` which will give you the expected result. Not sure what you expect from this code. Such a registered observer handler can simply tell if a shutdown should be aborted or not. But it doesn't tell you if other registered handlers aborted the quit.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113650 > OK, so to summarise: No changes are required here, and this issue can be resolved? Only if `notifyObservers` for cancelQuit is getting called in JS, and when you have updated the version of Firefox because `Firefox 54 at the latest` is not correct. > It’s still not clear to me what action I need to take here. Should I remove the observer when it fires? I hope the explanation of how `notifyObservers` works in the review of the other commit is helpful for you to understand how it works. Otherwise we can also have a Vidyo chat.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review112812 > > For example, I wish to call self.marionette.instance.runner.wait() explicitly and ensure session state in the client torn down. I couldn’t find a way to do that with the private Marionette._request_in_app_quit method. > > As mentioned in the other commit we should get rid of calling this method for newer versions of Firefox which contain your changes. Maybe that would help here? > > > We don’t have a good way of testing methods in testing/marionette/driver.js and the GeckoDriver object. > > That's something I didn't know. In that case it's indeed the better way for testing. > > > .. and those calling quitApplication directly, and it was easier to write a new class .. > > Maybe including the server part in the test class name like TestServerQuitApplication might make sense? We could more easily find those special tests later. > As mentioned in the other commit we should get rid of calling this method for newer versions of Firefox which contain your changes. Maybe that would help here? Yes, but because the Marionette Python client is used for upgrade tests, we can’t modify/drop any behaviour from `Marionette._request_in_app_quit` yet. Some of the behaviour in there is irrelevant to the tests of the `quitApplication` command in Marionette, and that’s why (parts of it) is replicated here, in the test. > Maybe including the server part in the test class name like TestServerQuitApplication might make sense? We could more easily find those special tests later. Fixed that.

Comment on attachment 8836204 [details] Bug 1337743 - Return error if quit request is cancelled; https://reviewboard.mozilla.org/r/111654/#review114832 OK. > This needs to be changed in the previous commit, not here. Moved to previous commit. > I don't see the usage of `notifyObservers` which will give you the expected result. Not sure what you expect from this code. > > Such a registered observer handler can simply tell if a shutdown should be aborted or not. But it doesn't tell you if other registered handlers aborted the quit. The expectation is even written out in plain English in the comment: we expect it to return an error if the quit request happens to get cancelled. > Such a registered observer handler can simply tell if a shutdown should be aborted or not. But it doesn't tell you if other registered handlers aborted the quit. That should be fine? We don’t need to tell if it was another observer that caused the application to quit.

Comment on attachment 8836204 [details] Bug 1337743 - Return error if quit request is cancelled; https://reviewboard.mozilla.org/r/111654/#review113250 > > That's correct, but it doesn't work with the quit-application-requested topic. Here you can only discard the quit yourself, but you do not get the feedback if other components canceled it. > > Which topic should I use instead? `quit-application-requested` is the topic currently used in the Python client: http://searchfox.org/mozilla-central/source/testing/marionette/client/marionette_driver/marionette.py#1145 > > > It's just how it works. See all the other code examples on DXR too. I'm not sure where it is documented, sorry. > > But herein lies the problem: I don’t want to _notify_ potential observers that I’m initiating a shutdown, I want _to be_ notified if anything _causes_ a shutdown so that I can return an error. Presumably `Services.startup.quit` issues `quit-apllication-requested` and `quit-application` notifications. hskupin: Do you mind replying to this comment? I still don’t understand how this code is wrong.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113650 > Only if `notifyObservers` for cancelQuit is getting called in JS, and when you have updated the version of Firefox because `Firefox 54 at the latest` is not correct. > Only if notifyObservers for cancelQuit is getting called in JS I don’t follow. What exact change is necessary? > and when you have updated the version of Firefox because Firefox 54 at the latest is not correct. Fixed this. > I hope the explanation of how `notifyObservers` works in the review of the other commit is helpful for you to understand how it works. Otherwise we can also have a Vidyo chat. > I hope the explanation of how notifyObservers works in the review of the other commit is helpful for you to understand how it works. You haven’t replied to my latest comment on that from last week. I am still at loss what changes are needed. `addObserver` clearly does not work as described in the API documentation.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113650 > > Only if notifyObservers for cancelQuit is getting called in JS > > I don’t follow. What exact change is necessary? > > > and when you have updated the version of Firefox because Firefox 54 at the latest is not correct. > > Fixed this. As initially said, you have to simply copy and paste the code from the `execute_script()` call in marionette.py: https://dxr.mozilla.org/mozilla-central/rev/e1135c6fdc9bcd80d38f7285b269e030716dcb72/testing/marionette/client/marionette_driver/marionette.py#1142-1146 Once we have reached a version when we no longer need the code in marionette.py we can get rid of it. This code is actually a must have for the Marionette server. > > I hope the explanation of how notifyObservers works in the review of the other commit is helpful for you to understand how it works. > > You haven’t replied to my latest comment on that from last week. I am still at loss what changes are needed. `addObserver` clearly does not work as described in the API documentation. I definitely replied about that but as it looks like it went into another issue for this commit. Lets continue in the other one.

Comment on attachment 8836204 [details] Bug 1337743 - Return error if quit request is cancelled; https://reviewboard.mozilla.org/r/111654/#review113250 > hskupin: Do you mind replying to this comment? I still don’t understand how this code is wrong. I think the best here is to talk about all that in the chitchat tomorrow.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; I cleared the flag in mozreview. So lets talk about the necessary changes as mentioned already.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review113650 > As initially said, you have to simply copy and paste the code from the `execute_script()` call in marionette.py: > > https://dxr.mozilla.org/mozilla-central/rev/e1135c6fdc9bcd80d38f7285b269e030716dcb72/testing/marionette/client/marionette_driver/marionette.py#1142-1146 > > Once we have reached a version when we no longer need the code in marionette.py we can get rid of it. This code is actually a must have for the Marionette server. I have made this change: % git diff diff --git a/testing/marionette/driver.js b/testing/marionette/driver.js index e940c10c6dfb..e27e1102a934 100644 --- a/testing/marionette/driver.js +++ b/testing/marionette/driver.js @@ -2641,11 +2641,6 @@ GeckoDriver.prototype.acceptConnections = function (cmd, resp) { GeckoDriver.prototype.quitApplication = function* (cmd, resp) { const quits = ["eConsiderQuit", "eAttemptQuit", "eForceQuit"]; - // return error if the quit request is cancelled - Services.obs.addObserver(() => { - throw new UnknownError("Quit was aborted"); - }, "quit-application-requested", false); - let flags = []; if (typeof cmd.parameters.flags != "undefined") { flags = assert.array(cmd.parameters.flags); @@ -2677,16 +2672,19 @@ GeckoDriver.prototype.quitApplication = function* (cmd, resp) { this._server.acceptConnections = false; this.deleteSession(); + // error if the quit request is cancelled + let cancel = (subject, topic) => { + Services.obs.removeObserver(errOnFailure, topic); + throw new UnknownError("Quit was aborted"); + }; + Services.obs.addObserver(cancel, "quit-application-requested", false); + // delay response until the application is about to quit - let quit = new Promise(resolve => { - Services.obs.addObserver(() => { - resp.send(); - resolve(); - }, "quit-application", false); - }); + let quitApplication = new Promise(resolve => + Services.obs.addObserver(resolve, "quit-application", false)); Services.startup.quit(mode); - yield quit; + yield quitApplication.then(() => resp.send()); }; GeckoDriver.prototype.installAddon = function (cmd, resp) { That copies the code from the Python client exactly and throws when all the observers for "quit-application-requested" have been notified, if I understand you correctly. I moved it slightly down in the function so the connection to Services.startup.quit is clearer. Can you take a look and sign off on this change?

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review123008 ::: testing/marionette/driver.js:2680 (Diff revisions 6 - 13) > + // error if the quit request is cancelled > + let cancel = (subject, topic) => { > + Services.obs.removeObserver(errOnFailure, topic); > + throw new UnknownError("Quit was aborted"); > + }; > + Services.obs.addObserver(cancel, "quit-application-requested", false); `cancel` in this case has to be a `supports-PRBool` and constructed as you are able to see in marionette.py. Please use exactly that method. It's not a callback in this case. Also the last parameter is `null`, and not `false`. ::: testing/marionette/driver.js:2684 (Diff revisions 6 - 13) > + }; > + Services.obs.addObserver(cancel, "quit-application-requested", false); > + > // delay response until the application is about to quit > - let quit = new Promise(resolve => { > - Services.obs.addObserver(() => { > + let quitApplication = new Promise(resolve => > + Services.obs.addObserver(resolve, "quit-application", false)); The `quit-application` observer notication's data parameter value should be included in the response data. It included `shutdown` or `restart`, which we can use on the Python side to check if the correct action has been performed.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review123008 > `cancel` in this case has to be a `supports-PRBool` and constructed as you are able to see in marionette.py. Please use exactly that method. It's not a callback in this case. > > Also the last parameter is `null`, and not `false`. That is certainly how it is being used throughout Gecko: http://searchfox.org/mozilla-central/search?q=Services.obs.addObserver&path= I don’t know what you mean by “it’s not a callback in this case”. Also according to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIObserverService, the last argument to addObserver is supposed to be of type boolean. > The `quit-application` observer notication's data parameter value should be included in the response data. It included `shutdown` or `restart`, which we can use on the Python side to check if the correct action has been performed. From what I can tell, Marionette doesn’t include any data from quit-application at the moment so I don’t understand why this is needed. quit-application is a new observer I add as part of this patch, and it has not previously been in use by Marionette.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review123008 > That is certainly how it is being used throughout Gecko: http://searchfox.org/mozilla-central/search?q=Services.obs.addObserver&path= > > I don’t know what you mean by “it’s not a callback in this case”. > > Also according to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIObserverService, the last argument to addObserver is supposed to be of type boolean. You better search for the following string: https://dxr.mozilla.org/mozilla-central/search?q=Services.obs.notifyObservers(cancelQuit&redirect=false All those results are equal to what we currently have implemented in marionette.py, and which you didn't copy 1:1. > From what I can tell, Marionette doesn’t include any data from quit-application at the moment so I don’t understand why this is needed. quit-application is a new observer I add as part of this patch, and it has not previously been in use by Marionette. Right, and I proposed an addition here, so that the Python client is informed about the type of shutdown. With that we can ensure that the correct behavior was triggered. I see this as a big value to finally being able to further investigate bug 974971. Don't you think that make sense?

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review126234

> Right, and I proposed an addition here This patch has now been in review for many weeks and is needed to fix problematic behaviour in marionette. I think it would be a good idea to leave feature additions for followup work at this point because not landing anything here is causing real short-term issues.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review123008 > Right, and I proposed an addition here, so that the Python client is informed about the type of shutdown. With that we can ensure that the correct behavior was triggered. I see this as a big value to finally being able to further investigate bug 974971. > > Don't you think that make sense? Sorry, I thought you meant I was missing some functionality that existed before, but I see what you mean now. Returning the cause does indeed seem like an attractive thing to do. Since this is new functionality, I have added this in a separate commit. But the gist of it is this: > diff --git a/testing/marionette/driver.js b/testing/marionette/driver.js > index be6d529..0fee680 100644 > --- a/testing/marionette/driver.js > +++ b/testing/marionette/driver.js > @@ -2728,6 +2728,11 @@ GeckoDriver.prototype.acceptConnections = function (cmd, resp) { > * 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. > + * > * @throws {InvalidArgumentError} > * If |flags| contains unknown or incompatible flags, for example > * multiple Quit flags. > @@ -2774,11 +2779,18 @@ GeckoDriver.prototype.quitApplication = function* (cmd, resp) { > Services.obs.addObserver(cancel, "quit-application-requested", false); > > // delay response until the application is about to quit > - let quitApplication = new Promise(resolve => > - Services.obs.addObserver(resolve, "quit-application", false)); > + let quitApplication = new Promise(resolve => { > + Services.obs.addObserver( > + (subject, topic, data) => resolve(data), > + "quit-application", > + false); > + }); > > Services.startup.quit(mode); > - yield quitApplication.then(() => resp.send()); > + > + yield quitApplication > + .then(cause => resp.body.cause = cause) > + .then(() => resp.send()); > }; Resolving this issue so we can continue the discussion on the new patch.

(In reply to James Graham [:jgraham] from comment #180) > > Right, and I proposed an addition here > > This patch has now been in review for many weeks and is needed to fix > problematic behaviour in marionette. I think it would be a good idea > to leave feature additions for followup work at this point because not > landing anything here is causing real short-term issues. I tend to agree with this sentiment. We needed this to be fixed yesterday. I have dropped the remaining issue and the code associated to it, which is about making the quit command return an error from the server side if the quit application request is cancelled, and filed https://bugzilla.mozilla.org/show_bug.cgi?id=1350862 to follow up on this later. I hope this is satisfactory to all parties.

Comment on attachment 8835631 [details] Bug 1337743 - Stop appending eAttemptQuit in quitApplication; https://reviewboard.mozilla.org/r/111308/#review123008 > You better search for the following string: > > https://dxr.mozilla.org/mozilla-central/search?q=Services.obs.notifyObservers(cancelQuit&redirect=false > > All those results are equal to what we currently have implemented in marionette.py, and which you didn't copy 1:1. OK, I am going to call a close to this discussion and remove the associated code from my patch. I have filed https://bugzilla.mozilla.org/show_bug.cgi?id=1350862 to follow up on moving the cancel notification from quitting the application from the client to the server, and I’m dropping this issue in the interest of ensuring we land this changeset in a timely manner.