Throw IOError if application quit and no instance available

RESOLVED FIXED in Firefox 52

Status

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jgraham, Assigned: whimboo)

Tracking

(Blocks 1 bug, {regression})

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
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.
Reporter

Updated

3 years ago
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin) → needinfo?(james)
Reporter

Comment 2

3 years ago
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.
Flags: needinfo?(james)
Reporter

Updated

3 years ago
Flags: needinfo?(hskupin)
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?
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin) → needinfo?(james)
Blocks: 1299216
No longer depends on: 1299216
Attachment #8815057 - Flags: review?(dburns)
Summary: execute_async_script returns None if the browser is closed → Throw IOError if application quit and no instance available
James, can you please check my patch? Locally it works just fine for me. Thanks.
Reporter

Comment 6

3 years ago
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.
Reporter

Comment 7

3 years ago
mozreview-review
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?
Assignee

Comment 8

3 years ago
mozreview-review-reply
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.
Attachment #8815057 - Flags: review?(dburns)
Assignee

Comment 9

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Attachment #8815057 - Flags: review?(dburns)
Assignee

Comment 11

3 years ago
mozreview-review
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.
Reporter

Comment 12

3 years ago
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.
Flags: needinfo?(james)
(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.
Blocks: 1320073
Flags: needinfo?(dburns)
Assignee

Comment 14

3 years ago
mozreview-review
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

3 years ago
mozreview-review
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
Attachment #8815057 - Flags: review?(dburns) → review+

Comment 16

3 years ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7a1bee2d23ac
Rethrow exception in case of socket failures and no application instance available. r=automatedtester

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7a1bee2d23ac
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Important test-only patch which fixes a major regression. We need this on aurora too. Thanks.
Flags: needinfo?(dburns)
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.