1507283 - Review possible relaxed deserialization of payload data (not strict WebDriver conformant)

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P1)

Description

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

A couple of Github issues have been filed recently which are all about several commands not working because Serde refuses deserialization due to unknown keys in the payload data, which mostly did exist for backward compatibility.

An example here is:

https://github.com/mozilla/geckodriver/issues/1426
https://github.com/theintern/leadfoot/issues/166

I wonder if we should relax our deserialization a bit so that unknown keys are simply ignored, and don't cause a failure. As it looks like there are still a couple of clients around, which don't work when geckodriver operates in strict mode.

What do you think?

Comment 1

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

Note, that my Serde changes caused this change of behavior in geckodriver.

Comment 2

[James Graham [:jgraham]]

Yes, sorry I didn't catch this originally. The spec ignores unknown keys except in a very few cases (notably the fields for capabilities). We should certainly do the same (and write tests for it!).

Comment 3

[David Burns :automatedtester]

We should allow unknown keys to go through unless there is a very good reason not to

Comment 4

[Andreas Tolfsen ❲:ato❳]

I just commented on this in
https://github.com/mozilla/geckodriver/issues/1422#issuecomment-439384695
before I saw this too, which gives the historical context similar
to what David and James said.

Comment 5

[Andreas Tolfsen ❲:ato❳]

The specific problem in the linked bugs are to do with Set Timeouts,
which also necessitates a change in the specification as well as
Marionette:
https://searchfox.org/mozilla-central/rev/c0b26c40769a1e5607a1ae8be37fe64df64fc55e/testing/marionette/capabilities.js#62
I’ve filed https://bugzilla.mozilla.org/show_bug.cgi?id=1507782
about relaxing the Marionette deserialisation too.

Comment 6

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

The problem with timeout parameters actually was that we denied unknown fields. Thankfully that flag for Serde was only set for this struct, so nothing else should actually be affected. I will add a couple more webdriver unittests.

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd66afb4aab5d8c6cb935422ed6cbaf3c836b53b

Comment 8

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

Attachment: Bug 1507283 - [geckodriver] Relax deserialization of timeouts parameters. r?ato

It fixes the regression from the transition to Serde (bug 1396821),
which accidentally denied unknown fields.

Comment 9

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

This is not blocked by bug 1507782.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bec3fe861b06
[geckodriver] Relax deserialization of timeouts parameters. r=ato

Comment 11

[Natalia Csoregi [:nataliaCs]]

Backed out for /webdriver/tests/set_timeouts/set.py failures

backout: https://hg.mozilla.org/integration/autoland/rev/aefe78c0ffdfd7db65e03a893b75e36c8d8fb111

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=wd2&revision=bec3fe861b0652f81ba635eee4155b8063430d5a&selectedJob=224032714

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=224032714&repo=autoland&lineNumber=85291

[task 2019-01-25T15:08:30.018Z] 15:08:30 INFO - TEST-PASS | /webdriver/tests/set_timeouts/set.py | test_parameters_empty_no_change
[task 2019-01-25T15:08:30.018Z] 15:08:30 INFO - TEST-PASS | /webdriver/tests/set_timeouts/set.py | test_script_parameter_empty_no_change
[task 2019-01-25T15:08:30.019Z] 15:08:30 INFO - TEST-UNEXPECTED-FAIL | /webdriver/tests/set_timeouts/set.py | test_key_invalid - assert 200 == 400
[task 2019-01-25T15:08:30.019Z] 15:08:30 INFO - session = <Session 6d57baf4-2d60-40d7-bc65-451cea91b5aa>
[task 2019-01-25T15:08:30.019Z] 15:08:30 INFO -
[task 2019-01-25T15:08:30.020Z] 15:08:30 INFO - def test_key_invalid(session):
[task 2019-01-25T15:08:30.020Z] 15:08:30 INFO - response = set_timeouts(session, {"foo": 1000})
[task 2019-01-25T15:08:30.020Z] 15:08:30 INFO - > assert_error(response, "invalid argument")
[task 2019-01-25T15:08:30.020Z] 15:08:30 INFO -
[task 2019-01-25T15:08:30.020Z] 15:08:30 INFO - response = <Responsetatus=200 body={"value": null}>
[task 2019-01-25T15:08:30.021Z] 15:08:30 INFO - session = <Session 6d57baf4-2d60-40d7-bc65-451cea91b5aa>
[task 2019-01-25T15:08:30.022Z] 15:08:30 INFO -
[task 2019-01-25T15:08:30.022Z] 15:08:30 INFO - tests/web-platform/tests/webdriver/tests/set_timeouts/set.py:53:
[task 2019-01-25T15:08:30.022Z] 15:08:30 INFO - _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
[task 2019-01-25T15:08:30.022Z] 15:08:30 INFO -
[task 2019-01-25T15:08:30.022Z] 15:08:30 INFO - response = <Responsetatus=200 body={"value": null}>
[task 2019-01-25T15:08:30.023Z] 15:08:30 INFO - error_code = 'invalid argument'
[task 2019-01-25T15:08:30.023Z] 15:08:30 INFO -
[task 2019-01-25T15:08:30.024Z] 15:08:30 INFO - def assert_error(response, error_code):
[task 2019-01-25T15:08:30.024Z] 15:08:30 INFO - """
[task 2019-01-25T15:08:30.024Z] 15:08:30 INFO - Verify that the provided webdriver.Response instance described
[task 2019-01-25T15:08:30.024Z] 15:08:30 INFO - a valid error response as defined by dfn-send-an-error and
[task 2019-01-25T15:08:30.024Z] 15:08:30 INFO - the provided error code.
[task 2019-01-25T15:08:30.024Z] 15:08:30 INFO -
[task 2019-01-25T15:08:30.025Z] 15:08:30 INFO - :param response: webdriver.Response instance.
[task 2019-01-25T15:08:30.025Z] 15:08:30 INFO - :param error_code: String value of the expected error code
[task 2019-01-25T15:08:30.025Z] 15:08:30 INFO - """
[task 2019-01-25T15:08:30.025Z] 15:08:30 INFO - > assert response.status == errors[error_code]
[task 2019-01-25T15:08:30.026Z] 15:08:30 INFO - E assert 200 == 400
[task 2019-01-25T15:08:30.026Z] 15:08:30 INFO - E + where 200 = <Responsetatus=200 body={"value": null}>.status
[task 2019-01-25T15:08:30.026Z] 15:08:30 INFO -
[task 2019-01-25T15:08:30.026Z] 15:08:30 INFO - error_code = 'invalid argument'
[task 2019-01-25T15:08:30.027Z] 15:08:30 INFO - response = <Responsetatus=200 body={"value": null}>
[task 2019-01-25T15:08:30.027Z] 15:08:30 INFO -
[task 2019-01-25T15:08:30.027Z] 15:08:30 INFO - tests/web-platform/tests/webdriver/tests/support/asserts.py:50: AssertionError
[task 2019-01-25T15:08:30.040Z] 15:08:30 INFO - ..................................
[task 2019-01-25T15:08:30.043Z] 15:08:30 INFO - TEST-OK | /webdriver/tests/set_timeouts/set.py | took 7860ms

Comment 12

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

This is a problem with an invalid test, which shouldn't have been added to the wdspec tests. Reason is that it uses an unknown key for the type of timeout, which should simply be ignored. And that is what this patch is actually doing.

As discussed with Andreas on IRC we will remove this specific test.

Comment 13

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

Attachment: Bug 1507283 - [wdspec] Remove invalid set timeout test "test_key_invalid". r?ato

Depends on D17634

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c311adb2434
[geckodriver] Relax deserialization of timeouts parameters. r=ato
https://hg.mozilla.org/integration/autoland/rev/4be8f5920abe
[wdspec] Remove invalid set timeout test "test_key_invalid". r=ato

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8c311adb2434
https://hg.mozilla.org/mozilla-central/rev/4be8f5920abe

Comment 16

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15101 for changes under testing/web-platform/tests