965782 - Exception marionette.errors.InvalidResponseException: InvalidResponseException()

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Carsten Book [:Tomcat]]

noticed in some recent timeout testfailures for b2g the error 

Exception marionette.errors.InvalidResponseException: InvalidResponseException() in <bound method Marionette.__del__ of <marionette.marionette.Marionette object at 0x1f7e3d0>> ignored

examples are

https://tbpl.mozilla.org/php/getParsedLog.php?id=33807507&tree=Mozilla-Inbound
and
https://tbpl.mozilla.org/php/getParsedLog.php?id=33808637&tree=Fx-Team

not sure what this is, maybe a regression ?

Comment 1

[Malini Das [:mdas] - Away, not checking bugmail]

These are showing up in Mochitests with the pattern:

05:46:30     INFO -  97351 INFO TEST-PASS | /tests/layout/style/test/test_value_cloning.html | computed values should match when cloning -moz-transform-origin: calc(20px + 50%) calc(50% - 10px)
05:46:30     INFO -  97352 INFO TEST-PASS | /tests/layout/style/test/test_value_cloning.html | serialization should match when cloning -moz-transform-origin: calc(20px + 1em) calc(20px / 2)
05:52:00     INFO -  9735B2GRunner TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_value_cloning.html | application timed out after 330.0 seconds with no output
05:52:01     INFO -  B2GRunner INFO | checking for crashes in '/data/local/tests/profile/minidumps'
05:53:10     INFO -  Mochitest INFO | runtestsb2g.py | Running tests: end.
05:56:11     INFO -  3 INFO TEST-PASS | /tests/layout/style/test/test_value_cloning.html | computed values should match when cloning -moz-transform-origin: calc(20px + 1em) calc(20px / 2)
05:56:11     INFO -  97354 INFO TEST-PASS | /tests/layout/style/test/test_value_cloning.html | serialization should match when cloning -moz-transform-origin: calc(20px) calc(20px)
05:56:11     INFO -  97355 INFO TEST-PASS | /tests/layout/style/test/test_value_cloning.html | computed values should match when cloning -moz-transform-origin: calc(20px) calc(20px)
05:56:11     INFO -  Exception marionette.errors.InvalidResponseException: InvalidResponseException() in <bound method Marionette.__del__ of <marionette.marionette.Marionette object at 0x165c3d0>> ignored
05:56:11    ERROR - Return code: 247
05:56:11     INFO - dumping logcat

Ie: a test times out, some tests run after that, and then marionette has trouble.

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

ahal suggested https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/remote.py#L209 and https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/remote.py#L332 as likely areas to look into

Comment 3

[Ed Morley [:emorley]]

Many of the logs being pasted in bug 965677 have this failure present.

Comment 4

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: don't throw exceptions when trying to cleanup

delete_session is useful if the session is still active and can be used. There are times when it isn't (socket closed, gecko process crashed, marionette crashed, timeout errors etc.), and calling it will just generate one of many kinds of errors.

I've modified the code so we simply try to delete the session if possible or ignore the error silently, since it is not crucial to delete the session here and the error is not useful on its own. Once the marionette object is deleted, the current connection will be closed anyway, and if the marionette server is still running, then it will accept a new connection for the next test.

The current behaviour is to throw the error and it's been causing some problems in the tests.

Comment 5

[William Lachance (:wlach)]

Comment on attachment 8368079 [details] [diff] [review]
don't throw exceptions when trying to cleanup

Agree with the general idea but I think we should be more explicit about which types of exceptions we want to ignore. Also, we should have a comment here describing the logic.

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

I would agree with you, but there aren't any Exceptions here that we do want to bubble up, unless you can think of one?  

The idea here is to just try to delete the session to do server side cleanup. If we can't successfully do that, it doesn't really matter to cleanup() or whoever is deleting the object, since we can't assume that the marionette server will be running (or in a good state) when we're done with the Marionette object.

Comment 7

[William Lachance (:wlach)]

(In reply to Malini Das [:mdas] from comment #6)
> I would agree with you, but there aren't any Exceptions here that we do want
> to bubble up, unless you can think of one?  

Even syntax errors? :) A blanket try...except allows code like this to fail silently:

>>> try:
...     bldka
... except Exception:
...     pass
... 
>>> 

I really frown on this because it can mask problems in your code. I would just enumerate out the failure possibilities and catch all of them.

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

ok, I should note that the exceptions thrown here will be varied, depending on the problem (InvalidResponseException, socket.timeout, some other stuff I saw locally but can't remember) so if we want to whitelist them, the list won't be complete until we let this out in the wild and add any upcoming exception

Comment 9

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: marioerr

Comment 10

[William Lachance (:wlach)]

Comment on attachment 8368094 [details] [diff] [review]
marioerr

Please add a comment explaining why we ignore these exceptions. r+ with that change.

Comment 11

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: ignore certain exceptions

posting updated patch, moving r+ forward.

landed in m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73e4adef1599

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/73e4adef1599