1619481 - Hang in IdlePromise if underlying window gets closed

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P3)

Description

[George Kovalev (:kovalev007)]

Testcase:
https://output.jsbin.com/fesusoj

1. click on button with text "open"
2. wait new window opened
3. click on button with text "close"
4. accept user promt
5. wait new window closed

Actual results: geckodriver hung after accept user promt

Comment 1

[George Kovalev (:kovalev007)]

Attachment: Test2App.java

Comment 2

[George Kovalev (:kovalev007)]

Attachment: mozregression_log_from_2019-06-13_to_2019-06-14.txt

Comment 3

[George Kovalev (:kovalev007)]

Information from mozregression:
Last good revision: 09614cb3a896ab3be71ccc44d7663eb5454cf5cf
First bad revision: 4651b31ffe5c012b5112fc3047c29d0d4a8f9594
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=09614cb3a896ab3be71ccc44d7663eb5454cf5cf&tochange=4651b31ffe5c012b5112fc3047c29d0d4a8f9594

Comment 4

[George Kovalev (:kovalev007)]

Attachment: discussion_in_irc.txt

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

To summarize, this is a hang in IdlePromise when the underlying window as operated on to wait for a requestAnimationFrame gets closed. This can happen at any time by random Javascript in the page.

To circumvent that we should add a listener or another Promise in IdlePromise for the unload event, and cancel the active requestAnimationFrame Promise, which will actually never happen.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

So here again a link to our IdlePromise implementation:
https://searchfox.org/mozilla-release/rev/00bea92f25c9edc8022593e9d9435210e8306047/testing/marionette/sync.js#334-340

I wonder if it is really necessary to also wait for requestAnimationFrame or if it is enough to just have the idleDispatchToMainThread in there.

Florian, do you have any feedback for us here or maybe know someone who can give that? Thanks.

Comment 7

[Florian Quèze [:florian]]

It's not clear to me what the IdlePromise callers expect it to do. If it's just to delay execution until the runnables already in the event queue have been processed, Services.tm.dispatchToMainThread would be enough, without waiting for and idle slice. If it's intended to wait for the window minimize/maximize/resize animations to be finished, then the current idle+requestAnimationFrame isn't waiting enough. Sorry that this doesn't really answer your question.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Thanks! I see, so I will have a patch which keeps that behavior and we could re-evaluate the usage of requestAnimationFrame at a later time if needed.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: Bug 1619481 - [marionette] IdlePromise should not hang when underlying window gets closed.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

George, can you please check if that patch works for you?

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e96cba7016e0
[marionette] IdlePromise should not hang when underlying window gets closed. r=marionette-reviewers,maja_zf

Comment 12

[Stefan Hindli [:stefan_hindli]]

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

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Moving the needinfo to bug 1601160.