Closed Bug 1461463 Opened 6 years ago Closed 6 years ago

WebDriver commands which should return data "null" return an empty dictionary

Categories

(Remote Protocol :: Marionette, enhancement, P1)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Bug 1461463 - [marionette] Empty response value should be null and not {}.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1461463 - [geckodriver] Remove extra colon from invalid "WebDriver::Forward" command.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1461463 - [geckodriver] Empty response value should be null and not {}.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1461463 - [wdspec] Add timeout reset finalizer to session fixture.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1461463 - [wdspec] Add tests for commands returning null as response value.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1461463 - [marionette] Add validation for known contexts.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1461463 - [wdclient] "message" argument of WebDriverException has to be optional.
59 bytes, text/x-review-board-request
ato
: review+
Details 
As lately seen while fixing some wdspec tests:

> [task 2018-05-14T19:38:30.936Z] 19:38:30     INFO -  1526326710926	Marionette	TRACE	151 -> [0,6,"WebDriver:AddCookie",{"cookie":{"path":"/","name":"foo","value":"bar","secure":false}}]
> [task 2018-05-14T19:38:30.936Z] 19:38:30     INFO -  1526326710928	Marionette	TRACE	151 <- [1,6,null,{}]

The expected return value here is `null`, but an empty dictionary is returned.

Here the spec entry: https://w3c.github.io/webdriver/#add-cookie

> 8. Return success with data null. 

This applies to all commands which return `null` as value.
But this is not the return value from geckodriver, it is the return
value from Marionette.  Marionette does not match WebDriver 1:1 and
I suspect you might find that geckodriver returns {"value": null}.
That's why I filed it in the Marionette component, and it may also need a patch for geckodriver if it doesn't pass-through the value 1:1. Here the log from a wdspec test job:

> [task 2018-05-15T05:48:15.015Z] 05:48:15     INFO - PID 1023 | 1526363295003	Marionette	TRACE	0 -> [0,2541,"WebDriver:AddCookie",{"cookie":{"domain":"web-platform.test","httpOnly":false,"name":"hello","path":"/","secure":false,"value":"world"}}]
> [task 2018-05-15T05:48:15.017Z] 05:48:15     INFO - PID 1023 | 1526363295005	Marionette	TRACE	0 <- [1,2541,null,{}]
> [task 2018-05-15T05:48:15.018Z] 05:48:15     INFO - PID 1023 | 1526363295006	webdriver::server	DEBUG	<- 200 OK {"value": {}}

Btw I talked with Jim yesterday and the IEDriver returns `null` in those cases.
Thanks for including the logs from webdriver::server, that was
useful.

Marionette returns {} (empty object) which geckodriver then wraps
in {"value": <response body>}, so it looks like the value is just
being passed through.  Unless there is an edge case I don’t fully
understand, I don’t see any particular value in changing the
Marionette data format, but there’s probably a case to be made that
it would be easier to change the return body to null so that
geckodriver wouldn’t have to do any data massaging.
The problem originates from:
https://dxr.mozilla.org/mozilla-central/rev/a382f8feaba41f1cb1cee718f8815cd672c10f3c/testing/marionette/message.js#239

> const ResponseBody = () => new Proxy({}, validator);

The call to `Proxy` returns an object without any keys set, and if commands do not set a value we will just send this empty object over the socket.

What we should do to send `null` is to check the body in `toPacket()` for contained keys. If there are none we should just send `null`.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P3 → P1
The change as proposed above works fine with Marionette and by running a couple of test modules I cannot find any failure. So it shouldn't be a backward incompatible change.

Also I checked the fix with wdspec and geckodriver, and as it looks like we already do some massaging of the response value:

> 0:19.55 pid:18803 1526388612895	Marionette	TRACE	0 -> [0,127,"WebDriver:SwitchToWindow",{"name":"8589934593"}]
> 0:19.55 pid:18803 1526388612899	Marionette	TRACE	0 <- [1,127,null,null]
> 0:19.56 pid:18803 1526388612899	webdriver::server	DEBUG	<- 200 OK {"value": {}}

So we clearly also need a fix in geckodriver, which can handle both response formats from Marionette.
The convertion from None to `{}` in webdriver rust happens here:
https://dxr.mozilla.org/mozilla-central/rev/a382f8feaba41f1cb1cee718f8815cd672c10f3c/testing/webdriver/src/response.rs#25-36

I cannot figure out how to best solve this given that obj is expected to be a String, and None isn't nor have a string representation. Would we have to re-write this whole method?

Andreas, maybe you could give an advise? Thanks.
Flags: needinfo?(ato)
This is a rather stupid serialiser.  Just s/{}/null/g should work.
Flags: needinfo?(ato)
Oh, that easy! It works, and I will update the wdspec tests with additional assertions to check that those commands which should not return a value really return `null`.
Hm, testing for `None` in `assert_success` doesn't work:
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/webdriver/tests/support/asserts.py#85-86

So unless we won't require an always assert for the value situation, we might have to assert separately by the return value in the test. Andreas, do you have another idea?
Flags: needinfo?(ato)
Flags: needinfo?(ato)
Attachment #8976537 - Flags: review?(ato)
Attachment #8976538 - Flags: review?(ato)
Attachment #8976539 - Flags: review?(ato)
Attachment #8976540 - Flags: review?(ato)
Attachment #8976541 - Flags: review?(ato)
Comment on attachment 8976537 [details]
Bug 1461463 - [marionette] Empty response value should be null and not {}.

https://reviewboard.mozilla.org/r/244646/#review250838

::: testing/marionette/driver.js:1193
(Diff revision 2)
>          "Marionette:waitForPageLoaded", parameters);
>    });
>  
>    await goBack;
> +
> +  return null;

Any idea why eslint complains about this addition here with https://eslint.org/docs/rules/no-return-await?
Comment on attachment 8976537 [details]
Bug 1461463 - [marionette] Empty response value should be null and not {}.

https://reviewboard.mozilla.org/r/244646/#review250974

The return value from a function which does not specify one is
undefined, and the this would normally get serialised to null.  For
this reason I don’t think it’s the right solution to explicitly add
"return null" to every command, but to rather fix the underlying
problem which is the _default response body value_.

You will find the details in testing/marionette/message.js’s Response
class.

::: commit-message-87cc1:3
(Diff revision 2)
> +Bug 1461463 - [marionette] Empty response value should be "null" and not "{}".
> +
> +WebDriver commands which do not return a value have to send "null'.

s/"//
s/'//

It’s a null type, not a string.

::: commit-message-87cc1:5
(Diff revision 2)
> +the spec we should make that change even it wouldn't be necessary
> +due to geckodriver translates it again.

This last part I don’t understand.
Attachment #8976537 - Flags: review?(ato) → review-
Comment on attachment 8976538 [details]
Bug 1461463 - [geckodriver] Remove extra colon from invalid "WebDriver::Forward" command.

https://reviewboard.mozilla.org/r/244648/#review250980

::: commit-message-aacd5:1
(Diff revision 2)
> +Bug 1461463 - [geckodriver] Fix WebDriver:Forward command.

This commit message is not adequate.
Attachment #8976538 - Flags: review?(ato) → review-
Comment on attachment 8976539 [details]
Bug 1461463 - [geckodriver] Empty response value should be null and not {}.

https://reviewboard.mozilla.org/r/244650/#review250982

::: commit-message-de840:1
(Diff revision 2)
> +Bug 1461463 - [geckodriver] Empty response value should be "null" and not "{}".

These are not strings but types, please remove quote marks.

::: commit-message-de840:3
(Diff revision 2)
> +Bug 1461463 - [geckodriver] Empty response value should be "null" and not "{}".
> +
> +WebDriver commands which do not return a value have to send "null'.

Remove quote marks.
Attachment #8976539 - Flags: review?(ato) → review-
Comment on attachment 8976540 [details]
Bug 1461463 - [wdspec] Add timeout reset finalizer to session fixture.

https://reviewboard.mozilla.org/r/244652/#review250984

::: testing/web-platform/tests/webdriver/tests/support/fixtures.py:178
(Diff revision 2)
>          if not _current_session.session_id:
>              raise
>  
>      # finalisers are popped off a stack,
>      # making their ordering reverse
> +    request.addfinalizer(lambda: _restore_timeouts(_current_session))

If the ordering is reverse, I would suggest setting this first since
some of the other finalisers are interacting with WebDriver in such
a way that they might be influenced by the timeout duration values.
Attachment #8976540 - Flags: review?(ato) → review+
Comment on attachment 8976541 [details]
Bug 1461463 - [wdspec] Add tests for commands returning null as response value.

https://reviewboard.mozilla.org/r/244654/#review250986

This is an excellent patch.  Thanks for doing this due diligence.

::: commit-message-8a3f2:1
(Diff revision 2)
> +Bug 1461463 - [wdspec] Add tests for commands returning "null" as response value.

null is a type, not a string.  Remove quote marks.

::: testing/web-platform/tests/webdriver/tests/accept_alert/accept.py:14
(Diff revision 2)
> +    value = assert_success(response)
> +    assert value is None

Throughout this would be equivalent to assert_success(response,
None), but this is obviously fine.

::: testing/web-platform/tests/webdriver/tests/actions/key.py:7
(Diff revision 2)
> +def test_null_response_value(session, key_chain):
> +    value = key_chain.key_up("a").perform()
> +    assert value is None
> +
> +    value = session.actions.release()
> +    assert value is None

I don’t know if there’s a particular value in testing mouse/key
chains separately since these are essentially the same endpoints?

::: testing/web-platform/tests/webdriver/tests/back/back.py:11
(Diff revision 2)
> +    return session.transport.send(
> +        "POST", "session/{session_id}/back".format(**vars(session)))
> +
> +
> +def test_null_response_value(session):
> +    session.url = inline("<div/>")

BTW this doesn’t actually close the element AFAIK, but obviously
not an issue.

::: testing/web-platform/tests/webdriver/tests/delete_cookie/delete.py:12
(Diff revision 2)
>              session_id=session.session_id,
>              name=name))
>  
>  
> +def test_null_response_value(session, url):
> +    response = delete_cookie(session, "foo")

Does the cookie already exist somehow, or does it always return
success?
Attachment #8976541 - Flags: review?(ato) → review+
Comment on attachment 8976538 [details]
Bug 1461463 - [geckodriver] Remove extra colon from invalid "WebDriver::Forward" command.

https://reviewboard.mozilla.org/r/244648/#review250988
Attachment #8976538 - Flags: review- → review+
Comment on attachment 8976537 [details]
Bug 1461463 - [marionette] Empty response value should be null and not {}.

https://reviewboard.mozilla.org/r/244646/#review250974

> This last part I don’t understand.

Maybe I should drop? The Marionette socket protocol doesn't have to obey the WebDriver specification, and as such it might not be necessary to return null. But it helps geckodriver to more generally return null for those commands which require it. Otherwise we would have to add code for each Response type in webdriver which convers "{}" to null.
Comment on attachment 8976540 [details]
Bug 1461463 - [wdspec] Add timeout reset finalizer to session fixture.

https://reviewboard.mozilla.org/r/244652/#review250984

> If the ordering is reverse, I would suggest setting this first since
> some of the other finalisers are interacting with WebDriver in such
> a way that they might be influenced by the timeout duration values.

Good idea. Will do it.
I discussed with Andreas how we could improve the situation in Marionette with handling the return value. We identified a couple of solutions which I will test later today if those work. I will comment then in more detail. For now I removed the review request from this particular commit.
Comment on attachment 8976539 [details]
Bug 1461463 - [geckodriver] Empty response value should be null and not {}.

https://reviewboard.mozilla.org/r/244650/#review251042
Attachment #8976539 - Flags: review?(ato) → review+
Comment on attachment 8976537 [details]
Bug 1461463 - [marionette] Empty response value should be null and not {}.

https://reviewboard.mozilla.org/r/244646/#review250974

> Maybe I should drop? The Marionette socket protocol doesn't have to obey the WebDriver specification, and as such it might not be necessary to return null. But it helps geckodriver to more generally return null for those commands which require it. Otherwise we would have to add code for each Response type in webdriver which convers "{}" to null.

Indeed, I agree with what you just said here.
Comment on attachment 8976537 [details]
Bug 1461463 - [marionette] Empty response value should be null and not {}.

https://reviewboard.mozilla.org/r/244646/#review250974

> Indeed, I agree with what you just said here.

So does it mean I have to update that part of the commit message, or drop it?
Comment on attachment 8976537 [details]
Bug 1461463 - [marionette] Empty response value should be null and not {}.

https://reviewboard.mozilla.org/r/244646/#review250974

> So does it mean I have to update that part of the commit message, or drop it?

Dropping it makes sense, since you say it doesn’t have a direct
relation to WebDriver.
Comment on attachment 8980339 [details]
Bug 1461463 - [marionette] Add validation for known contexts.

https://reviewboard.mozilla.org/r/246492/#review252880

Whilst I don’t disagree that having a setter for this.context is
probably somewhat safer that the status quo, you appear to have
misunderstood what the Context type is in the code in the setter.

Context.Content and Context.Chrome are both primitive string types,
which means the setter is far more complicated than it needs to be.
The "context in [Context.Content, Context.Chrome]" check will always
pass (given valid input) and the type check on :173 will never be
called.  :176 also duplicates code in Context.fromString.
Attachment #8980339 - Flags: review?(ato) → review-
Comment on attachment 8980340 [details]
Bug 1461463 - [wdclient] "message" argument of WebDriverException has to be optional.

https://reviewboard.mozilla.org/r/246494/#review252884

::: commit-message-59d5b:1
(Diff revision 4)
> +Bug 1461463 - [wdclient] Raising SessionNotCreatedException requires message.

I’m pretty sure it doesn’t.  What problem are you trying to fix
here?
Attachment #8980340 - Flags: review?(ato) → review-
Comment on attachment 8976537 [details]
Bug 1461463 - [marionette] Empty response value should be null and not {}.

https://reviewboard.mozilla.org/r/244646/#review252888
Attachment #8976537 - Flags: review?(ato) → review+
Comment on attachment 8980340 [details]
Bug 1461463 - [wdclient] "message" argument of WebDriverException has to be optional.

https://reviewboard.mozilla.org/r/246494/#review252884

> I’m pretty sure it doesn’t.  What problem are you trying to fix
> here?

It needs a message otherwise we see an exception in this line.

The message is not optional for WebDriverException which SessionNotCreatedException is based on:
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/error.py#112
https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/error.py#9

You can replicate that when ending the session, and calling a session command like we do in this patch series for restoring the timeouts.
Comment on attachment 8980339 [details]
Bug 1461463 - [marionette] Add validation for known contexts.

https://reviewboard.mozilla.org/r/246492/#review252880

So I should better remove that commit then?
Comment on attachment 8976537 [details]
Bug 1461463 - [marionette] Empty response value should be null and not {}.

Please review the latest changes again.
Attachment #8976537 - Flags: review?(ato)
Comment on attachment 8976537 [details]
Bug 1461463 - [marionette] Empty response value should be null and not {}.

Sorry, looks like there was an overlap. All fine.
Attachment #8976537 - Flags: review?(ato)
Comment on attachment 8980340 [details]
Bug 1461463 - [wdclient] "message" argument of WebDriverException has to be optional.

https://reviewboard.mozilla.org/r/246494/#review252884

> It needs a message otherwise we see an exception in this line.
> 
> The message is not optional for WebDriverException which SessionNotCreatedException is based on:
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/error.py#112
> https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/tools/webdriver/webdriver/error.py#9
> 
> You can replicate that when ending the session, and calling a session command like we do in this patch series for restoring the timeouts.

Ugh.  It feels like the right thing to do is to make message=None
so that we don’t have to pass an argument, because "No active
session" is basically duplicating the name of the exception type.
Comment on attachment 8980339 [details]
Bug 1461463 - [marionette] Add validation for known contexts.

https://reviewboard.mozilla.org/r/246492/#review252880

As I said, I think it’s a good idea to have a setter for it that
checks the input is correct every time it is called.  It makes the
code marginally safer.
Comment on attachment 8980340 [details]
Bug 1461463 - [wdclient] "message" argument of WebDriverException has to be optional.

https://reviewboard.mozilla.org/r/246494/#review252884

> Ugh.  It feels like the right thing to do is to make message=None
> so that we don’t have to pass an argument, because "No active
> session" is basically duplicating the name of the exception type.

This change will be part of my next update.
Comment on attachment 8980339 [details]
Bug 1461463 - [marionette] Add validation for known contexts.

https://reviewboard.mozilla.org/r/246492/#review253572
Attachment #8980339 - Flags: review?(ato) → review+
Comment on attachment 8980340 [details]
Bug 1461463 - [wdclient] "message" argument of WebDriverException has to be optional.

https://reviewboard.mozilla.org/r/246494/#review253574

::: testing/web-platform/tests/tools/webdriver/webdriver/error.py:20
(Diff revision 5)
> +        if self.message is not None:
> +            message += ": %s\n" % self.message
> +        else:
> +            message += "\n"

Looks like the new line must be added regardless, so the else-clause
is not necessary.
Attachment #8980340 - Flags: review?(ato) → review+
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5820f713fe9a
[marionette] Add validation for known contexts. r=ato
https://hg.mozilla.org/integration/autoland/rev/2df57c387242
[marionette] Empty response value should be null and not {}. r=ato
https://hg.mozilla.org/integration/autoland/rev/1c4a5bba3582
[geckodriver] Remove extra colon from invalid "WebDriver::Forward" command. r=ato
https://hg.mozilla.org/integration/autoland/rev/9d1398fa66ba
[geckodriver] Empty response value should be null and not {}. r=ato
https://hg.mozilla.org/integration/autoland/rev/abd4d105082a
[wdclient] "message" argument of WebDriverException has to be optional. r=ato
https://hg.mozilla.org/integration/autoland/rev/f4261bd006df
[wdspec] Add timeout reset finalizer to session fixture. r=ato
https://hg.mozilla.org/integration/autoland/rev/fe8aa70475dd
[wdspec] Add tests for commands returning null as response value. r=ato
No longer depends on: 1465283
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11953 for changes under testing/web-platform/tests
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: