video.js - infinite recursion with unprefixed Fullscreen API

[upsuper]

URL: https://github.com/videojs/video.js
Browser / Version: Firefox 47+ / Microsoft Edge
Operating System:
Problem type: Something else - I'll add details below

Video.js (in the url) has a bug in version 4.7~4.12 that it would trigger an infinite recursion in handler of fullscreenchange event if unprefixed Fullscreen API is available.

(Before 4.7, they were detecting unprefixed API via a leagcy name which no one ever uses. After 5.0, they fix this issue via changing their internal mechanism somehow.)

Browsers all have some kind of recursion limiting mechanism, so this doesn't break functionality significantly, but it could make fullscreen change very slow.

Steps to Reproduce

1. Navigate to sites use an old version of video.js, e.g. http://sho.co/embed/16VID
2. Click the fullscreen button

Expected Behavior:
It enters fullscreen immediately.

Actual Behavior:
It could block the browser for up to several seconds before the content is eventually in fullscreen. And in Firefox, a line "too much recursion" would be printed in the console.

From webcompat.com with ❤️

[upsuper]

cc @ehsan @annevk @foolip @tantek

[annevk]

I'm not sure what I can do to help here. I'm guessing you don't want to change the API?

[foolip]

Yeah, I have been made aware of this problem in Edge before. Do all site that have this problem still function correctly after the "too much recursion" bailout, or are there sites that have code after that that fails to run, breaking the page?

[upsuper]

To be honest, I didn't find many examples of this issue. I searched in Google's BigQuery (in the requests table), but most of the results are referred from an index page which doesn't actually use this library. So I cannot tell whether the page would be broken.

But given that we still have proper style applied, and this issue doesn't happen for unfullscreen, it shouldn't be too bad.

There are several potential solutions we discussed today:

1. Make exitFullscreen in document return false to fool their feature detection (not sure whether it is feasible, though).
2. Ask Video.js to fix all affected versions from their CDN, so that most sites would be fine. This shouldn't be too complicated for them to fix, they could just break their feature detection like what was done before 4.7 (they detected cancelFullScreen rather than exitFullscreen at that time) which should not break anything.

From the implementation side, we are considering limiting the stack size for handler of fullscreenchange to make it fail faster. (See our bug 1268794)

[foolip]

Replacing the copies in CDN is an interesting idea, have you talked to the video.js developers about it?

[upsuper]

I haven't. Probably we can page them here?

cc @gkatsev @heff: Would you consider changing copies in CDN to make this issue less serious?

[upsuper]

@foolip Do you have any plan to mitigate this issue in Blink before shipping unprefixed Fullscreen API? It seems to me this is the biggest single blocker. Gecko may try to limit the stack size as I said in some previous comment, and see what will happen.

[foolip]

I definitely need to test how it will behave as step zero, so I've filed https://bugs.chromium.org/p/chromium/issues/detail?id=620082

[karlcow]

I wonder what is the status of this issue 1year+

[foolip]

I've tested http://sho.co/embed/16VID with Chrome Canary with the unprefixed Fullscreen API enabled and did see "Maximum call stack size exceeded". It doesn't break the functionality of this site though. There is an Intent to Ship: Unprefixed Fullscreen API with FullscreenOptions on blink-dev, and we may run into this issue in the wild and see cases where the effects are more serious.

[foolip]

Based on @upsuper's 4.7-5.0 range, I did a search for 'Video.js v4.' in httparchive. There are 1519 matches. Probably not all of them are 4.7+, but most that I looked at are. In other words, this code is still fairly widespread. How often it results in noticeable breakage remains to be seen.

Details: on http://sho.co/embed/16VID the version used is 4.12.15, and the code added in videojs/video.js@753ce48#diff-3b0266ff1c037b289ec624ab25b0272eR608 (first in v4.0.3) is part of the code that I see on the stack. But as @upsuper says it wasn't reached at first because of a div.cancelFullscreen !== undefined, which remained until videojs/video.js@46aa551

[softvision-oana-arbuzov]

For me on Firefox the video enters full screen immediately but still displayed the "to much recursion" error message in the console.

On Chrome I also receive "Uncaught RangeError: Maximum call stack size exceeded" error message when entering full screen.

Tested with:
Browser / Version: Firefox Nightly 70.0a1 (2019-08-21), Chrome 76.0.3809.100
Operating System: Windows 10 Pro

@upsuper can you still reproduce the initial issues ?

[upsuper]

It seems that the fullscreen preceeds quickly now for me as well. I don't know what has been changed, but it seems that too deep recursion is no longer a problem?

[softvision-oana-arbuzov]

Thanks @upsuper, I will close it as fixed. If you will encounter the issue in the future feel free to reopen this.