Opened 6 years ago

Closed 6 years ago

#5626 closed Bug (fixed)

CKeditor 3.2.1 : html content attached makes ckeditor crash the browser FF/IE

Reported by: amedina5511 Owned by: paho
Priority: Normal Milestone: CKEditor 3.4.2
Component: General Version: 3.2.1
Keywords: Cc: alex@…

Description

Tested in Ckeditor 3.2.1 demo site. I tested mostly with FF but it was also reported with IE.

The content in the file attached causes FF and IE to utilize tons of memory and the browser reports a script not responding alert error ("unresponsive script" in FF). If you click on "Continue" in the browser alert window, the browser crashes.

The attached small_html_ckeditor_bug.html web page causes this error with very little html code.

Notes/observations:

  • If you only remove the the center tags, the problem goes away.
  • If you only remove the last li tags, the problem goes away.
  • if you only remove the inner table, the problem goes away.

Attachments (4)

small_html_ckeditor_bug.html (1.2 KB) - added by amedina5511 6 years ago.
5626.patch (1.1 KB) - added by garry.yao 6 years ago.
5626_ref.patch (750 bytes) - added by paho 6 years ago.
Patch
5626_2.patch (543 bytes) - added by paho 6 years ago.
Patch

Download all attachments as: .zip

Change History (20)

Changed 6 years ago by amedina5511

comment:1 Changed 6 years ago by garry.yao

  • Keywords Confirmed added
  • Milestone set to CKEditor 3.4
  • Priority changed from High to Normal

Reproduced in fullpage mode.

Changed 6 years ago by garry.yao

comment:2 Changed 6 years ago by garry.yao

  • Owner set to garry.yao
  • Status changed from new to assigned

Ticket Test added:
run OR view source.

comment:3 Changed 6 years ago by fredck

  • Milestone changed from CKEditor 3.4 to CKEditor 3.5

I think the idea behind the patch, as well as the tests are wrong. Leaving those <li>s as is could give unexpected results on the editor operations.

comment:4 Changed 6 years ago by tobiasz.cudnik

  • Owner garry.yao deleted
  • Status changed from assigned to new

comment:5 Changed 6 years ago by tobiasz.cudnik

  • Keywords Confirmed removed
  • Status changed from new to confirmed

comment:6 Changed 6 years ago by tobiasz.cudnik

  • Owner set to tobiasz.cudnik
  • Status changed from confirmed to assigned

comment:7 Changed 6 years ago by tobiasz.cudnik

  • Status changed from assigned to review

After deeper checking, i think that Garry's solution is fine, as all LIs are still fixed by a code executed before the new one.

I'm opting for another review.

comment:8 Changed 6 years ago by Saare

  • Status changed from review to review_failed

I tend to agree with Fred. We should opt to fix this code (gather this items in lists etc.) and not leaving it as is. For example now we can not use the list properties dialog on the list items.

comment:9 Changed 6 years ago by paho

If there is <LI> inside <TD> it's not allowed in DTD. We create new element <UL> between <TD> and <LI> to have <TD><UL><LI>.

Changed 6 years ago by paho

Patch

comment:10 Changed 6 years ago by paho

  • Owner changed from tobiasz.cudnik to paho
  • Status changed from review_failed to review

comment:11 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

The idea is good, but why you're moving the children? Test case:

<div>
	text
	<li>item</li>
</div>

produces

<div>
	<ul>
		text
		<li>
			item</li>
	</ul>
</div>

comment:12 Changed 6 years ago by garry.yao

Also, we must also not add the 'ul' directly, the <ul> might not fit in position as well, it should be something like:

parser.onTagOpen( 'ul', {} );

Changed 6 years ago by paho

Patch

comment:13 Changed 6 years ago by paho

  • Status changed from review_failed to review

comment:14 Changed 6 years ago by garry.yao

  • Status changed from review to review_passed

comment:15 Changed 6 years ago by garry.yao

Ticket test updated with [5989].

comment:16 Changed 6 years ago by paho

  • Resolution set to fixed
  • Status changed from review_passed to closed

Fixed with [5990].

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