Closed Bug 1504756 Opened 6 years ago Closed 6 years ago

Implement "WebDriver:NewWindow" command

Categories

(Remote Protocol :: Marionette, enhancement, P1)

Product:
Remote Protocol
Component:
Marionette
Type:
enhancement

Tracking

(firefox66 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: ato, Assigned: whimboo)

References

(Blocks 7 open bugs)

Details

Attachments

(7 files)

Bug 1504756 - [marionette] Add executeSoon() to run tasks on the main thread. r?ato
(deleted), text/x-phabricator-request
Details
Bug 1504756 - [marionette] Use waitForEvent() when waiting for events. r?ato
(deleted), text/x-phabricator-request
Details
Bug 1504756 - [marionette] Use waitForMessage() to wait for an expected message manager message. r?ato
(deleted), text/x-phabricator-request
Details
Bug 1504756 - [marionette] Use waitForObserverTopic when waiting for observer notifications. r?ato
(deleted), text/x-phabricator-request
Details
Bug 1504756 - [marionette] Added "WebDriver:NewWindow" command to open a new top-level browsing context. r?ato
(deleted), text/x-phabricator-request
Details
Bug 1504756 - [marionette] Added opening a new browsing context to Marionette client.
(deleted), text/x-phabricator-request
Details
Bug 1504756 - [marionette] Remove default timeout from PollPromise. r?ato
(deleted), text/x-phabricator-request
Details
There’s a proposal for adding a New Window command to WebDriver done by jgraham over in https://github.com/w3c/webdriver/pull/1324. We should attempt an implementation of this in Marionette and geckodriver.
Blocks: webdriver
Priority: -- → P2
Blocks: 1503274
I think that I will have a look at implementing this given that it would prevent a lot of intermittent failures from happening - at least for Marionette unit tests. See bug 1503274 for details.
Assignee: nobody → hskupin
Priority: P2 → P1
Note that the implementation for this bug will only cover the necessary work as needed to be done to be spec compliant. It does not cover chrome context and opening chrome windows. If that is needed we will have to get this implemented separately.
Status: NEW → ASSIGNED
We should rely on builtin helpers instead of rolling our own (ie window_manager.py currenly manually calls window.open() and just assumes it gets it a working browser window and that's... optimistic).
Depends on: 1506643
(In reply to :Gijs (he/him) from comment #3) > We should rely on builtin helpers For https://searchfox.org/mozilla-central/source/testing/marionette/harness/marionette_harness/runner/mixins/window_manager.py#102 or equivalent functionality, that'd be `OpenBrowserWindow()`.
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=1506608
No longer blocks: 1503274
Blocks: 1334095
Depends on: 1506796
(In reply to :Gijs (he/him) from comment #4) > For > https://searchfox.org/mozilla-central/source/testing/marionette/harness/ > marionette_harness/runner/mixins/window_manager.py#102 or equivalent > functionality, that'd be `OpenBrowserWindow()`. Great. So are there any other events and observer notifications I would have to wait for before returning the new window command? Given that we always open `about:blank` by default now, the load happens synchronous and no `load` event should have to be waited for.
Flags: needinfo?(gijskruitbosch+bugs)
Also I don't see a way to open the new window in the background. Given that we don't want to have it overlayed the currently selected window, I might have to call `lower()` or something adequate after opening the browser window to get this accomplished.
(In reply to Henrik Skupin (:whimboo) from comment #5) > (In reply to :Gijs (he/him) from comment #4) > > For > > https://searchfox.org/mozilla-central/source/testing/marionette/harness/ > > marionette_harness/runner/mixins/window_manager.py#102 or equivalent > > functionality, that'd be `OpenBrowserWindow()`. > > Great. So are there any other events and observer notifications I would have > to wait for before returning the new window command? Given that we always > open `about:blank` by default now, the load happens synchronous and no > `load` event should have to be waited for. You probably want to wait for the browser to load, and delayed-startup to fire. You may also want to wait for the window to be focused + activated (which is async on Linux, at least). Assuming that the load of about:blank is synchronous seems optimistic to me, but if you want you could avoid waiting, I guess. You could take a look at BrowserTestUtils.waitForNewWindow() to see some of what browser-chrome mochitests do when waiting for new browser windows.
Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1507276
Blocks: 1507771
Depends on: 1507803
Blocks: 1335085
Blocks: 1509513
Blocks: 1509677
I'm going to upload the current set of patches to get an early feedback. Note that a test still fails on Linux and needs investigation.
To be closer to other test harnesses which are using executeSoon() to run a task when the browser is idle, this patch adds the same method to Marionette's sync module.
The patch adds the end-point for the recently defined `Create window` command (https://github.com/w3c/webdriver/issues/1138). It allows to open a new top-level browsing context as tab or as window. Depends on D13662
WIP - A test still fails on Linux. The patch updates the Marionette client and all Marionette unit tests to make use of the new `Create Window` command as much as possible. Depends on D13663
The remaining test failure is strange, and I wonder if this is somewhat Linux window manager related? Here the failing part for test_switch_window_chrome.py TestSwitchWindowChrome.test_switch_tabs_for_new_background_window_without_focus_change https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=214387737&repo=try&lineNumber=22242-22248 As the lines show we open a new window, and we are waiting for `active`, `focus`, and `browser-delayed-startup-finished`. But then when we focus the original window again, it keeps staying in foreground even we receive additional `activate` and `focus` events. Gijs, did you ever notice this behavior? I wonder if we simply skip this test for now for Linux (MacOS and Windows work fine), and get it fixed as follow-up.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Henrik Skupin (:whimboo) from comment #16) > The remaining test failure is strange, and I wonder if this is somewhat > Linux window manager related? > > Here the failing part for test_switch_window_chrome.py > TestSwitchWindowChrome. > test_switch_tabs_for_new_background_window_without_focus_change > > https://treeherder.mozilla.org/logviewer.html#/ > jobs?job_id=214387737&repo=try&lineNumber=22242-22248 > > As the lines show we open a new window, and we are waiting for `active`, > `focus`, and `browser-delayed-startup-finished`. But then when we focus the > original window again, it keeps staying in foreground even we receive > additional `activate` and `focus` events. I don't understand. Why is that section of the log relevant? The failure says the wrong tab is selected. What does that have to do with the window focus behavior? How do you know which window is in the foreground?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(hskupin)
(In reply to :Gijs (he/him) from comment #17) > I don't understand. Why is that section of the log relevant? The failure > says the wrong tab is selected. What does that have to do with the window > focus behavior? How do you know which window is in the foreground? It's a tab in a different window which is wrongly selected, because the newly opened window is not moved to the background with an additional call to `original_window.focus()` as you have suggested. As such the new window stays in foreground, which I can see when using an interactive task on TC, or running the test in a Janitor container.
Flags: needinfo?(hskupin)
Ok, so the problem here seems to be related to the focus manager test mode. When `focus()` is called on the other window, the focus switches to it, but actually doesn't raise the window. As such `BrowserWindowTracker.getTopWindow()` will return the wrong window, which then results in the wrong tab index. While MacOS and Windows can handle that, Linux fails. This somewhat reminds me on bug 156333.
Blocks: 1511970
I have raised bug 1511970 to continue investigating this remaining test failure which only happens on Linux. Here the new try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=065c96ba4244750fa0ededceaca843146f01e4c8
There is a remaining failure on Android for a test which opens a private browsing tab. I missed to update the id of the button, which is actually different from Firefox. This try build should pass now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b3570a8b6e18923a80cea95a8fc122ee5fbdb6e
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/04da1f01afba [marionette] Add executeSoon() to run tasks on the main thread. r=ato https://hg.mozilla.org/integration/autoland/rev/c90e4b2e1b28 [marionette] Use waitForEvent() when waiting for events. r=ato https://hg.mozilla.org/integration/autoland/rev/ba627a1b61dc [marionette] Use waitForMessage() to wait for an expected message manager message. r=ato https://hg.mozilla.org/integration/autoland/rev/a8ea6d01fbe1 [marionette] Use waitForObserverTopic when waiting for observer notifications. r=ato https://hg.mozilla.org/integration/autoland/rev/30d345cce5be [marionette] Added "WebDriver:NewWindow" command to open a new top-level browsing context. r=ato https://hg.mozilla.org/integration/autoland/rev/d808b528532a [marionette] Added opening a new browsing context to Marionette client. r=ato
Backout by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/49bd1dd914a4 Backed out 6 changesets for wpt failures on webdriver/tests/execute_script/promise.py
With this patch nothing should have changed for webdriver/tests/execute_script/promise.py. I'm not sure sure why it doesn't timeout anymore in CI, and even locally it doesn't timeout. However, it looks like that I have to fix bug 1508700 first before I'm able to land this patch series again.
Depends on: 1508700
Flags: needinfo?(hskupin)
Blocks: 1508700
No longer depends on: 1508700
(In reply to Cosmin Sabou [:CosminS] from comment #23) > Backout link: > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > e4e9bf768be8ed3da736584a2b2ca7d178d026ad This is the wrong link. Actually it should have been: https://hg.mozilla.org/integration/autoland/rev/49bd1dd914a4a9d8c8b546dc503b6ae715d6e98c The reason why it works now can be seen at bug 1508700 comment 4. I will update the expectation data in the patch for waitForEvent revision.
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/65858c8c0fbd [marionette] Add executeSoon() to run tasks on the main thread. r=ato https://hg.mozilla.org/integration/autoland/rev/6068c233f4ef [marionette] Use waitForEvent() when waiting for events. r=ato https://hg.mozilla.org/integration/autoland/rev/f23b667d8bfa [marionette] Use waitForMessage() to wait for an expected message manager message. r=ato https://hg.mozilla.org/integration/autoland/rev/5c2826c58f9e [marionette] Use waitForObserverTopic when waiting for observer notifications. r=ato https://hg.mozilla.org/integration/autoland/rev/5c495fd7f64d [marionette] Added "WebDriver:NewWindow" command to open a new top-level browsing context. r=ato https://hg.mozilla.org/integration/autoland/rev/d7d78e79f0b3 [marionette] Added opening a new browsing context to Marionette client. r=ato
Depends on: 1512342
Depends on: 1513436
Depends on: 1514989
Depends on: 1514706
Depends on: 1512871
Depends on: 1514597
Backed out as requested by whimboo in order to stop some wpt and mn intermittents https://hg.mozilla.org/mozilla-central/rev/49be2c1eb4c8c64cadd4297ff9eb5454f4bb4a93
Status: RESOLVED → REOPENED
status-firefox65: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla65 → ---
The regressions (test failures) as mentioned which need to be fixed before re-landing this patch are: * Bug 1513451 - Affecting web-platform-tests (various Android 7.0 x86 wpt timeouts in /html/browsers) - might be related to the new waitForEvent or waitForObserverNotification promises * Bug 1513436 - outer window id is undefined (details see bug 1513436 comment 2) * Bug 1512342 - hang when opening a new window and not focusing it (details see bug 1512342 comment 2)
Status: REOPENED → ASSIGNED
Blocks: 1510940
(In reply to Henrik Skupin (:whimboo) [away 12/05-12/16] from comment #32) > * Bug 1513451 - Affecting web-platform-tests (various Android 7.0 x86 wpt > timeouts in /html/browsers) - might be related to the new waitForEvent or > waitForObserverNotification promises Partial backouts of the commits has been shown that the problematic commit is https://hg.mozilla.org/integration/autoland/rev/a8ea6d01fbe1, which adds the `waitForObserverTopic` promise.
Depends on: 1512336
Depends on: 1517414
(In reply to Henrik Skupin (:whimboo) from comment #32) > * Bug 1513436 - outer window id is undefined (details see bug 1513436 > comment 2) So I missed that the PollPromise is also using a timeout, which by default is set to 2s only. By checking Orange Factor for all the known failures of this particular problem it shows that both debug and ASAN opt builds mostly on Windows 7 32bit are affected when opening a new tab but not a new window. For both it can take longer than those 2s until the outerWindowId is set on the content browser instance. I find this a kinda long time. Mike or Felipe, is such a slow behavior known, or don't we have any of those measurements to look at eg via Talos? Here an example: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=216491580&repo=mozilla-beta&lineNumber=18590-18629 The TabOpen event fires at 1544573299318, and our frame script gets registered at 1544573299776 which is 460ms later! And another 1.6s later at 1544573301405 the outerWindowId is still null via the linkedBrowser.
Flags: needinfo?(mconley)
Flags: needinfo?(felipc)
(In reply to Henrik Skupin (:whimboo) from comment #34) > (In reply to Henrik Skupin (:whimboo) from comment #32) > > * Bug 1513436 - outer window id is undefined (details see bug 1513436 > > comment 2) > > So I missed that the PollPromise is also using a timeout, which by default > is set to 2s only. As discussed with Andreas on IRC we will fix that by letting PollPromise default to no using a timeout. It's tighter modeled to Promise than the TimedPromise.
(In reply to Henrik Skupin (:whimboo) from comment #32) > * Bug 1513451 - Affecting web-platform-tests (various Android 7.0 x86 wpt > timeouts in /html/browsers) - might be related to the new waitForEvent or > waitForObserverNotification promises I investigated that more today and found the following via this try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=81be51278fd275f0644e02e70403fe02f05912df&selectedJob=220042919 The failure happens when the test is started and the script "testharness_webdriver.js" gets executed. As it looks like the page load never happens, and as such it runs into a timeout which itself triggers bug 1286893: > 01-04 21:40:59.917 3064 3081 W GeckoConsole: [JavaScript Warning: "Loading failed for the <script> with source “http://web-platform.test:8000/resources/testharness.jsâ€." {file: "http://web-platform.test:8000/html/browsers/browsing-the-web/history-traversal/window-name-after-cross-origin-sub-frame-navigation.sub.html" line: 5}] > 01-04 21:41:39.904 3064 3081 E GeckoConsole: [JavaScript Error: "TypeError: window.win.timeout is not a function" {file: "tests/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py" line: 86}] It means that something might be wrong with the window handles which also is visible via: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=220053068&repo=try&lineNumber=4863-4869 > [task 2019-01-04T21:41:00.140Z] 21:41:00 INFO - Remaining handles: [u'14', u'94', u'97'] > [task 2019-01-04T21:41:00.141Z] 21:41:00 INFO - ** dismissing possible user prompts > [task 2019-01-04T21:41:00.142Z] 21:41:00 INFO - ** switching to handle: 94 > [task 2019-01-04T21:41:00.147Z] 21:41:00 INFO - Switched to handle: 94 > [task 2019-01-04T21:41:00.147Z] 21:41:00 INFO - Closing window 94 > [task 2019-01-04T21:41:00.166Z] 21:41:00 INFO - window 94 closed > [task 2019-01-04T21:41:00.167Z] 21:41:00 INFO - Remaining handles: [u'14', u'94'] > [task 2019-01-04T21:41:00.167Z] 21:41:00 INFO - ** dismissing possible user prompts > [task 2019-01-04T21:41:00.167Z] 21:41:00 INFO - ** window not found: 97 > [task 2019-01-04T21:41:00.167Z] 21:41:00 INFO - Remaining handles: [u'14', u'94'] > [task 2019-01-04T21:41:00.167Z] 21:41:00 INFO - Switching to runner handle: 14 > [task 2019-01-04T21:41:00.167Z] 21:41:00 INFO - Switched to runner handle: 14 Marionette switches to handle 94 and thinks to close it, but actually handle 97 is closed? This extra window seems to be also only around in case of failing tests. Otherwise two windows are present. That also means that after switching to the runner handle, and executing "testharness_webdriver.js" again for the next test, the unique window name is the same as for the window with the handle 94. As such this already open window will be re-used, and somehow the page load isn't triggered at all. I propose that we move out the specific patch for `waitForObserverTopic` to a follow-up bug, so that we can unblock this one. It's not really needed.
Blocks: 1518169
(In reply to Henrik Skupin (:whimboo) from comment #34) > So I missed that the PollPromise is also using a timeout, which by default > is set to 2s only. By checking Orange Factor for all the known failures of > this particular problem it shows that both debug and ASAN opt builds mostly > on Windows 7 32bit are affected when opening a new tab but not a new window. > For both it can take longer than those 2s until the outerWindowId is set on > the content browser instance. I find this a kinda long time. > > Mike or Felipe, is such a slow behavior known, or don't we have any of those > measurements to look at eg via Talos? I think that a 2s delay can indeed happen occasionally, specially on debug and asan builds, as opening a new tab will likely involve creating a new process. (The difference observed between new tab and new window might be just a coincidence). I think the question to answer is if the outerWindowId ever gets set, just slowly (i.e., past 2s), or if that never happens.. Bumping the 2s timeout to e.g. 30s would probably answer this easily. (Or, as it looks like you're planning, changing to a normal promise which will "bump" it to infinity)
Flags: needinfo?(felipc)

(In reply to Henrik Skupin (:whimboo) from comment #36)

Uh, I just noticed that we register the events for the newly opened window after calling focus() on it. This perfectly would indicate a race condition as seen here.

I pushed a new try build with all the fixes included:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4dd2722de6563a97a019b32fa65db99238a8e419

(In reply to :Felipe Gomes (needinfo me!) from comment #37)

I think the question to answer is if the outerWindowId ever gets set, just
slowly (i.e., past 2s), or if that never happens.. Bumping the 2s timeout
to e.g. 30s would probably answer this easily. (Or, as it looks like you're
planning, changing to a normal promise which will "bump" it to infinity)

Ok, great. So we are on the right path. Thanks for your reply!

Flags: needinfo?(mconley)

I missed to re-add the formerly removed import of waitForObserverTopic to browser.js. Here a new try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f352d949c5430172ab1226ca5fe6b23796d5217

This try build looks pretty promising. I triggered some extra Mn jobs to verify that the low intermittent for the race condition is also no longer present.

Blocks: 1329424
Blocks: 1518741
Attachment #9029249 - Attachment description: Bug 1504756 - [marionette] Added "WebDriver:NewWindow" command to open a new top-level browsing context. r?ato → Bug 1504756 - [marionette] Added "WebDriver:CreateWindow" command to open a new top-level browsing context. r?ato

Fixing summary given that the new command has been named differently as the name we initially assumed.

Summary: Implement New Window command → Implement "WebDriver:CreateWindow" command
Attachment #9034935 - Attachment description: Bug 1504756 - [marionette] By default PollPromise should't use a timeout. r?ato → Bug 1504756 - [marionette] Remove default timeout from PollPromise. r?ato
Attachment #9029249 - Attachment description: Bug 1504756 - [marionette] Added "WebDriver:CreateWindow" command to open a new top-level browsing context. r?ato → Bug 1504756 - [marionette] Added "WebDriver:NewWindow" command to open a new top-level browsing context. r?ato

We changed the spec to New Window.

Summary: Implement "WebDriver:CreateWindow" command → Implement "WebDriver:NewWindow" command
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/93ff3f0d95bd [marionette] Add executeSoon() to run tasks on the main thread. r=ato https://hg.mozilla.org/integration/autoland/rev/21658a2d0174 [marionette] Use waitForEvent() when waiting for events. r=ato https://hg.mozilla.org/integration/autoland/rev/58ab81d373b9 [marionette] Use waitForMessage() to wait for an expected message manager message. r=ato https://hg.mozilla.org/integration/autoland/rev/c25c93d2dc4d [marionette] Use waitForObserverTopic when waiting for observer notifications. r=ato https://hg.mozilla.org/integration/autoland/rev/9715660f8c07 [marionette] Remove default timeout from PollPromise. r=ato https://hg.mozilla.org/integration/autoland/rev/8f9d90979825 [marionette] Added "WebDriver:NewWindow" command to open a new top-level browsing context. r=ato https://hg.mozilla.org/integration/autoland/rev/9d80f662ad2b [marionette] Added opening a new browsing context to Marionette client. r=ato

List of failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=xpcshell&fromchange=9d80f662ad2b&tochange=4ff41b70cbd8e7d24242ee6839199875893b5cac

It's only happening on Windows and Linux but not MacOS, where I was testing locally. And I actually missed to also trigger xpcshell tests with my last try build :/.

Here the failure which is consistent across both platforms:

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=220893546&repo=autoland&lineNumber=2507

TEST-UNEXPECTED-FAIL | testing/marionette/test/unit/test_sync.js | test_PollPromise_timeoutElapse - [test_PollPromise_timeoutElapse : 217] 11 <= 10

With a timeout of 100ms and an interval of 10ms I thought it would run 10 times at maximum. But that is actually not the case because we also run the callback already before setting up the timer to cover the case of a timeout of 0s. Why it only runs 10 times on MacOS might be related to some extra delays.

I will get this fixed by just bumping the maximum number of iterations to 11 so that the check reads:

lessOrEqual(nevals, 11);

Flags: needinfo?(hskupin)

This try build looks fine now for xpcshell. I'm going to push this patch series again now.

No longer depends on: 1517414
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1c023add0d15 [marionette] Add executeSoon() to run tasks on the main thread. r=ato https://hg.mozilla.org/integration/autoland/rev/70e87e4f91d0 [marionette] Use waitForEvent() when waiting for events. r=ato https://hg.mozilla.org/integration/autoland/rev/4e0d85dd8ee0 [marionette] Use waitForMessage() to wait for an expected message manager message. r=ato https://hg.mozilla.org/integration/autoland/rev/ae09fe7cb735 [marionette] Use waitForObserverTopic when waiting for observer notifications. r=ato https://hg.mozilla.org/integration/autoland/rev/bd95222980e2 [marionette] Remove default timeout from PollPromise. r=ato https://hg.mozilla.org/integration/autoland/rev/8a0f8116e6bb [marionette] Added "WebDriver:NewWindow" command to open a new top-level browsing context. r=ato https://hg.mozilla.org/integration/autoland/rev/ecb0589b1079 [marionette] Added opening a new browsing context to Marionette client. r=ato
Blocks: 1501778
Blocks: 1522790
Blocks: 1523234
Depends on: 1534018
Blocks: 1559120
Depends on: 1563040
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: