Closed Bug 1708324 Opened 2 months ago Closed 1 month ago

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

Categories

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

Unspecified
macOS
defect

Tracking

()

RESOLVED FIXED
90 Branch
Fission Milestone M7a
Tracking Status
firefox-esr78 --- disabled
firefox88 --- disabled
firefox89 --- disabled
firefox90 --- fixed

People

(Reporter: mccr8, Assigned: haik)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

  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/

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/

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.

Blocks: fission
Severity: -- → S3
Priority: -- → P3

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

Blocks: fission-dogfooding
No longer blocks: fission
Fission Milestone: ? → M7a
Flags: needinfo?(echen)
Severity: S3 → S2
Priority: P3 → P2

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?

Assignee: nobody → echen
OS: Unspecified → macOS

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.

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.

Flags: needinfo?(echen)

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.

(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!

Flags: needinfo?(haftandilian)
See Also: → 1704452

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.

(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.

(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.
Flags: needinfo?(haftandilian)

(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!

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!

Flags: needinfo?(haftandilian)

WIP - ignore occlusion events during the fullscreen transition.

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.

Flags: needinfo?(haftandilian)

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? :)

Component: DOM: Core & HTML → Widget: Cocoa
Flags: needinfo?(haftandilian)
Attachment #9221119 - Attachment is obsolete: true
Duplicate of this bug: 1704452

(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.

Assignee: echen → haftandilian
Flags: needinfo?(haftandilian)

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

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.

Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c381a3500a2
[Fission] Full screen button for YouTube video embedded on Reddit doesn't work r=spohl
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
Attachment #9221440 - Attachment is obsolete: true

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.

Attachment #9223893 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.