Dispatching fullscreenchange event whilst entering fullscreen causes "InternalError: too much recursion"

VERIFIED FIXED in Firefox 53

Status

()

Core
DOM: Events
VERIFIED FIXED
9 months ago
8 months ago

People

(Reporter: tomasz.oponowicz, Assigned: xidorn)

Tracking

({regression})

47 Branch
mozilla54
regression
Points:
---

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52+ wontfix, firefox53+ verified, firefox54+ verified)

Details

(Whiteboard: [webcompat])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

9 months ago
str
Created attachment 8828915 [details]
index.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36

Steps to reproduce:

Firefox (tested against Firefox 50) crashes with `InternalError: too much recursion` when dispatching `fullscreenchange` event whilst entering fullsceen mode.

Define empty event handlers in order to avoid crash:

  document.addEventListener('fullscreenchange', function() {});

Test case:
1. Open `index.html` (please find file attached) in Firefox;
1. Press _Enter fullscreen_;



Actual results:

Firefox enters fullscreen mode and throws `InternalError: too much recursion` in the console.


Expected results:

Firefox enters fullscreen mode without any error.

Comment 1

9 months ago
The same error is displayed when exiting FS mode too.

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=666bfc7f00ff8b04faab18285bb046817b1d1c90&tochange=33509142de5fe1f6357006c391de85d7a6f11943

Xidorn Quan — Bug 1273468 - Revert video controls to use prefixed Fullscreen API again. r=dolske
Blocks: 1273468
Status: UNCONFIRMED → NEW
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
Component: Untriaged → Video/Audio Controls
Ever confirmed: true
Flags: needinfo?(xidorn+moz)
Keywords: regression
OS: Unspecified → All
Product: Firefox → Toolkit
Hardware: Unspecified → All
Version: 50 Branch → 47 Branch
It is not a regression from bug 1273468, but from bug 743198 where the unprefixed fullscreen events were added. The issue here is that, we unconditionally dispatch unprefixed event to handler of prefixed event, but we should only dispatch trusted ones.

This also affects webkit events handled there.
Assignee: nobody → xidorn+moz
Component: Video/Audio Controls → DOM: Events
Flags: needinfo?(xidorn+moz)
Product: Toolkit → Core
Comment hidden (mozreview-request)
Attachment #8829318 - Flags: review?(amarchesini) → review?(masayuki)
status-firefox50: affected → wontfix
Comment on attachment 8829318 [details]
Bug 1332699 - Don't check handler of legacy version if the event is not trusted.

https://reviewboard.mozilla.org/r/106432/#review107740

I agree with not dispatching legacy events when the original event is not trusted event.

Although, if this is chrome script, we still have this bug with this patch, perhaps, we cannot distinguish them under current design.

But I'd like to ask this smaug too.
Attachment #8829318 - Flags: review?(masayuki) → review+
Smaug, if you don't agree or have some comments about this change, let us know, but you look like too busy now. So, perhaps, if we don't get your reply for a couple of days, this patch can go and file a follow up bug for your concern if there are.
Flags: needinfo?(bugs)
(In reply to tomasz.oponowicz from comment #0)
> Firefox (tested against Firefox 50) crashes with `InternalError: too much
> recursion` when dispatching `fullscreenchange` event whilst entering
> fullsceen mode.
> 
> Define empty event handlers in order to avoid crash:
No crash anywhere.
Do other browsers check isTrusted in case of webkit* events?
Last time I checked blink's code, they didn't, IIRC.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (review queue closed until backlog cleared) from comment #7)
> Do other browsers check isTrusted in case of webkit* events?
> Last time I checked blink's code, they didn't, IIRC.

They don't... and this is pretty much a unspeced area, even in webcompat spec...

(BTW, their event handler is case-sensitive, that says, listening on webkittransitionend is useless, you have to listen on webkitTransitionEnd, but webkittransitionend works in Gecko...)

So the issue here is that people want to dispatch a unprefixed fullscreenchange event if they get a prefixed mozfullscreenchange event, but if we invoke the mozfullscreenchange handler when handling untrusted unprefixed fullscreenchange event, infinite loop would happen...

But I guess this isn't really an issue, because if people don't setup a handler for fullscreenchange event, why do they want to dispatch it at all...

If you are more concerned about the potential webcompat issue here, I guess we can close this issue as INVALID or WONTFIX. I don't think it's a good idea to have different behavior for different legacy events anyway.
Flags: needinfo?(bugs)
(Reporter)

Comment 9

9 months ago
Thanks your quick reply.

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #8)
> But I guess this isn't really an issue, because if people don't setup a
> handler for fullscreenchange event, why do they want to dispatch it at all...

The test case (i.e. file attached) was extracted from the fullscreen polyfill available in the Shaka player:

https://github.com/google/shaka-player/blob/v2.0.0/lib/polyfill/fullscreen.js#L83

...the idea is to unify events across browsers.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #8)
> (In reply to Olli Pettay [:smaug] (review queue closed until backlog
> cleared) from comment #7)
> > Do other browsers check isTrusted in case of webkit* events?
> > Last time I checked blink's code, they didn't, IIRC.
> 
> They don't... and this is pretty much a unspeced area, even in webcompat
> spec...
> 
> (BTW, their event handler is case-sensitive, that says, listening on
> webkittransitionend is useless, you have to listen on webkitTransitionEnd,
> but webkittransitionend works in Gecko...)
I don't understand this. Our event handlers and event listeners are case sensitive.
And they have https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Window.idl?q=onwebkittransitionend;&sq=package:chromium&l=192


 
> So the issue here is that people want to dispatch a unprefixed
> fullscreenchange event if they get a prefixed mozfullscreenchange event, but
> if we invoke the mozfullscreenchange handler when handling untrusted
> unprefixed fullscreenchange event, infinite loop would happen...
Right, and if we don't dispatch prefixed event, then if unprefixed is dispatched not because of some trusted event but for whatever other reason (testing?), then prefixed wouldn't be dispatched.

Could we add a horrible hack that if we're currently handling trusted prefixed event, if that causes untrusted un-prefixed event to be dispatched, we wouldn't dispatch yet another prefixed event
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #10)
> Could we add a horrible hack that if we're currently handling trusted
> prefixed event, if that causes untrusted un-prefixed event to be dispatched,
> we wouldn't dispatch yet another prefixed event

Detect the stack? That probably works...
But... yeah that sounds very hacky...
oh, hmm, edge doesn't seem to dispatch prefixed event from untrusted events.
Maybe we could follow that model after all.
Er, hmm, maybe it doesn't have fullscreenchange at all.
Perhaps mike has better ideas than what I said in comment 10.


One option... are we the only one to dispatch fullscreenchange?
Could we limit the isTrusted check for fullscreen related events only, report about the
buggy script in https://github.com/google/shaka-player/blob/v2.0.0/lib/polyfill/fullscreen.js#L83
and then try to remove the isTrusted later?
Flags: needinfo?(miket)
(In reply to Olli Pettay [:smaug] from comment #15)
> Perhaps mike has better ideas than what I said in comment 10.
> 
> One option... are we the only one to dispatch fullscreenchange?

We should not do this when unprefixed Fullscreen API is disabled anyway...

Chrome is still in-progress of implementing unprefixed Fullscreen API, and Edge should have disabled that due to various compatibility issues. I should probably reach them and discuss whether there is something we can do.

> Could we limit the isTrusted check for fullscreen related events only,
> report about the
> buggy script in
> https://github.com/google/shaka-player/blob/v2.0.0/lib/polyfill/fullscreen.
> js#L83
> and then try to remove the isTrusted later?

I wonder whether there is really any solution for them to avoid this?
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #8)
> (In reply to Olli Pettay [:smaug] (review queue closed until backlog
> cleared) from comment #7)
> > Do other browsers check isTrusted in case of webkit* events?
> > Last time I checked blink's code, they didn't, IIRC.
> 
> They don't... and this is pretty much a unspeced area, even in webcompat
> spec...

The relevant spec here is DOM, not compat: https://dom.spec.whatwg.org/#concept-event-listener-invoke (but yeah, no explicit mention of isTrusted-ness).

(In reply to Olli Pettay [:smaug] from comment #15)
> Perhaps mike has better ideas than what I said in comment 10.

That sounds like it will solve the problem, I'm not sure I have a better idea.

> One option... are we the only one to dispatch fullscreenchange?
> Could we limit the isTrusted check for fullscreen related events only,
> report about the
> buggy script in
> https://github.com/google/shaka-player/blob/v2.0.0/lib/polyfill/fullscreen.
> js#L83
> and then try to remove the isTrusted later?

That probably depends on how popular and widely deployed the shaka-player v2 and below are... but treating fullscreen differently than the existing webkitAnimation* + webkitTransitionEnd legacy hacks wrt isTrusted makes sense, I think. Those events should be dispatched whether they're trusted or not.
Flags: needinfo?(miket)
xidorn, what do you think of special casing fullscreen events. And then perhaps get that special case spec'ed if we don't get the script libraries fixed soon enough.
Flags: needinfo?(xidorn+moz)
OK, I don't like it, but I guess I can live with it... I wasn't aware that it is part of the DOM spec...
Attachment #8829318 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Flags: needinfo?(xidorn+moz)

Comment 21

9 months ago
mozreview-review
Comment on attachment 8831604 [details]
Bug 1332699 - Don't check handler of legacy version if the event is not trusted.

https://reviewboard.mozilla.org/r/108156/#review109220

There is nothing to like here. I mean, all the option are pretty horrible :(
Want to file a bug about that broken script?
Attachment #8831604 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #21)
> Want to file a bug about that broken script?

As I've mentioned before, I have no idea how this polyfill can be implemented in a different way... Unless there is a feasible solution, I don't think it makes much sense to bug that script...

I actually would prefer we change the DOM spec and make webkit-prefixed events also do similiar check... Generating those events in test script should be easy enough that we should not be blocked on that... Is there any reason the spec is written that way?
Anne, do you think we can change the DOM spec and make it check whether the event is trusted before we fallback to listeners of legacy event type? Is there webcompat concerns there?
Flags: needinfo?(annevk)

Comment 24

9 months ago
Probably best to ask Mike if comment 23 is feasible. He wrote that part of the DOM Standard.
Flags: needinfo?(annevk) → needinfo?(miket)
I've filed https://github.com/whatwg/dom/issues/404 . We can probably discuss there.
Flags: needinfo?(miket)
Whiteboard: [webcompat]
status-firefox51: affected → wontfix
status-firefox54: --- → affected
tracking-firefox51: ? → ---
tracking-firefox54: --- → ?
Comment hidden (mozreview-request)
https://github.com/whatwg/dom/issues/404 has been closed. We can try moving forward with the original patch now.
Comment on attachment 8831604 [details]
Bug 1332699 - Don't check handler of legacy version if the event is not trusted.

Somehow MozReview doesn't reset the review request although I think I've use a different commit ID...

smaug, this is the old patch which masayuki r+ed but you were worried about.

I've pushed the spec to change, and the new behavior matches Edge. It also seems to me that Blink is going to follow as well. Are you fine with this now?
Attachment #8831604 - Flags: review+ → review?(bugs)

Comment 29

8 months ago
mozreview-review
Comment on attachment 8831604 [details]
Bug 1332699 - Don't check handler of legacy version if the event is not trusted.

https://reviewboard.mozilla.org/r/108156/#review113174
Attachment #8831604 - Flags: review?(bugs) → review+

Comment 30

8 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce7561a6dea8
Don't check handler of legacy version if the event is not trusted. r=smaug

Comment 31

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ce7561a6dea8
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(xidorn+moz)
Tracking this regression for 52/53 and 54.
tracking-firefox52: ? → +
tracking-firefox53: ? → +
tracking-firefox54: ? → +
Comment on attachment 8831604 [details]
Bug 1332699 - Don't check handler of legacy version if the event is not trusted.

Approval Request Comment
[Feature/Bug causing the regression]: bug 743198
[User impact if declined]: take long time to enter fullscreen in some pages using the given library
[Is this code covered by automated tests?]: a test is added
[Has the fix been verified in Nightly?]: landed in nightly
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: this follows a behavior change in the spec, so it could expose certain level of compatibility risk, but I don't expect that to be high, as the new behavior matches Edge
[Why is the change risky/not risky?]: see above
[String changes made/needed]: n/a

Given its risk, I don't think we should uplift it to beta.
Flags: needinfo?(xidorn+moz)
Attachment #8831604 - Flags: approval-mozilla-aurora?
status-firefox52: affected → wontfix
Hi Florin,
could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(florin.mezei)
(In reply to Gerry Chang [:gchang] from comment #35)
> Hi Florin,
> could you help verify if this issue is fixed as expected on the latest
> Nightly build? Thanks!

Forwarding this to Brindusa for verification on Nightly -- instructions in Comment 0.
Flags: needinfo?(florin.mezei) → needinfo?(brindusa.tot)
Flags: qe-verify+
Verified as fixed, with instructions from Comment 0, on latest Nightly 54.0a1, Build ID 20170214030231.
Tested bug on Windows 10x64, Ubuntu 16.04, Mac OS X 10.10 and 10.12
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Flags: needinfo?(brindusa.tot)
Comment on attachment 8831604 [details]
Bug 1332699 - Don't check handler of legacy version if the event is not trusted.

Fix a fullscreen issue and was verified in nightly. Aurora53+.
Attachment #8831604 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 39

8 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/252e3d832683
status-firefox53: affected → fixed
Verified as fixed using Firefox 53 beta 1 under Win 10 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.11.
status-firefox53: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.