1826645 - Tests of DOM fullscreen are vulnerable to timeouts

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Brad Werth [:bradwerth]]

The DOM fullscreen code is not adequate to handle the rapid requests of enter -> exit -> enter fullscreen done by our testing harness. For platforms with asynchronous fullscreen transitions, our fullscreen tests will intermittently timeout. These tests serve the important purpose of ensuring that users experience correct fullscreen behavior, so we want to keep them enabled. Unfortunately, the code is fundamentally unable to do this, as demonstrated by a test that will be added in this Bug.

The root of the problem is that a fullscreen transition has several points of notification, but all the content-visible ones are fired before an asynchronous transition is complete. Our non-Chrome tests typically use these notifications unchecked to proceed to the next part ot the test, which might trigger another fullscreen transition. Events occur in this order (as an implementation detail, not by spec):

1. Resolve or reject the Promise and update internal state (immediately on the content process, possibly one event loop later on the parent process).
2. Trigger the transition in a platform-specific fashion.
3. Schedule the fullscreenchange or fullscreenerror event on the next event loop.
   -- transition completes --
4. (Chrome only) send the sizemodechange event.
5. Update window occlusion state.

On platforms with synchronous fullscreen transitions, these 5 things all happen in the same event loop, which allows our tests to rapid cycle requests and be successful. However on platforms with async fullscreen transitions, the following unexpected outcomes can occur with the pattern of enter -> exit -> enter:

• Upon exit, the content process may send the fullscreenchange event before the parent has processed the exit, then the content process requests a new enter which the parent processes immediately. It determines in FullscreenElementReadyCheck that the requesting element is already in fullscreen and resolves the Promise without ever sending the fullscreenchange event. Tests listening for the event will timeout. This type of failure may be possible even with a sync transition, since the content and the parent processes get out of agreement while the content process has already scheduled its fullscreenchange event.
• Before the window occlusion state is updated, a new fullscreen request will have its Promise rejected because the browsing context is not active. Tests awaiting enterFullscreen and not checking for rejects will be in an unexpected state and probably fail.
• When the asynchronous exit transition completes, it will again update internal state which could arrive during the processing of the next fullscreen request (which is also async). This introduces complicated overwriting of the internal state that is hard to diagnose and appears to either squash events or reject the Promise.

Even though these problems are probably never encountered by users, they impact the tests that check user-facing behavior. Possible fixes:

• Work through all these complicated timings and make the Document and AppWindow logic handle them. This is very hard.
• Add a new event fired at end of transition (similar to sizemodechange) that is checked by our tests, update all the tests. This has the disadvantage that it would be nice if the tests would "just work" and continue to use web-exposed APIs only.
• Make our test harness spoof the methods used to enter and exit fullscreen and add a delay representing user reaction time. Many entry points, possibly tricky, not guaranteed to work but more representative of user experience.
• Change the timing of the notifications. For example, Promises could be made to resolve at the end of fullscreen transitions. This doesn't appear to violate the spec and would allow any tests that wait on Promises to be left unchanged. Tests that wait for events would still be vulnerable.

None of these options seem great, and I welcome other approaches. I'll leave the Bug open while landing a test which demonstrates some of these points of failure.

Comment 1

[Brad Werth [:bradwerth]]

Attachment: Bug 1826645 Part 3: Add a test of fullscreen rapid-cycle enters and exits.

This test is marked expected fail on platforms with asynchronous
fullscreen transitions.

Comment 2

[Brad Werth [:bradwerth]]

The unlanded patch in Bug 1603334 is very useful for debugging Promise rejections.

Comment 3

[Brad Werth [:bradwerth]]

https://treeherder.mozilla.org/jobs?repo=try&revision=23da886ba632b87bbbce113740907993e6777c42

Comment 4

[Brad Werth [:bradwerth]]

Attachment: Bug 1826645 Part 1: Make FullscreenElementReadyCheck only check for no-ops in parent process.

Removing this check from content processes helps alleviate a race
condition on fullscreen exit followed by enter. This gives time for the
content process to process the exit without silently resolving the enter
without firing the event.

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Bug 1826645 Part 1: Make FullscreenElementReadyCheck check for focused tabs, not active tabs.

This change relaxes the check slightly. Spec requires that the tab is
focused. Our existing check for "active" is additionally requiring the tab
to be non-occluded. Since occlusion updates are asynchronous when the
transition itself is asynchronous, this change allows rapid requests to be
permitted as long as the underlying window and tab state is as expected.

Depends on D174982

Comment 6

[Brad Werth [:bradwerth]]

Attachment: Bug 1826645 Part 2: Make Document hold fullscreen requests while an exit is being processed.

This change makes the parent process delay a fullscreen request if there
is a pending fullscreen exit. It also changes the DOMFullscreenParent
actor listener lifecycle. Once it has started handling a fullscreen
request, it will remain a listener to the Document until it has received
an enter event and an exit event in either order.

Comment 7

[Edgar Chen [:edgar]]

(In reply to Brad Werth [:bradwerth] from comment #0)

• Before the window occlusion state is updated, a new fullscreen request will have its Promise rejected because the browsing context is not active. Tests awaiting enterFullscreen and not checking for rejects will be in an unexpected state and probably fail.

I am not too worried about this, as the promise is eventually rejected. Fullscreen requests can fail for multiple reasons, so the test should check the result of the promise as well. Can the test wait for the docshell to be active first, something like https://searchfox.org/mozilla-central/rev/ad732108b073742d7324f998c085f459674a6846/browser/components/customizableui/test/browser_panelUINotifications_fullscreen_noAutoHideToolbar.js#19-26 instead?

• When the asynchronous exit transition completes, it will again update internal state which could arrive during the processing of the next fullscreen request (which is also async). This introduces complicated overwriting of the internal state that is hard to diagnose and appears to either squash events or reject the Promise.

I think we could try to remove this, as nsGlobalWindowOuter::FinishFullscreenChange also ensures that the fullscreen state is synchronized, but only when there is no ongoing request.

• Work through all these complicated timings and make the Document and AppWindow logic handle them. This is very hard.

One of the major challenges is that there is no good way to track individual fullscreen IPC requests and handle multiple IPC requests arriving in a row. Document has a mechanism to handle multiple requests. While AppWindow doesn't do that directly, nsGlobalWindowOuter does.

Comment 8

[Brad Werth [:bradwerth]]

(In reply to Edgar Chen [:edgar] from comment #7)

I am not too worried about this, as the promise is eventually rejected. Fullscreen requests can fail for multiple reasons, so the test should check the result of the promise as well.

But many tests don't check the promises, and only check for the fullscreenchange events. Some of these tests are WPT tests which we couldn't justify changing just to make them work in Firefox.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb4ea58aaa6d
Part 1: Make FullscreenElementReadyCheck check for focused tabs, not active tabs. r=edgar
https://hg.mozilla.org/integration/autoland/rev/8f8c8f79661a
Part 2: Make Document hold fullscreen requests while an exit is being processed. r=edgar
https://hg.mozilla.org/integration/autoland/rev/aadec7cf67f8
Part 3: Add a test of fullscreen rapid-cycle enters and exits. r=edgar

Comment 10

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/eb4ea58aaa6d
https://hg.mozilla.org/mozilla-central/rev/8f8c8f79661a
https://hg.mozilla.org/mozilla-central/rev/aadec7cf67f8

Comment 11

[Brad Werth [:bradwerth]]

Looks like the patches make this test_fullscreen-api.html permafail in the Linux 18.04 x64 WebRender debug-mochitest-plain-headless-spi-nw test configuration. Those are Tier 2 but there's no reason for us to fail there. I'll do some try runs and figure out what that configuration needs changed.

Comment 12

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #11)

Looks like the patches make this test_fullscreen-api.html permafail in the Linux 18.04 x64 WebRender debug-mochitest-plain-headless-spi-nw test configuration. Those are Tier 2 but there's no reason for us to fail there. I'll do some try runs and figure out what that configuration needs changed.

Probably these patches will stay landed and I'll do the fix in Bug 1828910.

Comment 13

[Brad Werth [:bradwerth]]

Bug 1828910 was fixed, this can be resolved.