1819029 - Wrap response within a "value" field based on WebDriver commands and not the result's data type

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

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

As seen on bug 1793845 comment 2 and following we are sending a response in Marionette based on the returned value. This is just broken and need to be fixed.

We could update individual methods that have to use value and return the correct structure already there, or we would need an exclusion list in server.sys.mjs so that we do not wrap all results in a value field.

Commands that return a non-value keyed result are:

• Marionette:Quit
• WebDriver:CloseChromeWindow
• WebDriver:CloseWindow
• WebDriver:GetCookies
• WebDriver:GetTimeouts
• WebDriver:GetElementRect
• WebDriver:GetWindowHandles
• WebDriver:GetWindowRect
• WebDriver:FindElements
• WebDriver:NewSession
• WebDriver:GetWindowRect

All of those would violate the WebDriver specification so geckodriver fixes it up. As such changing the response to make them all spec compliant will be a breaking change for geckodriver and in some cases for Marionette client as well when used outside of mozilla-central (eg. wpt runner).

As such I would propose to exclude these commands from using a value key in server.sys.mjs.

Comment 1

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

I pushed a try build for the above list:
https://treeherder.mozilla.org/jobs?repo=try&revision=e04b83eccbc2737765843baa879566f17942020b

As it looks like there are other WebDriver commands as well, where we do not see the proper response returned (eg. print) where geckodriver fails only because the commands are not implemented in Marionette client.

Nevertheless I think that the exclusion list is better. So lets discuss.

Comment 2

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

I was wrong with the print command. It actually returns a result wrapped in value itself. Removing that will make all the related print tests passing.

Comment 3

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

Follow-up try build that will hopefully pass now even for reftests:
https://treeherder.mozilla.org/jobs?repo=try&revision=f439e2cf2e55c1aa9f07e80bbb7ee467f94e816a

Comment 4

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

Attachment: Bug 1819029 - [marionette] Fix returning response wrapped in "value" field.

Comment 5

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

We haven't had the time to discuss this bug in the triage session, and I think it's not really needed anymore given that a patch has already been uploaded.

Comment 6

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

Attachment: Bug 1819029 - [wdspec] Enhance tests for execute (async) script for primitive values.

Depends on D171084

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f71d8eaaac64
[marionette] Fix returning response wrapped in "value" field. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/f4cffe8e9381
[wdspec] Enhance tests for execute (async) script for primitive values. r=webdriver-reviewers,jdescottes

Comment 8

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

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

Comment 9

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/f71d8eaaac64
https://hg.mozilla.org/mozilla-central/rev/f4cffe8e9381

Comment 10

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

Upstream PR merged by moz-wptsync-bot