Closed Bug 1507782 Opened 2 years ago Closed 1 year ago

Accept unknown JSON fields to WebDriver:SetTimeouts


(Testing :: Marionette, enhancement, P2)



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

Tracking Status
firefox-esr60 --- wontfix
firefox65 --- wontfix
firefox66 --- fixed


(Reporter: ato, Unassigned)



(2 obsolete files)

In addition to relaxing serde JSON deserialisation behavoiur (tracked
in we need
to make Marionette’s WebDriver:SetTimeouts command accept unknown
JSON fields.

As mentioned in,
a specification change is also required for this change.
Can you please explain why we need a separate bug for that? I thought that all can be done on bug 1507283.

Should we better make bug 1507283 a meta bug?
Flags: needinfo?(ato)
See my comment here:

We have a literal implementation of the Set Timeouts deserialisation
description, erroring on unknown JSON fields, implemented in JS in

That problem is orthogonal from the way I interpret, which is to
do with relaxing serde deserialisation in geckodriver.
Flags: needinfo?(ato)

Ok, so to wrap this up... You mean that we would have to change the following substep for deserialize as a timeout of the spec to just ignore those keys and not throwing an invalid argument error?

  1. Find the timeout type from the table of session timeouts by looking it up by its keyword key.
    If no keyword matches key, return error with error code invalid argument.

If that is the case it will be a simple change, and I would like to see this uplifted to beta, and maybe even esr60.

I will mark bug 1507283 as being blocked by that change, so that I can verify the correct behavior of Firefox when making the geckodriver changes.

Blocks: 1507283
Flags: needinfo?(ato)

Yes, the specification here is wrong too. But fixing just substep
three would be poor form. The whole algorithm can just be changed
to access the different timeout types directly. There’s no need
to iterate the object by own properties.

I also note with interest that substep 4 is wrong, because the
script timeout can be null:

If value is not an integer, or it is less than 0 or greater than
the maximum safe integer, return error with error code invalid

Looks like there’s a bit of spec work to get out of the way first.

Flags: needinfo?(ato)

I rewrote the specification back in January, but forgot to post about it here:

Andreas, did you also land some wdspec tests for those changes yet? I assume it will be a single test only for an unknown key?

Flags: needinfo?(ato)

Actually, I think you did to a large degree in

Judging by
we’re still missing one test for an unknown key, as you said.

For the sake of clarity I’m resolving this and will follow up with
a new test in

Closed: 1 year ago
Flags: needinfo?(ato)
Resolution: --- → FIXED

WebDriver is meant to ignore unfamiliar fields in JSON Objects generally,
and whilst we are testing that an empty object, {}, does not mutate
the timeout duration values, we are not testing unknown field keys.

Fixes a lint problem.

This has actually ben fixed by bug 1507283.

Duplicate of bug: 1507283

Comment on attachment 9069342 [details]
bug 1507782: webdriver: add test for ignored fields to Set Timeouts;

Uggh, attached these to the wrong bug.

Attachment #9069342 - Attachment is obsolete: true
Attachment #9069343 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.