1575630 - The ordering between DOMFullscreen:Entered and UpdateDimensions is not enforced

Status: NEW

(Core :: DOM: Events, defect, P3)

Description

[Markus Stange [:mstange]]

test_videocontrols.html has a subtest at the end which assumes that, at the time at which the mozfullscreenchange event fires in the content process, the content document covers the entire screen.

However, this is not necessarily the case: Content process documents only learn about their size via UpdateDimensions messages, and those are dispatched by the parent from nsSubDocumentFrame::ReflowFinished, so they only happen if the parent process window document is reflown.
The parent process tells the content process about the full screen change with a DOMFullscreen:Entered message, which is sent from a MozDOMFullscreen:Entered event handler, which is called during the parent process's Document::ApplyFullscreen call.

In the past, on macOS, in the parent process, Document::ApplyFullscreen always ran after a reflow of the parent process window document, because Document::ApplyFullscreen runs after nsBaseWidget::InfallibleMakeFullScreen, which triggers a window resize, and in the past, window resizes would cause synchronous paints, and the synchronous paint flushed layout and triggered the reflow.
After bug 1574538, there will not be a synchronous paint during window resize operations. The paint will happen asynchronously. (The screen still updates atomically though.)

On Linux, resizes always cause asynchronous paints, as far as I know. And bug 1514060 shows that test_videocontrols.html was already failing intermittently, almost exclusively on Linux.

What should we do here? Relying on synchronous paints during window resizes is not a good idea in my opinion. I think we have two options:

• Enforce the ordering between UpdateDimensions and DOMFullscreen:Entered, for example by flushing layout in the parent process before sending DOMFullscreen:Entered to content.
• Do not enforce any ordering, and make content deal with the fact that fullscreen may fire at a time at which the document still has the old size.

I prefer the first option, but it's slightly risky. It also means we may end up reflowing the parent process window document twice, which runs counter to bug 1267568. (Unless mechanisms from bug 1267568 prevent a double reflow, I'm not sure about that.)

Until this bug is fixed, I'll try to work around this issue in the test, by using the document size instead of the screen size.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1575630 - Flush layout in ApplyFullscreen before dispatching MozDOMFullscreen:Entered so that content process documents receive their full screen size before they receive DOMFullscreen:Entered. r=xidorn,emilio

Comment 2

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=73a44e7eacda60fdf92ef50dc8edeebf743644ec

Comment 3

[Markus Stange [:mstange]]

Looks like this patch on its own doesn't fix test_videocontrols.html on Linux: There's still a failure on a Linux64 asan job.

Comment 4

[Markus Stange [:mstange]]

The approach in this fix was no good. In the review Emilio had a better idea:

Would it be better to make MozDOMFullScreen:Entered more async, like either DispatchFullscreenChange (which uses the refresh driver), or DispatchFullscreenNewOriginEvent (which posts to the event loop via AsyncEventDispatcher).

(This would give us a safer place to flush. And if we ran this event from the refresh driver after the layout flush phase, we'd already have flushed layout.)

I'm going to unassign this bug from myself for now, because my immediate problem is fixed by the workaround in the test, but I may pick this up again in the future.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Regardless of the internal ordering, I indeed want to ensure that when fullscreenchange event comes, the window that content see has been resized (but not yet reflowed). I had put lots effort to minimize the number of reflows happen in the content when going fullscreen.

So, yeah I think it makes sense to ensure that UpdateDimensions should come before fullscreen notification to content.

I'm unsure about CoreAnimation, but the story in Linux is more complicated than that. Basically on Linux there is no way you can reliable know when the window state has been changed to fullscreen and the window has been resized. See discussion in bug 1254448 as well as the related gtk bug I filed for that.

As far as it's not the case for CoreAnimation, and we can still know when the window state is stable, I think it's possible to guarantee the state content observes.

Comment 6

[Markus Stange [:mstange]]

Would it make sense to update dimensions and fullscreen state atomically, with the same message? Maybe renaming UpdateDimensions to UpdateDimensionProperties with size+fullscreenstate+sizemode would work.

Comment 7

[Chris Peterson [:cpeterson]]

Moving to "DOM: Events" component (though most of the fullscreen logic is implemented in frontend code).