Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Selection check infinite loop kicks in too often #3785

Closed
Reinmar opened this issue Jul 18, 2016 · 7 comments
Closed

Selection check infinite loop kicks in too often #3785

Reinmar opened this issue Jul 18, 2016 · 7 comments
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Jul 18, 2016

  1. Go to https://ckeditor5.github.io/
  2. Put selection in the first heading.
  3. Start pressing up, down, up, down arrows quickly.

If you do it frequently enough and repeat enough times you got:

selectionchange-infinite-loop: Selection change observer detected an infinite rendering loop. undefined

and the seleciton stops refreshing (the label of the headings dropdown stops updating).

Now, god bless our marvellous idea to log stuff like this to the console!!! Imagine the hours spent on debugging this security check if it ended up with a silent errror.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 21, 2016

This is even easier to trigger (which makes it pretty critical). Just keep pressing backspace for a longer time. The warning will be logged pretty quickly.

@scofalik
Copy link
Contributor

scofalik commented Oct 5, 2016

After fixing #3701 selection infinite loop kicks in also happen at the edge of <strong> element.

@scofalik
Copy link
Contributor

scofalik commented Nov 2, 2016

This is even easier to trigger (which makes it pretty critical). Just keep pressing backspace for a longer time. The warning will be logged pretty quickly.

I can't reproduce this. However in this case:

Go to https://ckeditor5.github.io/
Put selection in the first heading.
Start pressing up, down, up, down arrows quickly.

The warning/check kicks in incorrectly. When you comment out the whole if nothing breaks or crashes (no infinite loop).

@scofalik
Copy link
Contributor

scofalik commented Nov 2, 2016

Okay it appears that the _loopbackCounter in SelectionObserver is not cleared on user action which is just wrong. You can trigger infinite loop by pressing left and right arrow key and you don't even have to do it fast. The infinite loop condition should kick in only if there were multiple dom selection changes without user interaction. Do you have an idea how to check it without having interaction between other observers?

Maybe clear it on timeout? Or add keydown and mousedown event listeners here: https://github.com/ckeditor/ckeditor5-engine/blob/master/src/view/observer/selectionobserver.js#L104 that will clear it.

@Reinmar
Copy link
Member Author

Reinmar commented Nov 3, 2016

I think that what we want to check is the number of selection changes in a period of time. Like whether 100 last changes happened in a second. If that's true, then we have loop.

The first way to implement this which comes to my mind is having an array of last change timestamps. You add the current one and checks the one that you remove (like in a FIFO queue). I guess there may be smarter ways too :D.

Anyway, this should not fail us. In fact, this is how I thought it worked.

@Reinmar
Copy link
Member Author

Reinmar commented Nov 3, 2016

PS. The numbers must be chosen in such a way that it won't fail in automated tests :D. But it should be fine if in every beforeEach() we create a new observer. If not, then this is test what's broken.

@Reinmar
Copy link
Member Author

Reinmar commented Nov 16, 2016

Fixed by ckeditor/ckeditor5-engine#671.

@Reinmar Reinmar closed this as completed Nov 16, 2016
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 5 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

3 participants