Closed Bug 1507283 Opened 6 years ago Closed 6 years ago

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

Categories

(Testing :: geckodriver, defect, P1)

Product:
Testing
Component:
geckodriver
Type:
defect

Tracking

(firefox-esr60 unaffected, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1507283 - [geckodriver] Relax deserialization of timeouts parameters. r?ato
(deleted), text/x-phabricator-request
Details
Bug 1507283 - [wdspec] Remove invalid set timeout test "test_key_invalid". r?ato
(deleted), text/x-phabricator-request
Details
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?
Flags: needinfo?(james)
Flags: needinfo?(dburns)
Flags: needinfo?(ato)
Note, that my Serde changes caused this change of behavior in geckodriver.
Blocks: 1396821
Keywords: regression
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!).
Flags: needinfo?(james)
We should allow unknown keys to go through unless there is a very good reason not to
Flags: needinfo?(dburns)
Flags: needinfo?(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.
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.
Depends on: 1507782

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.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: -- → P1

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

This is not blocked by bug 1507782.

No longer depends on: 1507782
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/bec3fe861b06 [geckodriver] Relax deserialization of timeouts parameters. r=ato

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

Flags: needinfo?(hskupin)

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.

Flags: needinfo?(hskupin)
Pushed by hskupin@mozilla.com: 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

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

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15101 for changes under testing/web-platform/tests
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: