1320380 - Throw IOError if application quit and no instance available

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[James Graham [:jgraham]]

Before bug 1299216 running execute_async_script() when the browser closed raised an exception (IOError I think, but possibly something socket related). That bug regressed this behaviour and caused it to return None. This breaks wpt which uses a loop like the following to determine whether the browser is still open:

while True:
    try:
        marionette.exceute_async_script("")
    except Exception:
        break

To see this in action try running

|mach wpt testing/web-platform/tests/dom/historical.html|

and then close the browser.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Can you please point me to the line of code where this check is done? 

From my point of view nothing should have been changed. We are still re-raising the exception, and in case of a socket failure it will be IOError. If the browser is down you might get a MarionetteException:

>>> from marionette_driver.marionette import Marionette
>>> m = Marionette(bin="/Applications/FirefoxNightly.app/Contents/MacOS/firefox")
[manually shutdown Firefox]
>>> m.execute_script(" return 1")

The last line results in:

> marionette_driver.errors.MarionetteException: Please start a session

I don't see why None will be returned.

Comment 2

[James Graham [:jgraham]]

In [1]: import subprocess

In [2]: import marionette

In [4]: m = marionette.Marionette()

In [14]: subprocess.Popen(["obj-x86_64-pc-linux-gnu/dist/bin/firefox", "-P", "Test", "--marionette", "about:blank"])
Out[14]: <subprocess.Popen at 0x7f5d957dde50>

In [15]: ATTENTION: default value of option force_s3tc_enable overridden by environment.
ATTENTION: default value of option force_s3tc_enable overridden by environment.
1480356542502	Marionette	INFO	Listening on port 2828


In [15]: m.start_session()
Out[15]: 
{u'acceptInsecureCerts': False,
 u'browserName': u'firefox',
 u'browserVersion': u'53.0a1',
 u'command_id': 1,
 u'platformName': u'linux',
 u'platformVersion': u'4.4.0-47-generic',
 u'processId': 1599,
 u'proxy': {},
 u'raisesAccessibilityExceptions': False,
 u'rotatable': False,
 u'specificationLevel': 0}

In [16]: m.timeout.script = 2**28

In [17]: m.execute_async_script("")
<!-- kill firefox here -->
In [18]: 

So this clearly returns None. Note that your example is quite different because it closes marionette before the command is done.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I can see where the problem is. Will have a patch shortly.

Beside the above issue, is there a reason why you use subprocess.Popen() instead of passing the binary to Marionette and let it control the process itself?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1320380 - Rethrow exception in case of socket failures and no application instance available.

Review commit: https://reviewboard.mozilla.org/r/96048/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96048/

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

James, can you please check my patch? Locally it works just fine for me. Thanks.

Comment 6

[James Graham [:jgraham]]

Marionette can't control the process in web-platform-tests because the architecture separates out the browser process control and protocol for controlling the browser into components that can't directly communicate with each other.

Comment 7

[James Graham [:jgraham]]

Comment on attachment 8815057 [details]
Bug 1320380 - Rethrow exception in case of socket failures and no application instance available.

https://reviewboard.mozilla.org/r/96048/#review96216

::: testing/marionette/client/marionette_driver/marionette.py:777
(Diff revision 1)
> -            exc, val, tb = sys.exc_info()
> +        exc, val, tb = sys.exc_info()
>  
> +        if not self.instance:
> +            # If the application has been started externally we don't have any
> +            # further information.
> +            raise IOError, 'Process has been unexpectedly closed', tb

Why don't we re-raise the original exception here?

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8815057 [details]
Bug 1320380 - Rethrow exception in case of socket failures and no application instance available.

https://reviewboard.mozilla.org/r/96048/#review96216

> Why don't we re-raise the original exception here?

We raise the exception with the known tb, but I indeed miss the message of the former exception which we can include here similar to the other cases.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8815057 [details]
Bug 1320380 - Rethrow exception in case of socket failures and no application instance available.

https://reviewboard.mozilla.org/r/96048/#review96216

> We raise the exception with the known tb, but I indeed miss the message of the former exception which we can include here similar to the other cases.

Ok, so we cannot easily determine the current process status here. So re-raising the same exception might indeed be the best option here.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8815057 [details]
Bug 1320380 - Rethrow exception in case of socket failures and no application instance available.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96048/diff/1-2/

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8815057 [details]
Bug 1320380 - Rethrow exception in case of socket failures and no application instance available.

https://reviewboard.mozilla.org/r/96048/#review96716

It looks like that this patch misses another key part which is crash checking. We should also try to get a unit test up for it.

Comment 12

[James Graham [:jgraham]]

This patch works for me. Since it fixes a regression, please land it as soon as it has review and fix any further issues in a followup.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> It looks like that this patch misses another key part which is crash
> checking. We should also try to get a unit test up for it.

To wrap things up, I had another look at possible code paths and I don't think that the check for crashes should happen in Marionette itself if Marionette doesn't handle the status of the process. Instead the consumer of Marionette has to get crash checks implemented. Reason is that when we connect to a running process we do not have its PID, profile directory or any other kind of information. When the process shutdown unexpectedly there is also no way to retrieve more data. 

We could definitely improve the situation in the future but for now I would say we leave the patch as it is given the above argument.

David, your feedback regarding this issue is welcome before the review.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8815057 [details]
Bug 1320380 - Rethrow exception in case of socket failures and no application instance available.

https://reviewboard.mozilla.org/r/96048/#review96750

I will work on a test in a follow-up bug so we can get in this fix as soon as possible.

Comment 15

[David Burns :automatedtester]

Comment on attachment 8815057 [details]
Bug 1320380 - Rethrow exception in case of socket failures and no application instance available.

https://reviewboard.mozilla.org/r/96048/#review96776

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a1bee2d23ac
Rethrow exception in case of socket failures and no application instance available. r=automatedtester

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7a1bee2d23ac

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Important test-only patch which fixes a major regression. We need this on aurora too. Thanks.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/96cc481a52a3