Opened 9 years ago

Closed 6 years ago

#6504 closed Bug (fixed)

Race condition while loading several customConfig files

Reported by: Alfonso Martínez de Lizarrondo Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 4.1.2
Component: General Version:
Keywords: Cc: k@…, glen.84@…, jean-francois.fournier@…

Description

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.

Attachments (3)

testcase 6504.zip (2.1 KB) - added by Alfonso Martínez de Lizarrondo 9 years ago.
testcase
6504.patch (3.4 KB) - added by Alfonso Martínez de Lizarrondo 9 years ago.
Proposed patch
6504_1.patch (3.4 KB) - added by Jakub Ś 7 years ago.

Download all attachments as: .zip

Change History (21)

Changed 9 years ago by Alfonso Martínez de Lizarrondo

Attachment: testcase 6504.zip added

testcase

Changed 9 years ago by Alfonso Martínez de Lizarrondo

Attachment: 6504.patch added

Proposed patch

comment:1 Changed 9 years ago by Alfonso Martínez de Lizarrondo

Status: newreview

comment:2 Changed 7 years ago by klemens_u

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

comment:3 Changed 7 years ago by klemens_u

Cc: k@… added

Any news for this? This is a major regression!

comment:4 Changed 7 years ago by 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?

comment:5 Changed 7 years ago by 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.

Changed 7 years ago by Jakub Ś

Attachment: 6504_1.patch added

comment:6 Changed 7 years ago by Jakub Ś

Status: reviewreview_failed

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.

comment:7 Changed 7 years ago by 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?

comment:8 Changed 7 years ago by darkangel

Cc: glen.84@… added

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))"

comment:9 Changed 7 years ago by 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.

comment:10 Changed 7 years ago by 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.

Last edited 7 years ago by Jakub Ś (previous) (diff)

comment:11 Changed 7 years ago by Jeff Fournier

Cc: jean-francois.fournier@… added

Adding myslef to cc.

comment:12 Changed 6 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 4.1.2

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.

comment:13 Changed 6 years ago by Olek Nowodziński

Owner: changed from Alfonso Martínez de Lizarrondo to Olek Nowodziński
Status: review_failedassigned

comment:14 Changed 6 years ago by Olek Nowodziński

Status: assignedreview

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.

comment:15 Changed 6 years ago by Frederico Caldeira Knabben

Owner: changed from Olek Nowodziński to 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+.

comment:16 Changed 6 years ago by Frederico Caldeira Knabben

Btw, here again the mt looks like inappropriate.

comment:17 in reply to:  15 Changed 6 years ago by Olek Nowodziński

Status: reviewreview_passed

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.

comment:18 Changed 6 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

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

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy