1602701 - Improve handling for moving and resizing of windows on Linux

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

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

As mentioned on bug 1602387 any movement or resizing of windows needs to be handled in an asynchronous manner. That's not what Marionette actually does:

https://searchfox.org/mozilla-central/rev/23d4bffcad365e68d2d45776017056b76ca9a968/testing/marionette/driver.js#1522,1532

Instead of using the IdlePromise a TimedPromise with a DebounceCallback might be good to ensure that no further resize (also used when moving windows?) events are getting emitted. Here an example for the minimize window case:

https://searchfox.org/mozilla-central/rev/23d4bffcad365e68d2d45776017056b76ca9a968/testing/marionette/driver.js#3139-3146

Comment 1

[Edwin Takahashi (:egao | infrequent contributor)]

I have a first test push at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&tier=1%2C2%2C3&revision=f5b7a5a51c2f120022059db0b466d50776737906.

This seems to have reduced the instances of bug 1600391. Combined with the patch to change how the maximum available height is calculated, this means marionette now produces a consistently reproducible set of failures across multiple runs:

TEST-UNEXPECTED-FAIL | testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen | AssertionError: 1136 not greater than or equal to 1173
TEST-UNEXPECTED-FAIL | testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_to_available_screen_size | AssertionError: 1136 not greater than or equal to 1173

Not sure where the 1136 value is coming from, since the 1173 value is quite well established at this point. I will have to try applying this patch to my local VM and see how it behaves there for clues.

:whimboo - I realize my JS is poor, but does this patch kind of go along what you were expecting?

Comment 2

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

You will have to register the debounce callback as handler for some event. For resizing the window it will be:

window.addEventListener("resize", cb);

Maybe that will also work for moveTo?

Right now it should return immediately.

Comment 3

[Edwin Takahashi (:egao | infrequent contributor)]

I properly register the callback method as you suggested, also looking at the other functions that use a similar code flow for reference.

try push: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&selectedJob=280820896&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Crunning%2Cpending%2Crunnable&tier=1%2C2%2C3&revision=736b749591af57a30659afd1bb1968d5b60ebe62

The two failures about 1136 not greater than or equal to 1173 still remains. Not sure where this 1136 value is coming from - I tried to reproduce this locally on my macosx1015 and ubuntu1804 VM but the latest pull of mozilla-central seemed to have some issues that timed out marionette test after 120s.

I will need to revisit how the height is being calculated, the difference between ubuntu1604 and ubuntu1804 with the height dimensions and such. I think the window.outerHeight call in ubuntu1804 produced a number something like 1143, but that isn't 1136.

Comment 4

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

Note that there is no move event for Window. As mentioned above maybe it also used the resize event?

Also when you run the test locally do you see that the window occupies all the available screen size, or is there still a small area (top or bottom) which isn't covered?

Comment 5

[Edwin Takahashi (:egao | infrequent contributor)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+2] from comment #4)

Note that there is no move event for Window. As mentioned above maybe it also used the resize event?

Also when you run the test locally do you see that the window occupies all the available screen size, or is there still a small area (top or bottom) which isn't covered?

I think I understand what you mean now - I can't just choose my own event name, it has to match an existing one like resize.

I've made changes and pushed to try again, this time with resizeTo and moveTo both using the resize event: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Crunning%2Cpending%2Crunnable&tier=1%2C2%2C3&revision=ae914a6ba6b7e5d77d366b5adfe0a102d9fb4334

EDIT: Green run!

Comment 6

[Edwin Takahashi (:egao | infrequent contributor)]

Attachment: Bug 1602701 - wait for debouncecallback for win.moveTo and win.resizeTo operations in marionette driver

Comment 7

[Edwin Takahashi (:egao | infrequent contributor)]

I posted a patch, but I am not sure if I had implemented it correctly as I see this in the log:

Marionette WARN TimedPromise timed out after 5000 ms: stacktrace:

and the wdspec jobs don't succeed:

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception%2Cretry%2Crunning%2Cpending%2Crunnable&tier=1%2C2%2C3&revision=aa1d7abdc013de917f67e232f5c872031188eb40

Comment 8

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

Edwin, does the latest comment here still apply to the patch as you asked review for in Phabricator? I wonder because you didn't mention any problems there.

Comment 9

[Edwin Takahashi (:egao | infrequent contributor)]

Apologies I hadn't been clear enough - I have a try push with the latest changes to self.max/self.screensize incorporated, and it would likely exhibit the same failures on wdspec tests when run in ubuntu1804:

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&tier=1%2C2%2C3&revision=94d01b42ace05affae219899ada2c6a7f114403d

Comment 10

[Edwin Takahashi (:egao | infrequent contributor)]

I've taken another stab at this and incorporated the changes suggested:

https://hg.mozilla.org/try/rev/dbf4da833b1e1be549ee62111dcd20c1ea644fbb

However, the changes seem to make the test reliability much worse than what is in-tree currently:

in-tree (default, no changes to code): https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&selectedJob=291542980&tier=1%2C2%2C3&revision=741a0520225070819b0b2f5e9d84d98b86164932
with above changes applied: https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&selectedJob=291542980&tier=1%2C2%2C3&revision=6d8c55b3492ae578be7560b0c6365ab2068747e2

It seems that in the run with the changes applied, SetWindowRect fails for both moveTo and resizeTo, so perhaps the TimedPromise for resizeTo also doesn't work reliably.

Comment 11

[Edwin Takahashi (:egao | infrequent contributor)]

Another attempt, and it seems to be much better:
https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&tier=1%2C2%2C3&revision=6b34d865ee360f3141e8cd6b547e8d288dd67c6e

I will put up a patch in bug 1600389 to correct the expectation of test_move_to_negative_coordinates because on GNOME desktop environment systems, it is not possible to move the window to (0, 0) coordinate, period.

Once the expectation for that test is updated to properly reflect the top navigation bar, the most recent attempt should produce green test runs at reasonable frequency. This will help debugging easier.

Comment 12

[Edwin Takahashi (:egao | infrequent contributor)]

While Bug 1621483 has sidestepped this issue, for future GTK/GNOME based systems this should still be fixed.

Comment 13

[Robert Mader [:rmader]]

Attachment: Bug 1602701 - [wdspec] Mark some window rect specific user prompt tests as flaky on Linux. r?whimboo

These test fail much more often as soon as D93173 lands (see bug 1645528).
The list may not yet be comprehensive though.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6702949f238b
[wdspec] Mark some window rect specific user prompt tests as flaky on Linux. r=whimboo,webdriver-reviewers

Comment 15

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

Thanks Robert for the temporarily skip. Now with the tests failing more often we might have a chance to figure out how to better handle asynchronous window resizes / movements.

Comment 16

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6702949f238b

Comment 17

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

This problem is causing a lot of tests to fail upstream in the web-platform-tests CI. So we should check if it is something we might have to prioritize. Lets discuss in the next triage meeting.

Comment 18

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

As we discussed there is the possibility to mark test failures with bugs on wpt.fyi, so it shouldn't be such an annoying issue.

Nevertheless the window manager might deny resizing and moving of the window to a specific size or location if it's not possible. But there might also be a performance issue on these CI machines. We might want to try to increase the timer for resize events from 250ms to somewhat higher to try out if waiting longer for the last resize event helps here.

Comment 19

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

I noticed issues with resizing to the maximum size of the screen over on bug 1715629. I would like to get that fixed first.

Comment 20

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

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #19)

I noticed issues with resizing to the maximum size of the screen over on bug 1715629. I would like to get that fixed first.

Sorry, I actually meant to reference bug 1522855 here.

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:whimboo, maybe it's time to close this bug?

Comment 22

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

No, it's still required but we haven't had the time to work on this bug.

Comment 23

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

Test failures should actually be on the blocking list. We should re-check for improvements once the patches from bug 1745595 got landed.

Comment 24

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

With bug 1745595 fixed I've pushed a try build now and will do quite a lot of retriggers for the specific Wdspec jobs on Linux once it's clear which jobs actually run the related tests.

https://treeherder.mozilla.org/jobs?repo=try&revision=d99d057def35628fb3bae5862374fb6509f38140

Comment 25

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

Attachment: Bug 1602701- [wdspec] Re-enable window resize/move specific tests on Linux.

Comment 26

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

There is actually no other work needed here as just re-enabling the tests. As such I would update the bug summary as well given that it's not completely right. We had asynchronous code there as well before bug 1745595 got fixed.

The try build looks pretty good and I think that we should get all the tests re-enabled.

Comment 27

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

As discussed moving to our milestone 4.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6f859966443a
[wdspec] Re-enable window resize/move specific tests on Linux. r=webdriver-reviewers,jdescottes

Comment 29

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/6f859966443a