Closed Bug 965782 Opened 6 years ago Closed 6 years ago

Exception marionette.errors.InvalidResponseException: InvalidResponseException()

Categories

(Testing :: Marionette, defect)

x86
Linux
defect
Not set

Tracking

(b2g-v1.3 fixed)

RESOLVED FIXED
mozilla29
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: cbook, Assigned: mdas)

References

Details

Attachments

(1 file, 2 obsolete files)

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 ?
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.
Many of the logs being pasted in bug 965677 have this failure present.
Blocks: 965677
Blocks: 965705
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.
Attachment #8368079 - Flags: review?(wlachance)
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.
Attachment #8368079 - Flags: review?(wlachance) → review-
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.
(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.
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
Attached patch marioerr (obsolete) — Splinter Review
Attachment #8368079 - Attachment is obsolete: true
Attachment #8368094 - Flags: review?(wlachance)
Comment on attachment 8368094 [details] [diff] [review]
marioerr

Please add a comment explaining why we ignore these exceptions. r+ with that change.
Attachment #8368094 - Flags: review?(wlachance) → review+
posting updated patch, moving r+ forward.

landed in m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73e4adef1599
Attachment #8368094 - Attachment is obsolete: true
Attachment #8368114 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/73e4adef1599
Assignee: nobody → mdas
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.