Closed Bug 1776738 Opened 2 years ago Closed 1 year ago

Intermittent dom/tests/mochitest/pointerlock/test_pointerlock-api.html | single tracking bug

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
114 Branch
Tracking Flags:
Tracking Status
firefox114 --- fixed

People

(Reporter: jmaher, Assigned: bradwerth)

References

Details

(Keywords: intermittent-failure, intermittent-testcase)

Attachments

(1 file)

Bug 1776738: Make fullscreen and pointerlock mochitests use document.fullscreenElement to determine fullscreen-ness.
48 bytes, text/x-phabricator-request
Details | Review
No description provided.

Additional information about this bug failures and frequency patterns can be found by running: ./mach test-info failure-report --bug 1776738

I would like this fixed before landing more changes to macOS native fullscreen.

Assignee: nobody → bwerth

This seems to fail on macOS on the nestedFullScreen test because the second fullscreen request on the child is never detected as fullscreen. The method of comparison is to check the size of the div against the size of the screen. In emulated fullscreen mode, that child doesn't match the size of the screen, so the test times out.

With these changes, it's no longer necessary to store the normalSize on
the window, since we never check it, nor check for resize events. This
also removes the flaky timeouts, and replaces them with an executeSoon.

Blocks: 1802193
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1e63c1177db
Make fullscreen and pointerlock mochitests use document.fullscreenElement to determine fullscreen-ness. r=edgar

Backed out for causing mochitest failures in test_fullscreen-api.html

Flags: needinfo?(bwerth)

The failure logs from the repeated runs show failures in multiple places in test_fullscreen-api.html. requestFullscreen is getting rejected for one of the reasons in Document::FullscreenElementReadyCheck, but it's not clear which one. To be clear, the changes in this patch don't change how requestFullscreen works. I'll see if I can add a piece to make requestFullscreen infalliable or repeatable when we need it to continue the test.

Flags: needinfo?(bwerth)

It's hard to tell without a more detailed promise rejection, but it seems possible that the pattern:

container.appendChild(child);
child.requestFullscreen();

as used in file_fullscreen-api.html and elsewhere is sensitive to one of the failure conditions of Document::FullscreenElementReadyCheck.

And indeed the test is timing out the fullscreen request at this point with the promise rejection reason of FullscreenDeniedNotInDocument.

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

And indeed the test is timing out the fullscreen request at this point with the promise rejection reason of FullscreenDeniedNotInDocument.

I'm probably wrong about this. That error is an expected error from the previous part of the test, the exit3 function which intentionally requests fullscreen on an out-of-document element. Still looking...

With an annotated build, it appears that we're getting a call to Document::RemoteFrameFullscreenReverted after the appropriately-rejected fullscreen request in exit3, and that is causing the requestFullscreen to not be successfully applied. I'll try to figure out why that call is happening and how to stop it.

I'm making some progress on this locally, but there's a common calling pattern that breaks the test and I'm not sure what to do about it yet. This shows up in Linux nofis runs in sequences where the test has exited fullscreen and then entered fullscreen again -- as it does for nearly every transition between tasks in the test:

  1. nsGlobalWindowOuter::ProcessWidgetFullscreenRequest tries to leave fullscreen, calling MakeWidgetFullscreen which sets up a transition runnable.
  2. Document::RequestFullscreenInParentProcess calls nsGlobalWindowOuter::SetFullscreenInternal to enter fullscreen.
  3. AppWindow::SizeModeChanged decides it's not in fullscreen (due to the transition) and calls nsGlobalWindowOuter::SetFullscreenInternal to exit fullscreen.

So the end result is that the exiting of fullscreen has a sync part and an async part, and the entering of fullscreen gets inserted between them, having its effect overwritten, thus the test times out. Possible fixes:

  1. Make the fullscreenchange event fire after the sizemodechange event has been received. This would prevent the test from trying to enter fullscreen when the exit fullscreen is still being processed.
  2. Make the fullscreen enter request queue up behind the other transition runnable which would cause it to run after the exit is complete.

Tricky stuff, I'll probably try both approaches and see which one I can get working without breaking a lot of other behavior.

Edgar, I'd like to hear your thoughts on my theory about the root cause in comment 21. Does this seem correct to you? If so, is there something already in the window fullscreen code that should handle this case and isn't working correctly?

Flags: needinfo?(echen)

There's a possibility that this can be fixed by only executing this clause when the DOMFullscreenChild was the source of the request. This DOMFullscreen:Exit message sent to the parent is sometimes the transmission vector of an untimely request to exit fullscreen. Since it's only necessary to be sent when the child has exited fullscreen and the parent hasn't, we really don't want the parent to get any more "please exit fullscreen" messages than are absolutely necessary. It's not a semantically strong fix, but it's small and it may be sufficient.

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

There's a possibility that this can be fixed by only executing this clause when the DOMFullscreenChild was the source of the request.

Try run revealed this is not sufficient.

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

Edgar, I'd like to hear your thoughts on my theory about the root cause in comment 21. Does this seem correct to you? If so, is there something already in the window fullscreen code that should handle this case and isn't working correctly?

Your theory is correct. I also noticed that some intermittent failures are caused by the transition effect, especially in test-verify mode. That's why fullscreen tests typically run without transitions, as in test_fullscreen-api.html. So, I think the issue isn't related to transitions.

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

Make the fullscreenchange event fire after the sizemodechange event has been received. This would prevent the test from trying to enter fullscreen when the exit fullscreen is still being processed.

The fullscreenchange event is fired from FinishFullscreenChange which occurs after the sizemodechange event on Linux (and also on most of platform).

One possible case is that multiple sizemodechange events are triggered while exiting fullscreen. The first sizemodechange event triggers the fullscreenchange, allowing the test to continue and request fullscreen. However, the second sizemodechange event then arrives, causing the fullscreen state to be reset. But I am not sure if that is the case.

Flags: needinfo?(echen)

(Originally the test wait for resize event which might delay the test a bit, so the timing might be different)

Hmm, in comment 21 I was incorrect in stating that the call to MakeWidgetFullscreen sets up a transition runnable. It doesn't, but calls through to nsWindow::MakeFullScreen, which on Linux calls gtk_window_unfullscreen. Later, when the window gets the window state event, it sends the sizemodechange event, which arrives after the window has been put in fullscreen, causing a timeout. It's still asynchronous and leads to the same outcome as the slightly-wrong explanation in comment 21. Importantly, it can't be fixed by turning off transitions in the test, as is already done in this test.

I think the ideal would be to delay the fullscreenchange event until after the sizemodechange event has been received. I'll see if I can do it.

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

I think the ideal would be to delay the fullscreenchange event until after the sizemodechange event has been received. I'll see if I can do it.

Sorry, I still don't get it, Isn't the FullscreenExit added to the PendingFullscreenChangeList in https://searchfox.org/mozilla-central/rev/1157486abdf3245c2d3bf425745314c090745de5/dom/base/Document.cpp#14489, waiting for widgets exit fullscreen first then firing fullscreenchange? The pending FullscreenChange should resume after FinishFullscreenChange, which is expected to occurs after the sizemodechange, right?

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

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

I think the ideal would be to delay the fullscreenchange event until after the sizemodechange event has been received. I'll see if I can do it.

Sorry, I still don't get it...

There may be other issues, but one of the possible timeouts is that the subtest enter4 exits fullscreen by removing the fullscreenElement, which calls Element::UnbindFromTree which calls Document::ExitFullscreenInDocTree in the child process. Since that calls ResetFullscreen, the fullscreenChange event is sent from the child before the parent has left fullscreen, which happens later when DOMFullscreenChild notifies the parent. At least in this test, the fullscreenchange event sent from the child is enough to advance the next part of the test, which calls requestFullscreen. That can fail in the way described in comment 21, or it can fail in Document::fullscreenElementReadyCheck in a way that I've patched out in my local build.

So in this case the child fullscreenchange event should be sent only when the parent has finished exiting fullscreen also.

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

Isn't the FullscreenExit added to the PendingFullscreenChangeList in https://searchfox.org/mozilla-central/rev/1157486abdf3245c2d3bf425745314c090745de5/dom/base/Document.cpp#14489, waiting for widgets exit fullscreen first then firing fullscreenchange?

I can't find anything that is waiting for widgets to exit fullscreen, not anywhere. It seems to me that items in the PendingFullscreenChangeList are only executed serially of the same type (all enters iterated in HandlePendingFullscreenRequests and elsewhere, all exits in ExitFullscreenInDocTree and nowhere else) which makes intermingling of the enters and exits indeterminate. Also, the iteration of the exits in ExitFullscreenInDocTree doesn't actually exit fullscreen, it just resolves the promises.

I think the implementation of fullscreen in AppWindow and Document needs a comprehensive review, which the test test_fullscreen-api.html can't provide. Or perhaps is providing right now, given that the intermittency in the test as constructed is revealing intermittency in the underlying implementation (comment 21, comment 29, comment 30). But since my goal is to make the test intermittency go away, I'm now going to focus on making the test more resilient.

See Also: → 1826645

Sorry, pasted try run in wrong Bug.

status-firefox111: --- → fixed
status-firefox112: --- → fixed
status-firefox113: --- → fixed
status-firefox-esr102: --- → fixed
status-thunderbird_esr102: --- → fixed
status-thunderbird_esr91: --- → fixed

This needs the changes in Bug 1826645 to be landable.

Depends on: 1826645
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6f97d0d304d
Make fullscreen and pointerlock mochitests use document.fullscreenElement to determine fullscreen-ness. r=edgar

Backed out for causing mochitest failures on test_fullscreen-api.html

Backout link

Push with failures

Failure log

Flags: needinfo?(bwerth)
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cb5a699aa8d8
Make fullscreen and pointerlock mochitests use document.fullscreenElement to determine fullscreen-ness. r=edgar

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

Status: NEW → RESOLVED
Closed: 1 year ago
status-firefox114: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 114 Branch
Flags: needinfo?(bwerth)
See Also: → 1833142
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: