Closed Bug 1602701 Opened 3 years ago Closed 6 months ago

Improve handling for moving and resizing of windows on Linux

Categories

(Testing :: Marionette, defect, P1)

Version 3
All
Linux
defect
Points:
1

Tracking

(firefox-esr91 disabled, firefox85 disabled, firefox101 disabled, firefox102 disabled, firefox103 fixed)

RESOLVED FIXED
103 Branch
Tracking Status
firefox-esr91 --- disabled
firefox85 --- disabled
firefox101 --- disabled
firefox102 --- disabled
firefox103 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [webdriver:m4][webdriver-external])

Attachments

(2 files, 1 obsolete file)

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

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?

Flags: needinfo?(hskupin)

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.

Flags: needinfo?(hskupin)

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.

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?

(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!

Assignee: nobody → egao
Status: NEW → ASSIGNED

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

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.

Flags: needinfo?(egao)

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

Flags: needinfo?(egao)

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.

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.

Attachment #9115658 - Attachment is obsolete: true
Assignee: egao → nobody
Status: ASSIGNED → NEW
No longer blocks: 1600389
Depends on: 1600389

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

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

Assignee: nobody → robert.mader
Status: NEW → ASSIGNED
Keywords: leave-open
Assignee: robert.mader → nobody
Status: ASSIGNED → NEW
Assignee: nobody → robert.mader
Status: NEW → ASSIGNED
Attachment #9190514 - Attachment description: Bug 1602701 - Mark some test as flaky on Linux, r?whimboo → Bug 1602701 - [wdspec] Mark some window rect specific user prompt tests as flaky on Linux. r?whimboo
Pushed by cbrindusan@mozilla.com:
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

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.

Assignee: robert.mader → nobody
Status: ASSIGNED → NEW
Keywords: test-disabled
OS: Unspecified → Linux
Hardware: Unspecified → All

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.

Whiteboard: [webdriver:triage]

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.

Whiteboard: [webdriver:triage]

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

Depends on: 1715629

(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.

Depends on: 1522855
No longer depends on: 1715629

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

Flags: needinfo?(hskupin)

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

Flags: needinfo?(hskupin)
Blocks: 1520284

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

Blocks: 1522855, 1600389
No longer depends on: 1522855, 1600389

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

Blocks: 1743116
Assignee: nobody → hskupin
Status: NEW → ASSIGNED

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.

Summary: Moving and resizing of windows need to be handled as asynchronous operation → Improve handling for moving and resizing of windows on Linux

As discussed moving to our milestone 4.

Points: --- → 1
Keywords: leave-open
Priority: P2 → P1
Whiteboard: [webdriver:m4][webdriver-external]
Blocks: 1707390
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6f859966443a
[wdspec] Re-enable window resize/move specific tests on Linux. r=webdriver-reviewers,jdescottes
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → 103 Branch
You need to log in before you can comment on or make changes to this bug.