Open Bug 1575630 Opened 5 years ago Updated 2 years ago

The ordering between DOMFullscreen:Entered and UpdateDimensions is not enforced

Categories

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

defect

Tracking

()

People

(Reporter: mstange, Unassigned, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(1 obsolete file)

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.

Flags: needinfo?(xidorn+moz)

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.

Attachment #9087165 - Attachment is obsolete: true

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.

Assignee: mstange → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(xidorn+moz)
Flags: needinfo?(xidorn+moz)

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.

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.

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

Component: DOM: Content Processes → DOM: Events
Priority: -- → P3
Flags: needinfo?(smacleod)
Flags: needinfo?(smacleod)
Severity: normal normal → S3 S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: