Opened 7 years ago

Closed 7 years ago

#5626 closed Bug (fixed)

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

Reported by: Alex Medina Owned by: Paweł Horzela
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 Alex Medina 7 years ago.
5626.patch (1.1 KB) - added by Garry Yao 7 years ago.
5626_ref.patch (750 bytes) - added by Paweł Horzela 7 years ago.
Patch
5626_2.patch (543 bytes) - added by Paweł Horzela 7 years ago.
Patch

Download all attachments as: .zip

Change History (20)

Changed 7 years ago by Alex Medina

comment:1 Changed 7 years ago by Garry Yao

Keywords: Confirmed added
Milestone: CKEditor 3.4
Priority: HighNormal

Reproduced in fullpage mode.

Changed 7 years ago by Garry Yao

Attachment: 5626.patch added

comment:2 Changed 7 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

Ticket Test added:
run OR view source.

comment:3 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4CKEditor 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 7 years ago by Tobiasz Cudnik

Owner: Garry Yao deleted
Status: assignednew

comment:5 Changed 7 years ago by Tobiasz Cudnik

Keywords: Confirmed removed
Status: newconfirmed

comment:6 Changed 7 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: confirmedassigned

comment:7 Changed 7 years ago by Tobiasz Cudnik

Status: assignedreview

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 7 years ago by Sa'ar Zac Elias

Status: reviewreview_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 7 years ago by Paweł Horzela

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 7 years ago by Paweł Horzela

Attachment: 5626_ref.patch added

Patch

comment:10 Changed 7 years ago by Paweł Horzela

Owner: changed from Tobiasz Cudnik to Paweł Horzela
Status: review_failedreview

comment:11 Changed 7 years ago by Garry Yao

Status: reviewreview_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 7 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 7 years ago by Paweł Horzela

Attachment: 5626_2.patch added

Patch

comment:13 Changed 7 years ago by Paweł Horzela

Status: review_failedreview

comment:14 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

comment:15 Changed 7 years ago by Garry Yao

Ticket test updated with [5989].

comment:16 Changed 7 years ago by Paweł Horzela

Resolution: fixed
Status: review_passedclosed

Fixed with [5990].

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