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

[#14613] prevent plugin initialization if editor was already destroyed… #273

Merged
merged 1 commit into from
Jul 25, 2018

Conversation

brianmay27
Copy link

…d (race condition)

@brianmay27
Copy link
Author

I tried to figure out how to functional test this. I was able to reproduce this and catch the error with a event listener but could not figure out how to wait a few seconds for the call back to be called. Im happy to finish the test but need a little help :)

@brianmay27
Copy link
Author

Any updates? We would like to get this in as we have been seeing issues around this race issue and would like to get it merged.

@mlewand
Copy link
Contributor

mlewand commented Mar 13, 2017

Hi @brianmay27 - sorry for the delay, we've been pretty busy for a long time. We should be able to review this PR soon.

@mlewand
Copy link
Contributor

mlewand commented Mar 13, 2017

I see that we need manual / unit tests - but the problem is pretty clear, so info for the reviewer: before bouncing the ticket, please give a try to reproduce it on your own, if no problem with this just create mt, and unit tests - as it should be fairly simple.

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally the fix looks legit.

Most importantly we need to play a bit to write right unit tests to be protected from regression in a future and the code style needs to be adjusted.

We'll take it from here and add the missing parts in a followup pull request.

Thanks for the contribution!

@mlewand mlewand changed the base branch from master to t/2257 July 25, 2018 10:36
@mlewand mlewand merged commit 63af1dd into ckeditor:t/2257 Jul 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants