Closed Bug 1320380 Opened 4 years ago Closed 4 years ago

Throw IOError if application quit and no instance available

Categories

(Testing :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox52 fixed, firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: jgraham, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

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.
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)
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)
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.
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 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 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)
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.
Attachment #8815057 - Flags: review?(dburns)
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.
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)
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/7a1bee2d23ac
Status: ASSIGNED → RESOLVED
Closed: 4 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.