1708324 - [Fission] Full screen button for YouTube video embedded on Reddit doesn't work

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Andrew McCreight [:mccr8]]

1. Visit this page: https://www.reddit.com/r/movies/comments/n0e9x2/the_tomorrow_war_july_2nd_prime_video/
2. Click on the video so that it starts playing.
3. Click on the square in the right side of the video controls.

Expected behavior (seen in a non-Fission window): The video goes full-screen.
Actual behavior: The screen goes black, like it is transitioning into full-screen, but then I'm just back on the page like I hadn't clicked on anything.

I see the message in the browser console with Fission enabled (which I don't see in a non-Fission window):
Error: TelemetryStopwatch: key "FULLSCREEN_CHANGE_MS" was already initialized DOMFullscreenParent.jsm:136:28
handleEvent resource:///actors/DOMFullscreenParent.jsm:136

I haven't noticed issues with embedded full screen videos for a while, so I'm not sure what the difference is here. Trying to make it go full screen a bunch of times, it did work like maybe 1 out of 5 times.

Oddly enough, if I follow the same steps on this page, which also has a YouTube embed, it works just fine (including no error message): https://www.reddit.com/r/netflix/comments/n0frru/hit_run_official_teaser_netflix/

Comment 1

[Andrew McCreight [:mccr8]]

Here's another page where the full screen doesn't work if you want a more wholesome trailer to watch the first 4 seconds of over and over:
https://www.reddit.com/r/movies/comments/n0ea1j/disney_and_pixars_luca_official_trailer_disney/

Comment 2

[Andrew McCreight [:mccr8]]

Looking at the code, I'm guessing the TelemetryStopwatch error is because we got MozDOMFullscreen:Entered and MozDOMFullscreen:Exited without an intervening DOMFullscreen:Painted (because we popped in and out without actually showing anything full screen?), so maybe that's just a symptom rather than the cause of the error.

Comment 3

[Neha Kochar [:neha]]

Edgar, can you please debug this embedded fullscreen issue? I was able to reproduce this using MacOS.

Comment 4

[Chris Peterson [:cpeterson]]

Edgar, this is a new Fission fullscreen bug that sounds pretty serious.

This might be macOS-specific. In the Fission bug triage meeting, people running macOS can reproduce and people Windows and Linux can't. Perhaps this is related to the new native fullscreen code for macOS?

Comment 5

[Neha Kochar [:neha]]

It's also weird that when the the screen goes black, like it is transitioning into full-screen, and then it returns to the same page (non-fullscreen), it also makes the whole browser window fullscreen. So, maybe the embedded video fullscreen is somehow wrongly triggering the window fullscreen.

Comment 6

[Edgar Chen [:edgar]]

I can easily reproduce this on Nightly build, but the reproduce rate is not that high on my local build (even on non-debug build).

When issue happens, we run into https://searchfox.org/mozilla-central/rev/49e0a928390a8013a4eb08b7b3bbff025f01913a/browser/actors/DOMFullscreenChild.jsm#28-37 where DOMFullscreenChild exit the fullscreen right after receiving DOMFullscreen:Entered, it seems the fullscreen state is somehow cleared, need to check why.

Comment 7

[Edgar Chen [:edgar]]

Yeah, I think this is macOS-specific. Only macOS updates the occlusion state for now, https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/widget/cocoa/nsCocoaWindow.mm#2039-2055.

When issue happens, the occlusion state is flipped back and forth, and if the occlusion state is not yet stable and the embedded get the response from it's parent to contintue the fullscreen steps which will check the active state again, then it fails at https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/dom/base/Document.cpp#14554-14557 and exit the fullscreen.

It doesn't clear to me what cause the occlusion state changes, the fullscreen seems not to always make occlusion state fliping back and forth. But I think we could do something like what pointerlock does currently: only check the active state on initial check.

Comment 8

[Edgar Chen [:edgar]]

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

It doesn't clear to me what cause the occlusion state changes, the fullscreen seems not to always make occlusion state fliping back and forth.

Hi Haik, do you have any idea what would possible cause occlusion state filping while widget goes into fullscreen mode.
The occlusion changes is from https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/widget/cocoa/nsCocoaWindow.mm#2909-2913.
Thanks!

Comment 9

[Edgar Chen [:edgar]]

Attachment: WIP: Bug 1708324 - Don't check if document is in active tab again when we already pass the initial check;

On Mac OS, when wiget switch to fullscreen mode, the occlusion state might be
flipped back and forth. And checking the active state of BrowsingContext might
fail if it happends when the occlusion state isn't flipped back yet. To avoid
this, we only check if document is in active tab on initial check, like what
we do for PointerLock.

Comment 10

[Haik Aftandilian [:haik]]

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

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

It doesn't clear to me what cause the occlusion state changes, the fullscreen seems not to always make occlusion state fliping back and forth.

Hi Haik, do you have any idea what would possible cause occlusion state filping while widget goes into fullscreen mode.
The occlusion changes is from https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/widget/cocoa/nsCocoaWindow.mm#2909-2913.
Thanks!

Hi, I don't know what is triggering the occlusion event. I have some tracing code setup to help debug fullscreen so I will take a look at this case.

I ran into a similar problem with bug 1563947 to use the native Mac fullscreen API for DOM fullscreen support (which I'm working on now). In that case, the Mac window NSWindow instance is getting occlusion callbacks during the transition to fullscreen causing the tab-is-focused check to fail. Depending on when the occlusion events are firing, you may be able to check nsCocoaWindow::mInFullScreenTransition and change the nsCocoaWindow callback to ignore the occlusion change during the transition. That is how I worked around the problem in my WIP code.

I thought the occlusion events were driven by macOS as a quirk during native fullscreen transition, but if it is occurring for non-native too, it could be caused by something Firefox is doing.

Comment 11

[Haik Aftandilian [:haik]]

(In reply to Haik Aftandilian [:haik] from comment #10)

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

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

It doesn't clear to me what cause the occlusion state changes, the fullscreen seems not to always make occlusion state fliping back and forth.

Hi Haik, do you have any idea what would possible cause occlusion state filping while widget goes into fullscreen mode.
The occlusion changes is from https://searchfox.org/mozilla-central/rev/6371054f6260a5f8844846439297547f7cfeeedd/widget/cocoa/nsCocoaWindow.mm#2909-2913.
Thanks!

Hi, I don't know what is triggering the occlusion event. I have some tracing code setup to help debug fullscreen so I will take a look at this case.

When we transition to fullscreen we create a new window for the animation in nsCocoaWindow::PrepareForFullscreenTransition() for the black screen effect (i.e., fill the screen with black temporarily when going in/out of fullscreen)[1]. So it follows that we would receive occlusion events because the existing window is fully occluded for some period of time.

If it makes sense to solve this at the widget layer, having Mac widget code ignore the occlusion events during that time sounds reasonable to me. And I think it's appropriate to fix there because the occlusion event is a side effect of the Mac-specific implementation.

1. This might change when we transition to using native fullscreen in bug 1563947.

Comment 12

[Edgar Chen [:edgar]]

(In reply to Haik Aftandilian [:haik] from comment #11)

If it makes sense to solve this at the widget layer, having Mac widget code ignore the occlusion events during that time sounds reasonable to me.

Yeah, I think it makes sense to solve this at widget layer, I will prepare patch for that. Thanks for your help!

Comment 13

[Edgar Chen [:edgar]]

I tried to add a check for mInFullScreenTransition in https://searchfox.org/mozilla-central/rev/cca1566127a2fcc013e9c09f9d90ed70df2250a4/widget/cocoa/nsCocoaWindow.mm#2047, something like

if (mInFullScreenTransition || mIsFullyOccluded == newOcclusionState) {
  return;
}

But it doesn't work since the occlusion events happens after nsCocoaWindow::DoMakeFullScreen call, maybe the event is asynchronous? Hi Haik, any suggestion? Or something I could try? Thanks!

Comment 14

[Haik Aftandilian [:haik]]

Attachment: WIP: Bug 1708324 - WIP - [Fission] Full screen button for YouTube video embedded on Reddit doesn't work

WIP - ignore occlusion events during the fullscreen transition.

Comment 15

[Haik Aftandilian [:haik]]

I added an implementation of CleanupFullscreenTransition() in nsCocoaWindow and tested with this using PrepareForFullscreenTransition() and CleanupFullscreenTransition() to start/stop ignoring occlusion events. I can't reproduce the problem with the fix but it should be tested more.

Comment 16

[Edgar Chen [:edgar]]

Nice, after applying the patch I could not reproduce (tried over 20 times), I used to reproduce this problem very easy with https://www.reddit.com/r/movies/comments/n0ea1j/disney_and_pixars_luca_official_trailer_disney/. Thanks!!!

Haik, since your patch works, would you mind taking this bug then? :)

Comment 18

[Haik Aftandilian [:haik]]

(In reply to Edgar Chen [:edgar] (OOO ~ 05/19) from comment #16)

Haik, since your patch works, would you mind taking this bug then? :)

No problem.

Comment 19

[Haik Aftandilian [:haik]]

To add a bit more to Edgar's evaluation on comment 7, the reason the problem is intermittent is that Document::FullscreenElementReadyCheck() runs in the child process racing with the parent process executing the FullscreenTransitionTask.

In the child, first nsDOMWindowUtils::RemoteFrameFullscreenChanged() runs which sends an async message to the parent which eventually runs nsDOMWindowUtils::RemoteFrameFullscreenChanged() and ends up in MakeWidgetFullscreen() which enqueues the FullscreenTransitionTask runnable.

In the failure case, we have a tracing log like this:

CPU FUNCTION                                                                                  PID
  6  -> nsCocoaWindow::DispatchOcclusionEvent()                                               2990    
  6    -> mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool)             2990    Occluded
  6    <- mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool)             2990    
  6  <- nsCocoaWindow::DispatchOcclusionEvent()                                               2990    
  4    -> mozilla::dom::Document::FullscreenElementReadyCheck(mozilla::FullscreenRequest&)    3000    
  4    <- mozilla::dom::Document::FullscreenElementReadyCheck(mozilla::FullscreenRequest&)    3000    
  4  -> nsCocoaWindow::DoMakeFullScreen(bool, bool)                                           2990
  4  <- nsCocoaWindow::DoMakeFullScreen(bool, bool)                                           2990
  7  -> nsCocoaWindow::DispatchOcclusionEvent()                                               2990    
  7    -> mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool)             2990    Unoccluded
  7    <- mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool)             2990    
  7  <- nsCocoaWindow::DispatchOcclusionEvent()                                               2990    

Good case:

CPU FUNCTION                                                                                  PID
  5  -> nsCocoaWindow::DispatchOcclusionEvent()                                               2990
  5    -> mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool)             2990    Occluded
  5    <- mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool)             2990    
  5  <- nsCocoaWindow::DispatchOcclusionEvent()                                               2990
  5  -> nsCocoaWindow::DispatchOcclusionEvent()                                               2990
  5    -> mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool)             2990    Unoccluded
  5    <- mozilla::AppWindow::WidgetListenerDelegate::OcclusionStateChanged(bool)             2990
  5  <- nsCocoaWindow::DispatchOcclusionEvent()                                               2990
  4  -> nsCocoaWindow::DoMakeFullScreen(bool, bool)                                           2990
  4  <- nsCocoaWindow::DoMakeFullScreen(bool, bool)                                           2990
  4  -> mozilla::dom::Document::FullscreenElementReadyCheck(mozilla::FullscreenRequest&)      3000
  4  <- mozilla::dom::Document::FullscreenElementReadyCheck(mozilla::FullscreenRequest&)      3000

Comment 20

[Haik Aftandilian [:haik]]

Attachment: Bug 1708324 - [Fission] Full screen button for YouTube video embedded on Reddit doesn't work r?spohl

During fullscreen transitions on Mac, ignore occlusion events caused by the widget DOM fullscreen transition effect which uses a temporary window.

Add a test that attempts to enter fullscreen from a non-focused window. This test is to ensure the fix (and future fixes) do not regress the focus requirement for fullscreen.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c381a3500a2
[Fission] Full screen button for YouTube video embedded on Reddit doesn't work r=spohl

Comment 22

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8c381a3500a2

Comment 23

[Edgar Chen [:edgar]]

Attachment: Bug 1708324 - Exit fullscreen only when the focus change will raise the window;

Comment 24

[Phabricator Automation]

Comment on attachment 9223893 [details]
Bug 1708324 - Exit fullscreen only when the focus change will raise the window;

Revision D116182 was moved to bug 1712038. Setting attachment 9223893 [details] to obsolete.