Intermittent dom/tests/mochitest/pointerlock/test_pointerlock-api.html | single tracking bug
Categories
(Core :: DOM: Core & HTML, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox114 | --- | fixed |
People
(Reporter: jmaher, Assigned: bradwerth)
References
Details
(Keywords: intermittent-failure, intermittent-testcase)
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
Reporter | ||
Comment 1•2 years ago
|
||
Additional information about this bug failures and frequency patterns can be found by running: ./mach test-info failure-report --bug 1776738
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 11•2 years ago
|
||
I would like this fixed before landing more changes to macOS native fullscreen.
Assignee | ||
Comment 12•2 years ago
|
||
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.
Assignee | ||
Comment 13•2 years ago
|
||
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.
Comment 14•2 years ago
|
||
Comment 15•2 years ago
|
||
Backed out for causing mochitest failures in test_fullscreen-api.html
- Backout link
- Push with failures
- Failure Log
- Failure line: TEST-UNEXPECTED-FAIL | dom/base/test/fullscreen/test_fullscreen-api.html | Test timed out. -
Assignee | ||
Comment 16•2 years ago
|
||
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.
Assignee | ||
Comment 17•2 years ago
|
||
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
.
Assignee | ||
Comment 18•2 years ago
|
||
And indeed the test is timing out the fullscreen request at this point with the promise rejection reason of FullscreenDeniedNotInDocument
.
Assignee | ||
Comment 19•2 years ago
|
||
(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...
Assignee | ||
Comment 20•2 years ago
|
||
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.
Assignee | ||
Comment 21•2 years ago
|
||
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:
nsGlobalWindowOuter::ProcessWidgetFullscreenRequest
tries to leave fullscreen, callingMakeWidgetFullscreen
which sets up a transition runnable.Document::RequestFullscreenInParentProcess
callsnsGlobalWindowOuter::SetFullscreenInternal
to enter fullscreen.AppWindow::SizeModeChanged
decides it's not in fullscreen (due to the transition) and callsnsGlobalWindowOuter::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:
- Make the
fullscreenchange
event fire after thesizemodechange
event has been received. This would prevent the test from trying to enter fullscreen when the exit fullscreen is still being processed. - 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.
Assignee | ||
Comment 22•2 years ago
|
||
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?
Assignee | ||
Comment 23•2 years ago
|
||
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.
Assignee | ||
Comment 24•2 years ago
|
||
(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.
Comment 25•2 years ago
|
||
(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.
Comment 26•2 years ago
|
||
(Originally the test wait for resize
event which might delay the test a bit, so the timing might be different)
Assignee | ||
Comment 27•2 years ago
|
||
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.
Comment 28•2 years ago
|
||
(In reply to Brad Werth [:bradwerth] from comment #27)
I think the ideal would be to delay the
fullscreenchange
event until after thesizemodechange
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?
Assignee | ||
Comment 29•2 years ago
|
||
(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 thesizemodechange
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.
Assignee | ||
Comment 30•2 years ago
|
||
(In reply to Edgar Chen [:edgar] from comment #28)
Isn't the
FullscreenExit
added to thePendingFullscreenChangeList
in https://searchfox.org/mozilla-central/rev/1157486abdf3245c2d3bf425745314c090745de5/dom/base/Document.cpp#14489, waiting for widgets exit fullscreen first then firingfullscreenchange
?
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.
Assignee | ||
Comment 31•2 years ago
|
||
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.
Assignee | ||
Updated•2 years ago
|
Assignee | ||
Comment 32•2 years ago
|
||
Sorry, pasted try run in wrong Bug.
Updated•2 years ago
|
Assignee | ||
Comment 33•2 years ago
|
||
This needs the changes in Bug 1826645 to be landable.
Comment 34•2 years ago
|
||
Comment 35•2 years ago
|
||
Backed out for causing mochitest failures on test_fullscreen-api.html
Comment 36•2 years ago
|
||
Comment 37•2 years ago
|
||
bugherder |
Assignee | ||
Updated•2 years ago
|
Comment hidden (Intermittent Failures Robot) |
Updated•2 years ago
|
Description
•