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

RESOLVED FIXED in Firefox 42

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jgriffin, Assigned: jgriffin)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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().
Bug 1182681 - Raise IOError when receiving empty string on socket, r=jgraham
Attachment #8632334 - Flags: review?(james)
Attachment #8632334 - Flags: review?(james)
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 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!
Attachment #8632334 - Flags: review+
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.
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.
Agreed; I'll look at moving that logic into the driver.
Bug 1182681 - Raise IOError when we receive an emtry string, r=jgraham
Attachment #8632868 - Flags: review?(james)
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 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
Attachment #8632334 - Flags: review+ → review?(james)
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 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!
Attachment #8632868 - Flags: review?(james) → review+
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!
Attachment #8632334 - Flags: review?(james) → review+
Duplicate of this bug: 1183444
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.
Attachment #8633554 - Flags: review?(jgriffin)
Attachment #8633554 - Flags: review?(jgriffin) → review+
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
Attachment #8632334 - Flags: review+ → review?(james)
Attachment #8632868 - Attachment is obsolete: true
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.
Attachment #8632334 - Flags: review?(james) → review+
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!
(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.
https://hg.mozilla.org/mozilla-central/rev/6bf6fd025266
https://hg.mozilla.org/mozilla-central/rev/e667c2392aa8
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.