1507067 - "WebDriver:MinimizeWindow" has a substantial delay since bug 1492499 landed

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

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

Since bug 1492499 landed I can see a substantial delay of several seconds for the `WebDriver:MinimizeWindow` command. I assume that internally it even runs into a timeout for a `TimedPromise`.

Comment 1

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

Note that the delay is at least visible on MacOS. I didn't test other platforms.

Comment 2

[Andreas Tolfsen ❲:ato❳]

Depending on the platform, the visibilitychange event we need for
WebDriver:MinimizeWindow is not fired.  I have a suspicion this
happens in quite a lot of configurations, not just macOS.  It would
be good to get confirmation of that.

Because the timeout delay is sufficient enough to allow the window
to pass into minimized state, the tests for this feature work as
intended.  The only user-observable difference is that there is now
a bigger delay.

Comment 3

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

(In reply to Andreas Tolfsen ❲:ato❳ from comment #2)
> Depending on the platform, the visibilitychange event we need for
> WebDriver:MinimizeWindow is not fired.  I have a suspicion this
> happens in quite a lot of configurations, not just macOS.  It would
> be good to get confirmation of that.

The problem here doesn't occur due to missing platform features, but simply because the event listener is attached to the chrome window. The parent process will never receive such events. Instead `visibilitychange` needs to be registered via the content browser's document. This is also explained in the first sentence at:

https://developer.mozilla.org/en-US/docs/Web/Events/visibilitychange

The question is how to best implement that. We cannot simply add an event listener in driver.js, but most likely would have to add a global listener in listener.js, which then sends a specific Marionette event to the parent process.

Andreas, would you have the time to fix that bug? It would be good to get this regression fixed before the 65 release, which will already happen in 3 weeks!

Comment 4

[Andreas Tolfsen ❲:ato❳]

We used to listen for the visibilitychange event in content through
ContentEventObserverService:
https://searchfox.org/mozilla-central/source/testing/marionette/dom.js#134

You would activate a listener like this:

> await new Promise(resolve => {
>   webElement.addEventListener("visibilitychange", resolve, {once: true});
>   contentBrowser.minimize();
> });

The patch that added this infrastructure is
https://hg.mozilla.org/mozilla-central/rev/ead4f1cf5c5a.

The problem with that approach was similar to the explanation I
gave in https://bugzilla.mozilla.org/show_bug.cgi?id=1517414#c1:
visibilitychange in content also only fires under certain circumstances.

Maybe it’s worth resurrecting this infrastructure (or remove it if
we don’t need it) and use a TimedPromise for configurations where
it doesn’t fire?

Comment 5

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

(In reply to Andreas Tolfsen ⦗:ato⦘ from comment #4)

We used to listen for the visibilitychange event in content through
ContentEventObserverService:
https://searchfox.org/mozilla-central/source/testing/marionette/dom.js#134

This works great locally. And this approach should even be Fission compatible, right? Or at least easily adaptable later when we have to do the refactoring.

Maybe it’s worth resurrecting this infrastructure (or remove it if
we don’t need it) and use a TimedPromise for configurations where
it doesn’t fire?

This sounds exactly as I would implement it. For systems on which we don't have the event we can certainly wait those 2 seconds.

Comment 6

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

Oh and note that the "visibilitychange" event is raised a couple of times. As such we would also need the DebounceCallback.

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bfd09250f6edca363fc553bb833d29a8da28bd1

Comment 8

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

Attachment: Bug 1507067 - [marionette] Add logging of received DOM events to ContentEventObserverService. r?ato

Comment 9

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

Attachment: Bug 1507067 - [marionette] Add support for EventListener callback to WebElementEventTarget. r?ato

Depends on D16095

Comment 10

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

Attachment: Bug 1507067 - [marionette] Listen for "visibilitychange" event in content process. r?ato

To detect when the window got minimized usally "visibilitychange" events
are fired to the content window. The "WebDriver:MinimizeWindow" command
has to wait for those.

Depends on D16096

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab507b25036a
[marionette] Add logging of received DOM events to ContentEventObserverService. r=ato
https://hg.mozilla.org/integration/autoland/rev/826014440f86
[marionette] Add support for EventListener callback to WebElementEventTarget. r=ato
https://hg.mozilla.org/integration/autoland/rev/9b52701eff7f
[marionette] Listen for "visibilitychange" event in content process. r=ato

Comment 12

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/ab507b25036a
https://hg.mozilla.org/mozilla-central/rev/826014440f86
https://hg.mozilla.org/mozilla-central/rev/9b52701eff7f

Comment 13

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

Please uplift this test-only patch to mozilla-beta to fix the regression.

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/888054b8eec1
https://hg.mozilla.org/releases/mozilla-beta/rev/482eb149ad4a
https://hg.mozilla.org/releases/mozilla-beta/rev/189f49635cfd