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)

Product:
Remote Protocol
Component:
Marionette
Version:
52 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 unaffected, firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
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()`
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()
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?
Blocks: 1298800, 1298803
Status: UNCONFIRMED → NEW
Component: Untriaged → Marionette
Ever confirmed: true
Product: Core → Testing
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
Attachment #8800684 - Flags: review?(mjzffr)
Blocks: 1309899
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.
Attachment #8800684 - Flags: review?(mjzffr)
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.
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?
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.
Blocks: 1283906
Blocks: 1141483
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-
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.
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.
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
Blocks: 1303834
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 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 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.
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.
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/46457bc48303
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1317707
No longer depends on: 1317707
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.