1336445 - If switch_to_window is called with a chrome handle, the first tab of that window gets selected

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

We have some broken behavior in our switchToWindow() implementation which causes us to select the first tab when actually switching to the chrome window handle. This is because we also take all the content window handles into account, and return the index=0 for the first match.

We should only check the content windows if the handle is not for a chrome window.

Patch and test is upcoming.

This blocks bug 1322383 from landing, because it found this issue.

Comment 1

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

Attachment: Bug 1336445 - Don't select the first tab if switch_to_window() is called with a chrome window handle.

Using switch_to_window() with a chrome window handle should not change the selected tab within
a browser window. It means we first have to check if it is a chrome handle, and only if it's not,
we should care about content window handles.

Review commit: https://reviewboard.mozilla.org/r/109584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109584/

Comment 2

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8833325 [details]
Bug 1336445 - Don't select the first tab if switch_to_window() is called with a chrome window handle.

https://reviewboard.mozilla.org/r/109584/#review110662

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/autoland/rev/e9bc1d6fadb4a61c469e3d8d9a0afc8eda7c92f0

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=74406761&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/711aa14e4c4cfe5a39fd3c06ac67c4bb31573375

Comment 5

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

I had a look and totally my fault! When I see this I really wonder why no other Mn test actually noticed that problem! What we miss here is to initialize the first content browser when opening a new chrome window. Because we look for tabIndex, which will no longer be set by moving those code lines above the other block. 

I have a working solution locally but will add another unit test.

Comment 6

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

The code path you actually have to take to reproduce this problem is opening a new private browsing window. If you don't do that it's not reproducible. At least I can setup a proper unit test for marionette now.

Comment 7

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

Comment on attachment 8833325 [details]
Bug 1336445 - Don't select the first tab if switch_to_window() is called with a chrome window handle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109584/diff/1-2/

Comment 8

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

Comment on attachment 8833325 [details]
Bug 1336445 - Don't select the first tab if switch_to_window() is called with a chrome window handle.

Try shows bustage somewhere in session creation for fx-ui-tests.

Comment 9

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

Comment on attachment 8833325 [details]
Bug 1336445 - Don't select the first tab if switch_to_window() is called with a chrome window handle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109584/diff/2-3/

Comment 10

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

Comment on attachment 8833325 [details]
Bug 1336445 - Don't select the first tab if switch_to_window() is called with a chrome window handle.

Silly mistake I made in the last revision. The property hasTabBrowser is always present for non-browser windows. So a check for `"hasTabBrowser" in found` always results in trying to register browsers, which caused an obvious hang.

Comment 11

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

Comment on attachment 8833325 [details]
Bug 1336445 - Don't select the first tab if switch_to_window() is called with a chrome window handle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109584/diff/3-4/

Comment 12

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

Comment on attachment 8833325 [details]
Bug 1336445 - Don't select the first tab if switch_to_window() is called with a chrome window handle.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/109584/diff/4-5/

Comment 13

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

Last update contains a small fix for a race condition I noticed in the try builds on Linux, where we do not wait long enough for the body element to appear. It's better to exchange it with a call to execute_script() which indeed has the same effect.

Comment 14

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8833325 [details]
Bug 1336445 - Don't select the first tab if switch_to_window() is called with a chrome window handle.

https://reviewboard.mozilla.org/r/109584/#review111100

Comment 15

[Andreas Tolfsen ❲:ato❳]

The latest revision looks excellent.  It would be good if we could uplift this to the relevant channels, if possible.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f08c78b5f19b
Don't select the first tab if switch_to_window() is called with a chrome window handle. r=ato

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f08c78b5f19b

Comment 18

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

test-only changes we would like to see uplifted for aurora and beta. Thanks.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4116e3e12460

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/335f8a267be8