Race condition while loading several customConfig files

[Alfonso Martínez de Lizarrondo]

Reported in ​http://cksource.com/forums/viewtopic.php?f=11&t=20480

The problem is that if several different customConfig files are used, there's a risk that the load event for each one of them are not done synchronously: it fires for the first script, but it happens that the second one has also been loaded, and so it overwrites the CKEDITOR.editorConfig function and the first editor instance uses the config data of the second editor.

For me the solution is to create a queue of scripts that can't be loaded in parallel.

Attached simplified testcase (it fails most of the time in IE and sometimes in Firefox, other browsers might exhibit similar behaviors) and proposed solution.

[Alfonso Martínez de Lizarrondo]

testcase

[Alfonso Martínez de Lizarrondo]

Proposed patch

[klemens_u]

I have got this problem as well. Sadly, the patch does not apply for the current version. Please fix this!

[klemens_u]

Any news for this? This is a major regression!

[Alfonso Martínez de Lizarrondo]

Do you mean that this is important for you and in these three months you just have been waiting for someone else to update the patch?

[klemens_u]

Don't be rude, and I have to apologize if I sounded rude. Sadly I don't have the js skills to fix this myself. I'm a contributor to php open source projects, but in case of this js project my contribution is giving feedback on issues like that. Let me know if I can do anything to help. Recherche, whatever.

It's quite common to have more than one ckeditor on one page. So I'm surely not the only one experiencing that issue.

[Jakub Ś]

I have updated the patch (or I only think i have) but it doesn't seem to work with current version of editor 3.6.5.

Sample scriptRace has failed in Firefox and IE every time I have tried it.

[Kiroff]

I've similar problem. I've also applied the patches manually. The result is that the .queue command is called and the queue itself is full, but CKEDITOR.scriptLoader.advanceQueue() never gets called, thus no config loaded (for me) or may be I'm putting it in the wrong place?

[darkangel]

Just adding myself to CC, as I have the same issue.

I had to write something here, otherwise I get:

"Submission rejected as potential spam (Akismet says content is spam, StopForumSpam says this is spam (username))"

[Piotrek Koszuliński]

I wonder if removing ​this setTimeout would solve the issue. It would be strange if browsers were firing onload in different order than JS is executed.

[Jakub Ś]

#10377 was marked as duplicate.

Please see #10377 for another working test-case. In that ticket situation is a little different as it looks like second editor doesn't get configuration file assigned (TC is different there as config gets called from within another config called by editor)

NOTE: User @jffou17 has provided his fix for CKEditor 4.X in #10377.

[Jeff Fournier]

Adding myslef to cc.

[Frederico Caldeira Knabben]

Let's give it a try for the 4.1.2. If it'll be too complex, we may postpone it for the next minor.

[Olek Nowodziński]

Dev

• ​Created t/6504 branch.
• Implemented CKEDITOR.scriptLoader.queue method based on proposed patch. This one is somewhat simpler and gentle.

Tests

• Created t/6504 branch in tests-v4.
• Added mt for this ticket.
• Added tests for CKEDITOR.scriptLoader.queue.
• Added tests for customConfig race.

[Frederico Caldeira Knabben]

The proposed fix looks cool, except for the fact that the new queue() method was getting stuck when used more than once.

I've just pushed a fix for this, added by a new test. Other than this I would be ok fot a R+.

[Frederico Caldeira Knabben]

Btw, here again the mt looks like inappropriate.

[Olek Nowodziński]

Replying to fredck:

The proposed fix looks cool, except for the fact that the new queue() method was getting stuck when used more than once.

I've just pushed a fix for this, added by a new test. Other than this I would be ok fot a R+.

I'm cool with it.

[Olek Nowodziński]

Merged fix into master, dev (​​git:33a7150), tests (55c885d).