1182681 - Raise IOError when we receive an empty string over the socket, and we're not tracking the gecko instance

Status: RESOLVED FIXED

(Testing :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

Bug 1166033 added support for failing fast when a Marionette socket request is pending and the underlying binary has died.

Unfortunately, this broke wptrunner, because we no longer raise IOError when receiving an empty string on socket.recv().

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham

Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham

Comment 2

[James Graham [:jgraham]]

Comment on attachment 8632334 [details]
MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham

https://reviewboard.mozilla.org/r/13041/#review11635

::: testing/marionette/transport/marionette_transport/transport.py:61
(Diff revision 1)
>                  # If we've launched the binary we've connected to, make

This is fine, I guess but I'm still confused about what the case is where the connection is closed (i.e. the empty string is returned) but we want to keep polling for more data. Are we expecting the connection to be established at some point in the future? If that's the case, how did connect() succeed here? Or are we skipping the connect() call somehow (in which case it seems like the calling code is wrong).

Comment 3

[James Graham [:jgraham]]

Comment on attachment 8632334 [details]
MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham

https://reviewboard.mozilla.org/r/13041/#review11637

Ship It!

Comment 4

[Jonathan Griffin (:jgriffin)]

We can get an empty string back while the process is in the midst of shutting down; keeping polling like this means we don't raise an IOError until the process has died, which means we won't try to restart it while the original process is still running.

I can change the loop so that it just polls the instance in this case, rather than the socket + the instance.

Comment 5

[James Graham [:jgraham]]

Yeah I guess the structure I would prefer is

while True:
  data = socket.recv()
  if not data:
    break
  response += data
  if have_full_response:
    return response

if self.instance:
  self.instance.wait()

raise IOError

Although really it seems strange that the transport layer is in charge of managing the instance here.

Comment 6

[Jonathan Griffin (:jgriffin)]

Agreed; I'll look at moving that logic into the driver.

Comment 7

[Jonathan Griffin (:jgriffin)]

Attachment: MozReview Request: Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham

Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham

Comment 8

[James Graham [:jgraham]]

https://reviewboard.mozilla.org/r/13039/#review11727

::: testing/marionette/driver/marionette_driver/marionette.py:695
(Diff revision 2)
> +                returncode = self.instance.runner.process_handler.wait()

I think the runner itself has a .wait() method.

Comment 9

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8632334 [details]
MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham

Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham

Comment 10

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8632868 [details]
MozReview Request: Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham

Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham

Comment 11

[James Graham [:jgraham]]

Comment on attachment 8632868 [details]
MozReview Request: Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham

https://reviewboard.mozilla.org/r/13145/#review11733

Ship It!

Comment 12

[James Graham [:jgraham]]

Comment on attachment 8632334 [details]
MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham

https://reviewboard.mozilla.org/r/13041/#review11767

Ship It!

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5195daae919e

Comment 14

[(no longer active)]

This broke web-platform-tests, so I backed it out:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5195daae919e
https://hg.mozilla.org/integration/mozilla-inbound/rev/84ffb30dd5b9

Comment 16

[James Graham [:jgraham]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acfcf574a1e9

This was missing re-raising the IOError in the case that self.instance was None. However it's also going to be necessary to update some wpt metadata because this patch also makes us correctly detect some crashes again (with the current behaviour on inbound they are incorrectly being seen as timeouts because we wait so long in the marionette driver).

Ehsan: I don't know if it was obvious, but this is the bug to make quiting the browser after running a wpt actually quit the harness rather than hanging.

Comment 17

[James Graham [:jgraham]]

Attachment: wpt-expected.diff

Comment 18

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8632334 [details]
MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham

Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham
* * *
Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham

Comment 19

[Jonathan Griffin (:jgriffin)]

try run with these updated patches:  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4d52c59e0d5

Comment 20

[Jonathan Griffin (:jgriffin)]

This looks good except for a perma-orange on OSX 10.8 which doesn't matter since we don't run 10.8 on trunk branches (not sure why we do on try...).  I'll land if you give an r+ jgraham.

Comment 21

[James Graham [:jgraham]]

Comment on attachment 8632334 [details]
MozReview Request: Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham

https://reviewboard.mozilla.org/r/13041/#review11939

Ship It!

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bf6fd025266
https://hg.mozilla.org/integration/mozilla-inbound/rev/e667c2392aa8

Comment 23

[(no longer active)]

(In reply to James Graham [:jgraham] from comment #16)
> Ehsan: I don't know if it was obvious, but this is the bug to make quiting
> the browser after running a wpt actually quit the harness rather than
> hanging.

Awesome!  Thanks for the explanation.

Comment 24

[[Back August 16] Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/6bf6fd025266
https://hg.mozilla.org/mozilla-central/rev/e667c2392aa8