1793845 - Intermittent /web-platform/tests/webdriver/tests/get_element_property/get.py | test_primitives, test_primitives_set_by_execute_script - AssertionError: assert 404 == 400

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P5)

Description

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

The test is currently marked as failing.

 0:15.46 pid:61192 1665000533458	Marionette	DEBUG	0 -> [0,473,"WebDriver:GetElementProperty",{"id":"4d340591-0066-4089-887d-f627f37c6dde","name":"__window"}]
 0:15.46 pid:61192 1665000533459	Marionette	DEBUG	0 <- [1,473,{"error":"javascript error","message":"Cyclic object value","stacktrace":"RemoteError@chrome://remote/content/shared/RemoteError.sys.mjs:8:8\nWebDriverError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:182:5\nJavaScriptError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:361:5\nevaluate.assertAcyclic@chrome://remote/content/marionette/evaluate.sys.mjs:53:11\nevaluate.toJSON@chrome://remote/content/marionette/evaluate.sys.mjs:366:14\nreceiveMessage@chrome://remote/content/marionette/actors/MarionetteCommandsChild.sys.mjs:172:36\n"},null]
 0:15.46 pid:61192 1665000533459	webdriver::server	DEBUG	<- 500 Internal Server Error {"value":{"error":"javascript error","message":"Cyclic object value","stacktrace":"RemoteError@chrome://remote/content/shared/RemoteError.sys.mjs:8:8\nWebDriverError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:182:5\nJavaScriptError@chrome://remote/content/shared/webdriver/Errors.sys.mjs:361:5\nevaluate.assertAcyclic@chrome://remote/content/marionette/evaluate.sys.mjs:53:11\nevaluate.toJSON@chrome://remote/content/marionette/evaluate.sys.mjs:366:14\nreceiveMessage@chrome://remote/content/marionette/actors/MarionetteCommandsChild.sys.mjs:172:36\n"}}

Comment 1

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

These tests are failing because Marionette doesn't wrap the value to be returned into a value field:

number: 0:15.54 pid:88036 1665063903466 Marionette DEBUG 0 <- [1,25,null,{"value":42}]
array: 0:15.57 pid:88036 1665063903498 Marionette DEBUG 0 <- [1,44,null,[]]
object: 0:15.64 pid:88036 1665063903566 Marionette DEBUG 0 <- [1,63,null,{}]

Not sure if that is on purpose or an oversight. I'll have to check the WebDriver classic specification.

Comment 2

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

Here is what the spec contains: https://w3c.github.io/webdriver/#dfn-send-a-response

Let response’s body be the UTF-8 encoded JSON serialization of a JSON Object with a key "value" set to data. 

As such we should wrap everything in value. But that doesn't work given that geckodriver doesn't expect the new session response to be wrapped in value. Changing this might require a new geckodriver release that would support both type of response payloads and that for at least one release. Once we can be sure that users have updated we could make a change like the above to not cause backward incompatibility issues.

James, should we still find a solution for that even it takes quite some efforts? I mean our implementation works that way for a long time and we basically didn't get complaints about this specific issue. So maybe we should ignore and continue with BiDi only. At least that would be my preferred solution.

Comment 3

[James Graham [:jgraham]]

What marionette returns doesn't matter, as long as geckodriver is always converting it to the spec format. I assume if it wasn't we'd have heard long ago.

Looking at the failures in get.py, the ones that try to return a Window are expected, given that we don't support serializing windows yet.

The other failures are trying to return Object properties set via execute_script. That bug is due to the fact that we're always running in a sandbox rather than directly in content, so I think this is effectively https://bugzilla.mozilla.org/show_bug.cgi?id=1743788. The correct solution might be to move classic WebDriver to the BiDi script module once it's reached feature parity.

Comment 4

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

The tests in test_primitives don't use Execute Script but Get Element Property and given my patch from bug 1398792 it should also work for arrays and objects. The problem is that in such cases we do not wrap the response from Marionette into a value, so geckodriver fails to actually read it.

https://searchfox.org/mozilla-central/rev/d416a7b827c2d76b12848e355a4e03dde49ece25/testing/geckodriver/src/marionette.rs#1080-1091

But because of this code in server.sys.mjs we have different behavior between array/object and basic types. So we might need a tri-state handling in geckodriver. Means if value is present use it, and if not use the result.

The other option is to always wrap the result in a value field but that causes failures in eg. NewSession with capabilities. But maybe that would be easier to fix.

Comment 5

[James Graham [:jgraham]]

I see. Yeah, so that marionette code trying to infer how to wrap the return type based on the type of the value seems pretty broken. Wouldn't it make more sense to just ensure that we always return {value: foo} from the individual commands where we want the marionette message to have that form, rather than doing it some cases but not in others, and trying to fix it up post-hoc? Of if we want to wrap for most commands but not all, then we could have an explicit list of the commands with different behaviour, rather than trying to inspect the value at runtime.

Comment 6

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

Sure but it's a complicated task when we want to maintain backward compatibility. First we would have to make a change to geckodriver to handle both situations. The one that we have right now, and the case when we always wrap inside a value field. That should probably stabilize over 1 or 2 geckodriver releases. Then we could make the change on mozilla-central and let it ride the trains. At a later point when Firefox 102ESR is no longer supported (until then we need to support both situations) we could remove the actual behavior.

If you think that this is fine we could get started and I can file a new geckodriver bug for step 1.

Comment 7

[James Graham [:jgraham]]

I don't think I'm suggesting anything that breaks compatibility? I'm suggesting that we try removing the fixup codepath, and update the marionette internal callees to always return data in the form that we want to send over the wire. We could just proceed by adding an error here where we currently do the fixup, and then on try see which commands are hitting the error path, and change those.

Comment 8

[BMO Automation]

Moving bug to Remote Protocol::Marionette component per bug 1815831.

Comment 9

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

Fixed by the patch on bug 1819029.