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

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[tomasz.oponowicz]

Attachment: 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

[Loic]

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1332699 - Don't check handler of legacy version if the event is not trusted.

Review commit: https://reviewboard.mozilla.org/r/106432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106432/

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Do other browsers check isTrusted in case of webkit* events?
Last time I checked blink's code, they didn't, IIRC.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 9

[tomasz.oponowicz]

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.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(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

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

But... yeah that sounds very hacky...

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

oh, hmm, edge doesn't seem to dispatch prefixed event from untrusted events.
Maybe we could follow that model after all.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Er, hmm, maybe it doesn't have fullscreenchange at all.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 17

[Mike Taylor [:miketaylr]]

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

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1332699 - Don't check handler of legacy version if the event is not trusted.

Review commit: https://reviewboard.mozilla.org/r/108156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108156/

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

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?

Comment 24

[Anne (:annevk)]

Probably best to ask Mike if comment 23 is feasible. He wrote that part of the DOM Standard.

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

I've filed https://github.com/whatwg/dom/issues/404 . We can probably discuss there.

Comment 26

[Xidorn Quan [:xidorn] UTC+10]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108156/diff/1-2/

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

https://github.com/whatwg/dom/issues/404 has been closed. We can try moving forward with the original patch now.

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

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?

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ce7561a6dea8

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 33

[Marcia Knous [:marcia]]

Tracking this regression for 52/53 and 54.

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 35

[Gerry Chang [:gchang]]

Hi Florin,
could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!

Comment 36

[Andrei Vaida [:avaida]]

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

Comment 37

[Brindusa Tot, Desktop QA]

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

Comment 38

[Gerry Chang [:gchang]]

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

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/252e3d832683

Comment 40

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed using Firefox 53 beta 1 under Win 10 64-bit, Ubuntu 14.04 64-bit and Mac OS X 10.11.