"WebDriver:SetWindowRect" throws invalid TimeoutError if window is resized to a not reachable dimension
Categories
(Remote Protocol :: Marionette, defect, P1)
Tracking
(firefox65 fixed, firefox66 fixed)
People
(Reporter: ato, Assigned: whimboo)
References
(Blocks 1 open bug, )
Details
(Keywords: regression)
Attachments
(3 files)
See description and discussion in https://github.com/mozilla/geckodriver/issues/1326.
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 2•6 years ago
|
||
I asked the reporter on the github issue for feedback. If it turns out that it works for him now, we can close this bug.
|
|
Assignee | |
Comment 3•6 years ago
|
||
So I actually can reproduce the problem. Here is what happens based on the following script: > def test_check(driver): > driver.set_window_size(1680, 1050) > driver.set_window_size(1680, 1050) The screen size for my external display the tests run on under MacOS is 1680x1050. Now I try to set the window size to that dimension (or higher). The window resizes until it reaches the maximal allowed size, which are the screen dimensions subtracted the height of the menu bar, and the dock. Given that the dock is on the left side for me it results in the following dimensions: > TRACE 0 -> [0,2,"WebDriver:SetWindowRect",{"height":1050,"width":1680}] > TRACE 0 <- [1,2,null,{"x":48,"y":22,"width":1632,"height":1028,"state":"maximized"}] Now with the next call to `set_window_size()` we notice that the requested dimension is different from the current one, and start resizing the window again. But given that it has already reached the maximum size, no more `resize` event will be fired. What's good is that with bug 1492499 fixed there is no hang anymore. As such we can lower the severity of this bug. But we get a timeout error, which by the spec is not allowed. There are two things to fix: 1) Catch and ignore any timeout error for window manipulation commands, and always return success. 2) Only resize a window to a larger dimension if it hasn't reached its maximized state. As it looks like we perfectly detect that already.
|
|
Assignee | |
Updated•6 years ago
|
|
|
Assignee | |
Comment 4•5 years ago
|
||
We have to fix this for the Firefox 65 release, otherwise we will get a lot of complains from users because of those timeout errors.
|
|
Assignee | |
Comment 5•5 years ago
|
||
Throwing the timeout will actually be fixed by bug 1515867. But running into it which causes a delay similar to minimize window, I will pick up afterward.
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 6•5 years ago
|
||
Sorry for the back and forth but we decided to fix this for real and not to get away with the timeout error only.
|
|
Assignee | |
Updated•5 years ago
|
|
|
Assignee | |
Comment 7•5 years ago
•
|
||
I reproduced this now by trying to set a dimension of 100x100 twice for the browser window. Whereby the absolute minimal size can be width=335px, height=121px on my system. As such we are trying to resize but never get a resize event. As such the DebounceCallback never gets run, and early exits.
Maybe we need a maximum expected time until the first event should actually happen. And if that is not the case bail out.
|
|
Assignee | |
Comment 8•5 years ago
|
||
Ok, so if you call win.resizeTo() the resize operation will be done in a single step. As such only a single resize event is being fired. That means we clearly don't need the debounceCallback method here.
Further and similar to win.moveTo() the function returns when the operation has been done. As such we should be able to even get rid of using the TimedPromise. We should only use the IdlePromise to allow the resize event to be fired.
Here how it looks like when resizing to a new dimension:
1547219997180 Marionette DEBUG 3 -> [0,15,"WebDriver:SetWindowRect",{"y":null,"x":null,"width":335,"height":100}]
** before resizeTo
** before IdlePromise
** resize event
** after IdlePromise
1547219997317 Marionette DEBUG 3 <- [1,15,null,{"x":48,"y":23,"width":335,"height":121}]
|
|
Assignee | |
Comment 9•5 years ago
|
||
With the upcoming patch we will also have a huge performance improvement. I run the Mn unit test test_window_rect.py 10 times, and here is the output:
before: 24.85s user 6.69s system 43% cpu 1:12.92 total
after: 24.83s user 6.70s system 69% cpu 45.294 total
As visible we are way better utilizing the CPU, and as such it is ~38% faster.
|
|
Assignee | |
Comment 10•5 years ago
|
||
|
|
Assignee | |
Comment 11•5 years ago
|
||
|
|
Assignee | |
Comment 12•5 years ago
|
||
To have the same pre-conditions for all wdspec tests, the session fixture has to set a default window position. Depends on D16338
|
|
Assignee | |
Comment 13•5 years ago
|
||
Depends on D16340
|
||
Comment 14•5 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/602e8cefc900 [marionette] Don't wait for resize events in "Set Window Rect". r=ato https://hg.mozilla.org/integration/autoland/rev/d4a75d417b03 [wdspec] Enforce a default window position for each session. r=ato https://hg.mozilla.org/integration/autoland/rev/3ef29704bc3d [wdspec] Enhance tests for the "Set Window Rect" command. r=ato
|
||
Comment 15•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/602e8cefc900
https://hg.mozilla.org/mozilla-central/rev/d4a75d417b03
https://hg.mozilla.org/mozilla-central/rev/3ef29704bc3d
|
|
Assignee | |
Comment 16•5 years ago
|
||
This is a test-only regression for Firefox 65. Please uplift to mozilla-beta.
|
||
Comment 17•5 years ago
|
||
| bugherder uplift | ||
https://hg.mozilla.org/releases/mozilla-beta/rev/2d63cb7c2ccc
https://hg.mozilla.org/releases/mozilla-beta/rev/c52783e0d7e1
https://hg.mozilla.org/releases/mozilla-beta/rev/c3a3d5aadad2
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14864 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks: Github PR https://github.com/web-platform-tests/wpt/pull/14864 * Taskcluster (pull_request) (https://tools.taskcluster.net/task-group-inspector/#/NF3aBn3AQP23lDFSCoDbLQ)
Upstream PR was closed without merging
|
|
Assignee | |
Updated•5 years ago
|
Upstream PR merged
|
||
Updated•1 year ago
|
Description
•