1301073 - WebDriver:SwitchToWindow should not allow switching by window name

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P3)

Description

[Mark Banner (:standard8)]

The w3c specification for WebDriver [1] doesn't allow for passing a window "name" as a handle to switchToWindow - only an id.

Currently the code allows for a "name" or an "id", however the implementation of "name" is wrong as it uses the window name of the actual window rather than the window.name of the content process within a tab (xref bug 1283941).

Seeing as it doesn't need to be supported, we should just remove the name option to save confusion.

[1] https://w3c.github.io/webdriver/webdriver-spec.html#switch-to-window

Comment 1

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

The necessary code change this bug covers are:

JS: https://searchfox.org/mozilla-central/rev/5e830ac8f56fe191cb58a264e01cdbf6b6e847bd/testing/marionette/driver.js#1542-1577
Py: https://searchfox.org/mozilla-central/rev/5e830ac8f56fe191cb58a264e01cdbf6b6e847bd/testing/marionette/client/marionette_driver/marionette.py#1346-1357
Rust: https://searchfox.org/mozilla-central/rev/5e830ac8f56fe191cb58a264e01cdbf6b6e847bd/testing/geckodriver/src/marionette.rs#1667-1674

Note that for geckodriver the handle argument has been added via bug 1519053 to geckodriver 0.25.0. So if we make the change to driver.js now, it would mean that everything before 0.25.0 (which only supports name) will not work with Firefox 71 and later anymore. But geckodriver 0.25.0 and later which support both name and handle will not require us to bump the minimum supported version of Firefox.

Another solution would be to add the support for handle in driver.js first, and get this released. And then we follow-up later (when we have the time for) with the name removal.

Given the actual problems with 0.25.0 with MSVCRT we may want to do the interim solution and give people more time in case they want/have to stay with geckodriver 0.24.0 for now?

Personally I would prefer the latter option. Maja and Andreas, what do you think?

Comment 2

[Andreas Tolfsen ❲:ato❳]

If switching by name breaks backwards compatibility I would prefer
not removing this feature.

Comment 3

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

Ok, so in that case I will file a bug for Marionette which simply adds using the handle if specified for now. Then after a couple of Firefox releases (maybe after the next ESR) we can work on this bug, and get it removed.

Comment 4

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

As filed this is more a geckodriver related bug. So I'm going to move it into the correct component.

Note that with bug 1588424 fixed we no longer accept name as argument for WebDriver:SwitchToWindow in Marionette. The remaining patch to do will be to finally remove passing in the name by geckodriver:

https://searchfox.org/mozilla-central/rev/82c04b9cad5b98bdf682bd477f2b1e3071b004ad/testing/geckodriver/src/marionette.rs#1720-1723

By doing that it would move the minimal supported version of Firefox to 67.0 for the geckodriver release including this change. We can easily do that for the 0.28 release.

Comment 5

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

Turns out that we actually didn't swap the Marionette code from name to handle earlier on, and my fix on bug 1588424 made the hard switch. It means that the minimum version of Firefox as supported will not be 67.0 but 80.0.

That means that we will still have to wait a while (at least until 78.0esr is no longer supported) until we can get it removed in geckodriver. Nevertheless I will attach a WIP patch.

Comment 6

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

Attachment: Bug 1301073 - [geckodriver] Don't send "name" argument for "WebDriver:SwitchToWindow" to Marionette.

Starting with geckodriver 0.25.0 the "handle" argument (available
since Firefox 67) for the "WebDriver:SwitchToWindow" call to
Marionette has been added. Since then the "name" argument in
Marionette has been removed (Firefox 80).

That means sending "name" is no longer necessary, and can be
removed. By doing that the minimum supported Firefox version
will be bumped to Firefox 80.0.

Comment 7

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

We should at least wait with landing this patch until the next ESR will be released, and that will happen with 91.0 by mid of this year. Otherwise we would break users running ESR78.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/089fc7a7d0dd
[geckodriver] Don't send "name" argument for "WebDriver:SwitchToWindow" to Marionette. r=webdriver-reviewers,jgraham

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/089fc7a7d0dd