Closed
Bug 1309556
Opened 7 years ago
Closed 7 years ago
delete_session() never gets called if a callback is passed to marionette.quit() and marionette.restart()
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(firefox51 unaffected, firefox52 fixed)
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox51 | --- | unaffected |
firefox52 | --- | fixed |
People
(Reporter: CuriousLearner, Assigned: whimboo)
References
Details
Attachments
(1 file)
Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used.
58 bytes,
text/x-review-board-request
|
ato
:
review+
|
Details |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:49.0) Gecko/20100101 Firefox/49.0 Build ID: 20160922113459 Steps to reproduce: While solving bug 1298803, we realized that call to `marionette.quit()` or `marionette.restart()` which was implemented in bug 1298800 does not call `delete_session()`. To reproduce: Call `marionette.quit()` with `in_app=True` and give a valid callback function. Actual results: `self.delete_session()` was never called. Expected results: `self.delete_session()` should be called even if a callback is passed to `marionette.quit()` or `marionette.restart()`
Reporter | ||
Updated•7 years ago
|
Summary: delete_session() never gets called if a callback is passed to marionette.quit() → delete_session() never gets called if a callback is passed to marionette.quit() and marionette.restart()
Assignee | ||
Comment 1•7 years ago
|
||
So what we should do is: 1. Move the call to delete_session() to the correct place 2. Update the unit tests for using a callback() to have their own shutdown code as copied from _request_inapp_shutdown Sanyam, do you want to work on this fix?
Assignee | ||
Comment 2•7 years ago
|
||
I will pick this up so we can get it in the next planned marionette-client release which happens soon.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8800684 -
Flags: review?(mjzffr)
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8800684 [details] Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. https://reviewboard.mozilla.org/r/85562/#review84166 Try shows some bustage on Linux64 so far only. Looks like I have to investigate more.
Assignee | ||
Updated•7 years ago
|
Attachment #8800684 -
Flags: review?(mjzffr)
Assignee | ||
Comment 5•7 years ago
|
||
We actually face a race condition at least on Linux where the pid stays the same with a restart of Firefox. The shutdown is a bit slower, and Marionette already arrives at "start_session". It means that for a split of a second we are connecting to the old Firefox instance which is going to shutdown. And then the connection dies: IOError: Process killed because the connection to Marionette server is lost. Check gecko.log for errors (Reason: No data received over socket) Maybe it is related to the missing quitApplication call, which is not done via a callback.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
Latest version of this patch works great again across platforms as the try build shows: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8047592c30e7 Sadly, all tests on WinXP are busted again due to the a11y crashes! This is really getting frustrating because I really didn't change anything which should cause this. May this be a try build specific issue only and doesn't appear on integration branches?
Assignee | ||
Comment 8•7 years ago
|
||
Oh, I think I know what's going on.... We do notrun e10s tests on WinXP for integration branches! That's why all is fine: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=mn%20Windows%20XP&bugfiler So I will simply ignore those failures and only care about Mn jobs in the future for XP. Beside this I was also thinking about backward compatibility. But given that the usage of a callback has been added with Firefox 52 (the current branch on mozilla-central) we don't need that. Former versions won't hit this code path, except update tests which will use the new client but for those we won't integrate it.
Assignee | ||
Updated•7 years ago
|
status-firefox51:
--- → unaffected
status-firefox52:
--- → affected
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8800684 [details] Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. https://reviewboard.mozilla.org/r/85562/#review84508 This patch is excellent. I just have a few minor requests for changes. ::: testing/marionette/driver.js:2579 (Diff revision 3) > + * > + * @param {boolean} state > + * True if the server should accept new socket connections. > + */ > +GeckoDriver.prototype.acceptConnections = function(cmd, resp) { > + logger.info(cmd.parameters.state) Delete ::: testing/marionette/driver.js:2580 (Diff revision 3) > + * @param {boolean} state > + * True if the server should accept new socket connections. > + */ > +GeckoDriver.prototype.acceptConnections = function(cmd, resp) { > + logger.info(cmd.parameters.state) > + this._acceptConnections(cmd.parameters.state); Use `value` instead of `state`. Also add a boolean type check. ::: testing/marionette/driver.js:2581 (Diff revision 3) > + * True if the server should accept new socket connections. > + */ > +GeckoDriver.prototype.acceptConnections = function(cmd, resp) { > + logger.info(cmd.parameters.state) > + this._acceptConnections(cmd.parameters.state); > + resp.send(); This is done implicitly, so can be safely removed. ::: testing/marionette/server.js:75 (Diff revision 3) > Services.io.manageOfflineStatus = false; > Services.io.offline = false; > } > > - let stopSignal = () => this.stop(); > - return new GeckoDriver(appName, stopSignal); > + let acceptConnections = () => this.acceptConnections(); > + return new GeckoDriver(appName, acceptConnections); I wonder if we can just pass a reference to `MarionetteServer`, i.e. `this`. ::: testing/marionette/server.js:112 (Diff revision 3) > }; > > MarionetteServer.prototype.onSocketAccepted = function( > serverSocket, clientSocket) { > + if (!this._acceptConnections) { > + logger.debug('New connections are currently now allowed.'); I think we should drop this in favour of a log message n disabling new connections. Also use double quotes and drop the punctuation.
Attachment #8800684 -
Flags: review?(ato) → review-
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800684 [details] Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. https://reviewboard.mozilla.org/r/85562/#review84508 > This is done implicitly, so can be safely removed. Interesting that we still have this in quitApplication(). I assume it has to be sent earlier because we quit the application? > I wonder if we can just pass a reference to `MarionetteServer`, i.e. `this`. Good idea. It works just fine. > I think we should drop this in favour of a log message n disabling new connections. > > Also use double quotes and drop the punctuation. Maybe we also want to have a case when we accept connections again. Otherwise it will be hard to debug.
Assignee | ||
Comment 12•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800684 [details] Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. https://reviewboard.mozilla.org/r/85562/#review84508 > Use `value` instead of `state`. > > Also add a boolean type check. None of the other methods are actually doing a type check. I think if we want that we should do it in another patch.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 14•7 years ago
|
||
Former try build before my latest changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8047592c30e7 With the latest changes I triggered an artifact only build for Linux64: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f29a457a4f59
Comment 15•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800684 [details] Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. https://reviewboard.mozilla.org/r/85562/#review84508 > None of the other methods are actually doing a type check. I think if we want that we should do it in another patch. Unfortunately not everything does proper type checks, but we should enfoce it with new additions. Reopening this. You can do: ```js if (typeof cmd.parameters.state != "boolean") { throw InvalidArgumentError(); } ``` > Interesting that we still have this in quitApplication(). I assume it has to be sent earlier because we quit the application? Precisely. A second call to `Response#send` is harmless as it checks whether the response has been sent. > Maybe we also want to have a case when we accept connections again. Otherwise it will be hard to debug. Sure, that’s fine. Let’s do both. Make sure the log message when disbaling it is INFO level, and the one when attempting to connect again is WARN.
Comment 16•7 years ago
|
||
mozreview-review |
Comment on attachment 8800684 [details] Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. https://reviewboard.mozilla.org/r/85562/#review84882 ::: testing/marionette/driver.js:94 (Diff revision 4) > * documentation refers to the contents of the {@code cmd.parameters} > * object. > * > * @param {string} appName > * Description of the product, for example "B2G" or "Firefox". > - * @param {function()} stopSignal > + * @param {object} marionetteServer s/object/MarionetteServer/ for a more concrete type. ::: testing/marionette/driver.js:99 (Diff revision 4) > - * @param {function()} stopSignal > - * Signal to stop the Marionette server. > + * @param {object} marionetteServer > + * The instance of Marionette server. > */ > -this.GeckoDriver = function(appName, stopSignal) { > +this.GeckoDriver = function(appName, marionetteServer) { > this.appName = appName; > - this.stopSignal_ = stopSignal; > + this._marionetteServer = marionetteServer; I think I’d prefer this to be just `_server`.
Attachment #8800684 -
Flags: review?(ato) → review-
Comment 17•7 years ago
|
||
mozreview-review |
Comment on attachment 8800684 [details] Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. https://reviewboard.mozilla.org/r/85562/#review84886 ::: testing/marionette/server.js:77 (Diff revision 4) > } > > - let stopSignal = () => this.stop(); > - return new GeckoDriver(appName, stopSignal); > + return new GeckoDriver(appName, this); > +}; > + > +MarionetteServer.prototype.acceptConnections = function(value) { Make this a setter. ::: testing/marionette/server.js:79 (Diff revision 4) > - let stopSignal = () => this.stop(); > - return new GeckoDriver(appName, stopSignal); > + return new GeckoDriver(appName, this); > +}; > + > +MarionetteServer.prototype.acceptConnections = function(value) { > + if (!value) { > + logger.debug("New connections are no longer accepted"); info ::: testing/marionette/server.js:80 (Diff revision 4) > + } > + else { put `else` on the same line as `}` ::: testing/marionette/server.js:82 (Diff revision 4) > +MarionetteServer.prototype.acceptConnections = function(value) { > + if (!value) { > + logger.debug("New connections are no longer accepted"); > + } > + else { > + logger.debug("New connections are accepted"); info ::: testing/marionette/server.js:107 (Diff revision 4) > this.alive = false; > }; Remember to set `this._acceptConnections` to false here.
Assignee | ||
Comment 18•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800684 [details] Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. https://reviewboard.mozilla.org/r/85562/#review84886 > Remember to set `this._acceptConnections` to false here. Good catch. Totally missed that here.
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8800684 [details] Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. https://reviewboard.mozilla.org/r/85562/#review84882 > I think I’d prefer this to be just `_server`. Yeah. With MarionetteServer as type it will be enough.
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8800684 [details] Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. https://reviewboard.mozilla.org/r/85562/#review85036
Attachment #8800684 -
Flags: review?(ato) → review+
Comment 22•7 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/46457bc48303 Ensure to correctly shutdown the application for quit/restart when callbacks are used. r=ato
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/46457bc48303
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•10 months ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•