2003857 - Wait for document to be visible in browsingContext.create with background=false

Status: RESOLVED FIXED

(Remote Protocol :: WebDriver BiDi, task, P3)

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 2003857 - [bidi] Wait for document to be visible in browsingContext.create with background=false

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 2003857 - [bidi] Update waitForVisibility helper to wait either for visible or hidden state

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/793fc3bce145
https://hg.mozilla.org/integration/autoland/rev/b00b6e487c7d
[bidi] Wait for document to be visible in browsingContext.create with background=false r=Sasha
https://github.com/mozilla-firefox/firefox/commit/c90c138fc659
https://hg.mozilla.org/integration/autoland/rev/06139a41cf08
[bidi] Reuse waitForVisibility helper to wait for document to be visible r=Sasha

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/7359b193a2dc
https://hg.mozilla.org/integration/autoland/rev/7e546543d132
Revert "Bug 2003857 - [bidi] Reuse waitForVisibility helper to wait for document to be visible r=Sasha" for causing puppeteer timeouts @ screenshot.spec.js

Comment 5

[amarc]

Backed out for causing puppeteer timeouts

• Backout link
• Push with failures
• Failure Log

Comment 6

[Julian Descottes [:jdescottes]]

Thanks for the ping. I did spot this on try, but since we had a known intermittent for this failure I assumed it was just this.
I imagine we can have an issue if a consumer is calling browsingContext.create several times in a row quickly. The n-1 call might hang if the next call already created a tab which hides the previous one.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 2003857 - [bidi] Update helper to wait for visibility to use visibilityChange event

The current polling approach is very likely to miss updates and face race issues.

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 2003857 - [wdspec] Add test for parallel calls to browsingContext.create

Comment 9

[Julian Descottes [:jdescottes]]

Using an event to wait for the visibility fixes the issue locally and on some platforms, but it still fails from time to time on CI and more importantly, the puppeteer test still fails on CI (although it also passes locally). At this point I'm not sure we can reliably wait for the visibility to change after using browsingContext.create. It seems hard to account for all the potential scenarios where another tab could be created right next after browsingContext.create created its new tab.

After looking at parallel calls to browsingContext.create, I tested other combinations. I found that doing a browsingContext.create and a browsingContext.activate in parallel was already failing with our current implementation. Similarly this is because we are waiting for visibility to change, but the other command makes it impossible to achieve the expected state.

So we already have a brittle implementation due to waiting for visibility changes and I would not add more to it. Potentially we could still keep the code cleanup, the switch to event instead of polling and the new test. But in terms of fixing the intermittent, I think we should either fix it in the test or update the expectations for win ccov.

Comment 10

[Julian Descottes [:jdescottes]]

I'm still unsure whether we should fix it in the BiDi implementation or in the tests. I looked at fixing the two currently failing tests, and while that works, it's also a bit hard to justify waiting in those tests. End users might also run into similar situations.

Typically what we are seeing is that the focus step happening during the browsingContext.create might race with another command which is also trying to focus another browsing context. At the moment we are not waiting for the visibility to be set after calling the command. In the activate test (bug 2002827) the new context steals the focus from the context we try to activate in the next command. In the background.py test, it's more straightforward, the visibility is simply not updated yet.

Waiting for the visibility at the end of the command does create potential issues when commands are sent in parallel, or simply if there is a tab creation/selection not controller by the webdriver session. I'm doing a last attempt here to see if waiting for visibility, but with a timeout as an escape hatch, could be a decent compromise. If not, maybe for now we just update expectations, and plan more work on this in the future.

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 2003857 - [bidi] Wait for document to be visible in browsingContext.create with background=false

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 2003857 - [wdspec] Reenable test_switch_between_contexts[window] on wayland

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: Bug 2003857 - [bidi] Add a timeout parameter to wait for visibility

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/d6eadc17d8c3
https://hg.mozilla.org/integration/autoland/rev/92c37c3e1a25
[bidi] Update waitForVisibility helper to wait either for visible or hidden state r=Sasha
https://github.com/mozilla-firefox/firefox/commit/d1180226a76e
https://hg.mozilla.org/integration/autoland/rev/92039cc46919
[bidi] Update helper to wait for visibility to use visibilityChange event r=Sasha
https://github.com/mozilla-firefox/firefox/commit/dba09acbc4fd
https://hg.mozilla.org/integration/autoland/rev/32fe9972e6cb
[bidi] Wait for document to be visible in browsingContext.create with background=false r=Sasha,whimboo
https://github.com/mozilla-firefox/firefox/commit/80f703ee9209
https://hg.mozilla.org/integration/autoland/rev/9e82be20f856
[wdspec] Add test for parallel calls to browsingContext.create r=Sasha
https://github.com/mozilla-firefox/firefox/commit/ab61a8b4e5c7
https://hg.mozilla.org/integration/autoland/rev/bd018601bc5c
[wdspec] Reenable test_switch_between_contexts[window] on wayland r=Sasha

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/56671 for changes under testing/web-platform/tests

Comment 16

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/92c37c3e1a25
https://hg.mozilla.org/mozilla-central/rev/92039cc46919
https://hg.mozilla.org/mozilla-central/rev/32fe9972e6cb
https://hg.mozilla.org/mozilla-central/rev/9e82be20f856
https://hg.mozilla.org/mozilla-central/rev/bd018601bc5c

Comment 17

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot